Skip to content

Commit 62ede5d

Browse files
buenaflorcursoragent
buenaflor
and
cursoragent
committed
fix(flutter): Release replay JNI refs
Release Android replay JNI references when worker isolates shut down and when replay integration handles are replaced. This prevents replay bitmap/config/native replay references from being retained after use. Fixes GH-3633 Co-Authored-By: Claude <[email protected]> Co-authored-by: Cursor <[email protected]>
1 parent 87fcdd8 commit 62ede5d

5 files changed

Lines changed: 130 additions & 31 deletions

File tree

‎packages/flutter/lib/src/isolate/isolate_worker.dart‎

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,9 @@ abstract class WorkerHandler {
129129
/// Handle request/response payloads sent from the host.
130130
/// Return value is sent back to the host. Default: no-op.
131131
FutureOr<Object?> onRequest(Object? payload) => {};
132+
133+
/// Release resources held by the worker handler before isolate shutdown.
134+
FutureOr<void> close() {}
132135
}
133136

134137
/// Runs the Sentry worker loop inside a background isolate.
@@ -151,8 +154,21 @@ void runWorker(
151154
inbox.listen((msg) async {
152155
if (msg == _shutdownCommand) {
153156
internalLogger.debug('${config.debugName}: isolate received shutdown');
154-
inbox.close();
155-
internalLogger.debug('${config.debugName}: isolate closed');
157+
try {
158+
await handler.close();
159+
} catch (exception, stackTrace) {
160+
internalLogger.error(
161+
'${config.debugName}: isolate failed to close handler',
162+
error: exception,
163+
stackTrace: stackTrace,
164+
);
165+
if (config.automatedTestMode) {
166+
rethrow;
167+
}
168+
} finally {
169+
inbox.close();
170+
internalLogger.debug('${config.debugName}: isolate closed');
171+
}
156172
return;
157173
}
158174

‎packages/flutter/lib/src/native/java/android_replay_recorder.dart‎

Lines changed: 53 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -56,23 +56,28 @@ class AndroidReplayRecorder extends ScheduledScreenshotRecorder {
5656
}
5757

5858
Future<void> _addReplayScreenshot(
59-
Screenshot screenshot, bool isNewlyCaptured) async {
59+
Screenshot screenshot,
60+
bool isNewlyCaptured,
61+
) async {
6062
final timestamp = screenshot.timestamp.millisecondsSinceEpoch;
6163

6264
try {
6365
final data = await screenshot.rawRgbaData;
6466
options.log(
65-
SentryLevel.debug,
66-
'$logName: captured screenshot ('
67-
'${screenshot.width}x${screenshot.height} pixels, '
68-
'${data.lengthInBytes} bytes)');
69-
70-
await _worker!.request(_WorkItem(
71-
timestamp: timestamp,
72-
data: data.buffer.asUint8List(),
73-
width: screenshot.width,
74-
height: screenshot.height,
75-
));
67+
SentryLevel.debug,
68+
'$logName: captured screenshot ('
69+
'${screenshot.width}x${screenshot.height} pixels, '
70+
'${data.lengthInBytes} bytes)',
71+
);
72+
73+
await _worker!.request(
74+
_WorkItem(
75+
timestamp: timestamp,
76+
data: data.buffer.asUint8List(),
77+
width: screenshot.width,
78+
height: screenshot.height,
79+
),
80+
);
7681
} catch (error, stackTrace) {
7782
options.log(
7883
SentryLevel.error,
@@ -96,7 +101,7 @@ class _AndroidReplayHandler extends WorkerHandler {
96101
final WorkerConfig _config;
97102
// Android Bitmap creation is a bit costly so we reuse it between captures.
98103
native.Bitmap? _bitmap;
99-
late final native.ReplayIntegration _nativeReplay;
104+
native.ReplayIntegration? _nativeReplay;
100105

101106
_AndroidReplayHandler(this._config) {
102107
_nativeReplay =
@@ -106,14 +111,25 @@ class _AndroidReplayHandler extends WorkerHandler {
106111
@override
107112
FutureOr<void> onMessage(Object? message) {
108113
internalLogger.warning(
109-
'${_config.debugName}: Unexpected fire-and-forget message: $message');
114+
'${_config.debugName}: Unexpected fire-and-forget message: $message',
115+
);
116+
}
117+
118+
@override
119+
FutureOr<void> close() {
120+
_bitmap?.release();
121+
_bitmap = null;
122+
123+
_nativeReplay?.release();
124+
_nativeReplay = null;
110125
}
111126

112127
@override
113128
FutureOr<Object?> onRequest(Object? payload) {
114129
if (payload is! _WorkItem) {
115-
internalLogger
116-
.warning('${_config.debugName}: Unexpected payload type: $payload');
130+
internalLogger.warning(
131+
'${_config.debugName}: Unexpected payload type: $payload',
132+
);
117133
return null;
118134
}
119135

@@ -129,21 +145,35 @@ class _AndroidReplayHandler extends WorkerHandler {
129145
}
130146
}
131147

132-
// https://developer.android.com/reference/android/graphics/Bitmap#createBitmap(int,%20int,%20android.graphics.Bitmap.Config)
133-
// Note: while the generated API is nullable, the docs say the returned value cannot be null..
134-
_bitmap ??= native.Bitmap.createBitmap$10(
135-
item.width, item.height, native.Bitmap$Config.ARGB_8888);
148+
if (_bitmap == null) {
149+
// https://developer.android.com/reference/android/graphics/Bitmap#createBitmap(int,%20int,%20android.graphics.Bitmap.Config)
150+
// Note: while the generated API is nullable, the docs say the returned value cannot be null..
151+
native.Bitmap$Config? bitmapConfig;
152+
try {
153+
bitmapConfig = native.Bitmap$Config.ARGB_8888;
154+
_bitmap = native.Bitmap.createBitmap$10(
155+
item.width,
156+
item.height,
157+
bitmapConfig,
158+
);
159+
} finally {
160+
bitmapConfig?.release();
161+
}
162+
}
136163

137164
jBuffer = JByteBuffer.fromList(item.data);
138165
_bitmap!.copyPixelsFromBuffer(jBuffer);
139166

140167
// TODO timestamp is currently missing in onScreenshotRecorded()
141-
_nativeReplay.onScreenshotRecorded(_bitmap!);
168+
_nativeReplay?.onScreenshotRecorded(_bitmap!);
142169

143170
return null;
144171
} catch (exception, stackTrace) {
145-
internalLogger.error('Failed to add replay screenshot',
146-
error: exception, stackTrace: stackTrace);
172+
internalLogger.error(
173+
'Failed to add replay screenshot',
174+
error: exception,
175+
stackTrace: stackTrace,
176+
);
147177
if (_config.automatedTestMode) {
148178
rethrow;
149179
}

‎packages/flutter/lib/src/native/java/sentry_native_java.dart‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,11 @@ class SentryNativeJava extends SentryNativeChannel {
4444
@visibleForTesting
4545
AndroidReplayRecorder? get testRecorder => _replayRecorder;
4646

47+
void _setNativeReplay(native.ReplayIntegration? nativeReplay) {
48+
_nativeReplay?.release();
49+
_nativeReplay = nativeReplay;
50+
}
51+
4752
@override
4853
void init(Hub hub) {
4954
initSentryAndroid(hub: hub, options: options, owner: this);
@@ -187,7 +192,7 @@ class SentryNativeJava extends SentryNativeChannel {
187192
Future<void> close() async {
188193
await _replayRecorder?.stop();
189194
await _envelopeSender?.close();
190-
_nativeReplay?.release();
195+
_setNativeReplay(null);
191196
return super.close();
192197
}
193198

‎packages/flutter/lib/src/native/java/sentry_native_java_init.dart‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -107,8 +107,9 @@ native.ReplayRecorderCallbacks? createReplayRecorderCallbacks({
107107
SentryId.fromId(replayIdString.toDartString(releaseOriginal: true));
108108

109109
owner._replayId = replayId;
110-
owner._nativeReplay =
111-
native.SentryFlutterPlugin.privateSentryGetReplayIntegration();
110+
owner._setNativeReplay(
111+
native.SentryFlutterPlugin.privateSentryGetReplayIntegration(),
112+
);
112113
owner._replayRecorder = AndroidReplayRecorder.factory(options);
113114
await owner._replayRecorder!.start();
114115
hub.configureScope((s) {
@@ -131,6 +132,7 @@ native.ReplayRecorderCallbacks? createReplayRecorderCallbacks({
131132
final future = owner._replayRecorder?.stop();
132133
owner._replayRecorder = null;
133134
await future;
135+
owner._setNativeReplay(null);
134136
},
135137
replayReset: () {
136138
// ignored
@@ -236,17 +238,18 @@ void configureAndroidOptions({
236238

237239
native.SdkVersion? sdkVersion = androidOptions.getSdkVersion()
238240
?..releasedBy(arena);
241+
final versionName = native.BuildConfig.VERSION_NAME!..releasedBy(arena);
242+
final versionNameString = versionName.toDartString();
239243
if (sdkVersion == null) {
240244
sdkVersion = native.SdkVersion(
241245
androidSdkName.toJString()..releasedBy(arena),
242-
native.BuildConfig.VERSION_NAME!..releasedBy(arena),
246+
versionName,
243247
)..releasedBy(arena);
244248
} else {
245249
sdkVersion.setName(androidSdkName.toJString()..releasedBy(arena));
246250
}
247251
androidOptions.setSentryClientName(
248-
'$androidSdkName/${native.BuildConfig.VERSION_NAME}'.toJString()
249-
..releasedBy(arena));
252+
'$androidSdkName/$versionNameString'.toJString()..releasedBy(arena));
250253
androidOptions
251254
.setNativeSdkName(nativeSdkName.toJString()..releasedBy(arena));
252255
for (final integration in options.sdk.integrations) {

‎packages/flutter/test/isolate/isolate_worker_test.dart‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,22 @@ class _DebugNameHandler extends WorkerHandler {
5151
}
5252
}
5353

54+
class _CloseAckHandler extends WorkerHandler {
55+
SendPort? _closeAckPort;
56+
57+
@override
58+
Future<void> onMessage(Object? message) async {
59+
if (message is SendPort) {
60+
_closeAckPort = message;
61+
}
62+
}
63+
64+
@override
65+
void close() {
66+
_closeAckPort?.send('closed');
67+
}
68+
}
69+
5470
void _entryEcho((SendPort, WorkerConfig) init) {
5571
final (host, config) = init;
5672
runWorker(config, host, _EchoHandler());
@@ -71,6 +87,11 @@ void _entryDebugName((SendPort, WorkerConfig) init) {
7187
runWorker(config, host, _DebugNameHandler());
7288
}
7389

90+
void _entryCloseAck((SendPort, WorkerConfig) init) {
91+
final (host, config) = init;
92+
runWorker(config, host, _CloseAckHandler());
93+
}
94+
7495
void main() {
7596
group('Worker isolate', () {
7697
test('request/response echoes', () async {
@@ -198,5 +219,29 @@ void main() {
198219
worker.close();
199220
}
200221
});
222+
223+
test('close notifies handler before shutdown', () async {
224+
final worker = await spawnWorker(
225+
const WorkerConfig(
226+
debug: true,
227+
diagnosticLevel: SentryLevel.debug,
228+
debugName: 'CloseAckWorker',
229+
),
230+
_entryCloseAck,
231+
);
232+
final closeAck = ReceivePort();
233+
try {
234+
worker.send(closeAck.sendPort);
235+
worker.close();
236+
237+
expect(
238+
await closeAck.first.timeout(const Duration(seconds: 5)),
239+
'closed',
240+
);
241+
} finally {
242+
closeAck.close();
243+
worker.close();
244+
}
245+
});
201246
});
202247
}

0 commit comments

Comments
 (0)