Repository navigation
fix(android): dispose video renderers on engine detach - #2192
Merged
cloudwebrtc merged 1 commit intoSep 21, 2026
Merged
cloudwebrtc merged 1 commit into
cloudwebrtc merged 1 commit into
Conversation
MethodCallHandlerImpl.dispose() tore down localStreams, localTracks, and mPeerConnectionObservers, but never iterated the renders map (LongSparseArray<FlutterRTCVideoRenderer>). FlutterRTCVideoRenderer's Dispose() -- and therefore producer.release() on its TextureRegistry.SurfaceProducer -- never ran for renderers still alive at engine detach, so the producer stayed registered with the (now-detaching) engine. A video frame already buffered in the producer's ImageReader could then fire onImage after FlutterJNI had detached, crashing with: RuntimeException: Cannot execute operation because FlutterJNI is not attached to native. Since onDetachedFromEngine runs before flutterJNI.detachFromNativeAndReleaseResources(), releasing renderers as early as possible in dispose() -- before the existing streams/tracks/peer-connection teardown -- closes that window before JNI goes away. This ordering is also safer than doing it after: Dispose() calls setVideoTrack(null), which only removes a sink from a still-live VideoTrack; running it before the track/stream/peer-connection objects are disposed avoids the IllegalStateException catch path that could otherwise fire if a track were already disposed underneath it. The added loop and null guard, and clearing renders afterward, mirror this file's own existing conventions: the per-item try/catch matches the three teardown loops immediately below it, and clearing the map after matches localStreams.clear()/localTracks.clear()/ mPeerConnectionObservers.clear(). The null guard on valueAt(i) is defensive rather than load-bearing -- traced every renders.put() call site (only one, always a freshly-constructed non-null renderer) and confirmed it can never actually be null here, kept anyway since it's free and matches the shape of the fix suggested in the issue itself. Verification: no Android unit-test infrastructure exists for this class in this repo (the only existing test, RemoteTrackRegistryTest, covers a dependency-free pure-Java helper; no Robolectric/Mockito/ testOptions are configured), and no local emulator/device is available in this environment. Attempted `flutter build apk --debug` as a compile check; it fails identically on unmodified upstream/main with a pre-existing Gradle/AGP/Kotlin-plugin version mismatch in the example app's build config, confirmed via stash-and-rerun against a clean base (identical failure in under a second, before any Java compilation would even begin) -- unrelated to and unreachable by this change. Verification here is a full manual trace of every call path into dispose()/Dispose()/setVideoTrack()/renders mutation sites, described above. Fixes flutter-webrtc#2110
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The change is small, localized, consistent with existing teardown patterns in the same method, and directly addresses the documented engine-detach crash scenario.
Review effort: Lite
Findings: None
What changed in this PR
This PR fixes an Android crash/leak during Flutter engine detach by ensuring all live FlutterRTCVideoRenderer instances are disposed before the rest of the plugin teardown runs, releasing their underlying TextureRegistry.SurfaceProducer objects promptly.
Changes:
- Dispose every renderer in the
renders(LongSparseArray<FlutterRTCVideoRenderer>) map duringMethodCallHandlerImpl.dispose(). - Clear the renderer map immediately after disposal to avoid dangling references during teardown.
| File | Description |
|---|---|
| android/src/main/java/com/cloudwebrtc/webrtc/MethodCallHandlerImpl.java | Dispose and clear all live video renderers during engine detach teardown to prevent leaked producers / post-detach callbacks. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
cloudwebrtc
approved these changes
Sep 21, 2026
cloudwebrtc
pushed a commit
that referenced
this pull request
Sep 22, 2026
…d platform threads (#2194) ## Problem `common/cpp/src/flutter_data_channel.cc` shares a `data_channel_observers_` map (guarded by `base_->mutex_`) between two threads: - `OnDataChannel()` (in `flutter_peerconnection.cc`), which fires on **libwebrtc's signaling thread** when a remote peer opens a data channel, and correctly inserts under `base_->lock()`. - `DataChannelForId()` and `DataChannelClose()`, called from the **Flutter platform thread** for `dataChannelSend`/`dataChannelClose`, which read/erase the same map **without** taking that lock. This is the remaining native-side half of #2156 — the Dart-side half was already fixed in #2191. It's a real, unsynchronized concurrent `std::map` access, not just an ordering issue: a remote peer opening a channel can race a local `send()`/`close()` call on the same map. ## Fix Wrap the `find()` in `DataChannelForId()` and the `find()`+`erase()` in `DataChannelClose()` in the same `base_->lock()`/`base_->unlock()` pair already used by both insert sites (`CreateDataChannel()`, `OnDataChannel()`). `DataChannelForId()` was restructured to a single-exit shape so `unlock()` always runs. ## Verification No Windows/Linux native build toolchain is available in the environment this fix was developed in. Rather than skip verification, extracted the exact map+mutex locking shape (insert-under-lock from one thread, find/erase from another) into a standalone program and ran it under Clang ThreadSanitizer: - **Red:** the unfixed shape (matching current upstream code exactly) reliably reports `WARNING: ThreadSanitizer: data race` between the two threads. - **Green:** the fixed shape (matching this PR's change exactly) runs clean under the identical harness and thread pattern. This mirrors the same "extracted, disclosed" verification approach used in #2192, which merged cleanly. Fixes #2156.
hiroshihorie
added a commit
that referenced
this pull request
Oct 7, 2026
## Motivation Ship the fixes merged since 1.6.2+hotfix.3 so downstream packages can pin them. ## Summary Bumps `pubspec.yaml` to `1.6.2+hotfix.4` and adds the changelog section for: - #2190 Darwin: do not dereference a freed video renderer from the first frame callback. - #2186 Darwin: keep `sharedSingleton` on the plugin instance that owns the peer connection factory. - #2195 iOS: take the audio session configuration lock once in `ensureAudioSession`. - #2192 Android: dispose video renderers when the plugin detaches from the engine. - #2188 Android, Darwin, Windows and Linux: enable SCTP SNAP instead of DSCP marking when WARP is enabled. Windows and Linux move to libwebrtc m150.7871.03. - #2191 Dart: `RTCDataChannel.close()` moves the channel to the closed state, and `send()` after `close()` throws. - #2194 Windows/Linux: synchronize data channel observer map access across threads. Podspec versions stay at `1.6.2`, as in the earlier hotfix releases. `flutter pub publish --dry-run` reports no warnings.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2110
Problem
MethodCallHandlerImpl.dispose()tears downlocalStreams,localTracks, andmPeerConnectionObservers, but never iterates therendersmap (LongSparseArray<FlutterRTCVideoRenderer>).FlutterRTCVideoRenderer.Dispose()— and thereforeproducer.release()on itsTextureRegistry.SurfaceProducer— never runs for renderers still alive at engine detach, so the producer stays registered with the (now-detaching) engine. A video frame already buffered in the producer'sImageReadercan then fireonImageafterFlutterJNIhas detached, crashing withRuntimeException: Cannot execute operation because FlutterJNI is not attached to native.Fix
Releases every live renderer at the top of
dispose(), before the existing teardown, exactly as suggested in the issue. SinceonDetachedFromEngineruns beforeflutterJNI.detachFromNativeAndReleaseResources(), doing this as early as possible indispose()closes the crash window before JNI goes away.This ordering is also safer than doing it after the rest of teardown:
Dispose()callssetVideoTrack(null), which only removes a sink from a still-liveVideoTrack. Running it before tracks/streams/peer connections are disposed avoids theIllegalStateExceptioncatch path that could otherwise fire if a track were already disposed underneath it.The per-item
try/catchand therenders.clear()afterward mirror this file's own existing conventions for the three teardown loops immediately below. Theif (renderer != null)guard is defensive rather than load-bearing — I traced the onlyrenders.put()call site and confirmed it's always a freshly-constructed non-null renderer, so this can't actually be null today, but kept it since it's free and matches the shape suggested in the issue.Per the issue's own note, this fixes the plugin-side leak and the dominant crash case; the separate engine-side caveat (a frame already posted to the main
Looperbeforeproducer.release()could still slip through untilflutter/flutter#174859adds an attached-guard toscheduleFrame) is out of this plugin's control.Verification
No Android unit-test infrastructure exists for this class in this repo — the only existing test (
RemoteTrackRegistryTest) covers a dependency-free pure-Java helper, with no Robolectric/Mockito/testOptionsconfigured, and I don't have a local emulator or device available. I attemptedflutter build apk --debugas a compile check; it fails identically on unmodifiedupstream/mainwith a pre-existing Gradle/AGP/Kotlin-plugin version mismatch in the example app's build config (confirmed via stash-and-rerun against a clean base — identical failure in under a second, before any Java compilation would even begin), so it's unrelated to and unreachable by this change.Given that, verification here is a full manual trace instead: every
rendersmutation/read call site in the file, andFlutterRTCVideoRenderer.Dispose()/setVideoTrack()to confirm the ordering is safe (see above). Since you already have a reliable, documented repro for this (memory-pressure-drivenActivitydestruction during an active call), I'd appreciate it if you're able to smoke-test this branch against it — happy to iterate on anything that turns up.