Closed Bug 829907 Opened 7 years ago Closed 7 years ago

Failure to exit when quitting with an active getUserMedia camera stream

Categories

(Core :: WebRTC: Audio/Video, defect)

All
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla21
Tracking Status
firefox19 --- unaffected
firefox20 + fixed
firefox21 + fixed

People

(Reporter: jesup, Assigned: jesup)

References

Details

(Keywords: hang, Whiteboard: [getUserMedia], [blocking-gum+])

Attachments

(2 files, 2 obsolete files)

Appears to be Mac-only (need to test on PC).  Latest inbound; leaving a local getUseMedia stream active and attached to a video element causes Mac Quit to hang.  (Not sure if being attached has anything to do with it; probably not).
Duplicate of this bug: 837788
We enabled GUM in Firefox 20 by default. So shouldn't we track this for the release given that any website could trigger that?
Hardware: x86 → All
(In reply to Henrik Skupin (:whimboo) [away 02/09 - 02/017] from comment #2)
> We enabled GUM in Firefox 20 by default. So shouldn't we track this for the
> release given that any website could trigger that?

Yeah, we should. We're planning on shipping on FF 20. So we probably need to track on that one.
This hang doesn't seem to happen in a debug build. Quitting Firefox with an active gUM stream works fine.
Attachment #713803 - Attachment is obsolete: true
We were releasing the GUM streams in GC (or thereabouts) in shutdown, before the xpcom-shutdown notification, from the MediaManager thread, and hanging somewhere inside of [_captureDevice release].  (ObjC)  This meant later we couldn't shut down the thread, and hung.

So the source of the problem turns out to be that QTKit uses sends to the main CFRunLoop thread to implement thread-safety.  The reason this stops working is that when we're exiting (mExiting is true in the MainThread nsAppShell), we stop processing native events:

http://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/nsBaseAppShell.cpp#294

We can't easily and safely hook in at an earlier point since we can't guarantee we process and finish it before shutdown proceeds too far.

The solution is to remove the need to do so by proxying the delete to the MainThread such that the native event send isn't needed.  (Thanks to bsmedberg for helps diagnose this)
Attachment #713813 - Flags: review?(benjamin)
We'll want to uplift this to Aurora ASAP.

Also note we trigger the Mac WritePoisoning code due to the webrtc trace impl flushing at exit if you set NSPR_LOG_MODULES to webrtc_trace:xxxxx.  This isn't new, but I hit it while testing this.  I'll file a separate bug on that.
Whiteboard: [getUserMedia], [blocking-gum+] → [getUserMedia], [blocking-gum+][webrtc-uplift]
Comment on attachment 713813 [details] [diff] [review]
release video capture device on MainThread (mac only)

Please use

nsCOMPtr<nsIThread> mainThread = do_GetMainThread();

or the even simpler NS_DispatchToMainThread(WrapRunnable(...), NS_DISPATCH_SYNC)
Attachment #713813 - Attachment is obsolete: true
Attachment #713813 - Flags: review?(benjamin)
Comment on attachment 713990 [details] [diff] [review]
release video capture device on MainThread (mac only)

revised per comment, added comments about NS_DISPATCH_SYNC.  Will retest on mac but no surprises expected
Attachment #713990 - Flags: review?(benjamin)
Attachment #713990 - Flags: review?(benjamin) → review+
https://hg.mozilla.org/mozilla-central/rev/ebeaa97d710d
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Keywords: verifyme
A different bug is tracking the work already for implementing automation here.
Flags: in-testsuite-
Comment on attachment 713990 [details] [diff] [review]
release video capture device on MainThread (mac only)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A (ever since we generally fixed shutdown to properly shut down)

User impact if declined: Failure to exit if you have a video camera active

Testing completed (on m-c, etc.): on m-c since 2/14.  Tested repeatedly by multiple developers.

Risk to taking this patch (and alternatives if risky): Low risk.  I verified the underlying webrtc.org code for invoking the release is threadsafe (uses critical sections, etc), so releasing it from mainthread is safe, and I verified we never DISPATCH_SYNC from mainthread to the mediamanager thread.

String or UUID changes made by this patch: none
Attachment #713990 - Flags: approval-mozilla-aurora?
Attachment #713990 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Not seeing a hang on shutdown on FF nightly on 2/22 build. Verified.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Whiteboard: [getUserMedia], [blocking-gum+][webrtc-uplift] → [getUserMedia], [blocking-gum+]
You need to log in before you can comment on or make changes to this bug.