chore: cleanup for android on bridge reload#30
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 56 minutes and 7 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded explicit disposal flows: new Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant WRM as WebRTCModule
participant Exec as ThreadUtils Executor
participant GMI as GetUserMediaImpl
participant PCO as PeerConnectionObserver
participant MS as MediaStream
participant Fac as PeerConnectionFactory
App->>WRM: invalidate()
WRM->>Exec: submit cleanup task (blocking .get())
Exec-->>WRM: task executes
rect rgba(100,150,200,0.5)
WRM->>PCO: for each observer -> dispose()
PCO-->>WRM: observer disposed
end
rect rgba(150,100,200,0.5)
WRM->>GMI: disposeAllTracks()
GMI->>GMI: iterate and dispose each TrackPrivate
GMI-->>WRM: tracks cleared
end
rect rgba(200,150,100,0.5)
WRM->>MS: for each local stream -> remove tracks & dispose stream
MS-->>WRM: streams disposed
end
rect rgba(150,200,100,0.5)
WRM->>Fac: dispose() and set null
Fac-->>WRM: factory disposed
end
WRM->>WRM: super.invalidate()
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@android/src/main/java/com/oney/WebRTCModule/GetUserMediaImpl.java`:
- Around line 358-359: When detecting webRTCModule.mFactory == null in the
onServiceConnected guard, reject and clear any pending display-media request
before returning: if webRTCModule.displayMediaPromise (or the field holding the
pending promise from getDisplayMedia()) is non-null, call its reject/resolve
with an appropriate error (e.g., "Operation aborted" or the same error used
elsewhere), then set webRTCModule.displayMediaPromise = null (and clear any
related state like a pending displayMediaStreamId) so the pending
getDisplayMedia() call is settled and future calls won't hit "Another operation
is pending."; keep this logic immediately before the existing return in
GetUserMediaImpl where mFactory is checked.
In `@android/src/main/java/com/oney/WebRTCModule/WebRTCModule.java`:
- Around line 147-155: The loop that disposes localStreams in
WebRTCModule.dispose is unguarded and a thrown exception from any
stream.dispose() can abort remaining teardowns and prevent localStreams.clear()
and mFactory.dispose(); fix by wrapping each stream.dispose() call in a
try/catch (catch Throwable) and log the exception using the same
log-and-continue pattern used elsewhere (e.g., PeerConnection teardown), so the
loop continues for all entries, and ensure that after the loop
localStreams.clear() and the existing mFactory disposal still execute.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9db95331-1170-476b-9fae-897602a33a43
📒 Files selected for processing (2)
android/src/main/java/com/oney/WebRTCModule/GetUserMediaImpl.javaandroid/src/main/java/com/oney/WebRTCModule/WebRTCModule.java
There was a problem hiding this comment.
🧹 Nitpick comments (1)
android/src/main/java/com/oney/WebRTCModule/GetUserMediaImpl.java (1)
288-297: Use a snapshot when disposingtracksto avoid fail-fast iteration issues.
disposeAllTracks()currently iterates the live map while invokingdispose(). If disposal indirectly mutatestracks, teardown can fail withConcurrentModificationException.Proposed defensive change
void disposeAllTracks() { - for (Map.Entry<String, TrackPrivate> entry : tracks.entrySet()) { + for (Map.Entry<String, TrackPrivate> entry : new ArrayList<>(tracks.entrySet())) { try { entry.getValue().dispose(); } catch (Exception e) { Log.w(TAG, "disposeAllTracks: error disposing " + entry.getKey(), e); } } tracks.clear(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/src/main/java/com/oney/WebRTCModule/GetUserMediaImpl.java` around lines 288 - 297, disposeAllTracks currently iterates the live tracks map and calls TrackPrivate.dispose(), which may mutate the tracks map and cause ConcurrentModificationException; fix by iterating over a snapshot of the entries (e.g., copy tracks.entrySet() into a new list) and call entry.getValue().dispose() against that snapshot, still catching exceptions per entry, and then clear the original tracks map once iteration completes to ensure safe teardown.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@android/src/main/java/com/oney/WebRTCModule/GetUserMediaImpl.java`:
- Around line 288-297: disposeAllTracks currently iterates the live tracks map
and calls TrackPrivate.dispose(), which may mutate the tracks map and cause
ConcurrentModificationException; fix by iterating over a snapshot of the entries
(e.g., copy tracks.entrySet() into a new list) and call
entry.getValue().dispose() against that snapshot, still catching exceptions per
entry, and then clear the original tracks map once iteration completes to ensure
safe teardown.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b5424c14-30d4-4e20-901d-1042e4b07a87
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
android/src/main/java/com/oney/WebRTCModule/GetUserMediaImpl.javaandroid/src/main/java/com/oney/WebRTCModule/WebRTCModule.javapackage.json
✅ Files skipped from review due to trivial changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- android/src/main/java/com/oney/WebRTCModule/WebRTCModule.java
2 participants
Summary by CodeRabbit
Bug Fixes
Chores