829907 - Failure to exit when quitting with an active getUserMedia camera stream

Status: VERIFIED FIXED

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

Description

[Randell Jesup [:jesup] (needinfo me)]

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).

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

We enabled GUM in Firefox 20 by default. So shouldn't we track this for the release given that any website could trigger that?

Comment 3

[Jason Smith [:jsmith]]

(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.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This hang doesn't seem to happen in a debug build. Quitting Firefox with an active gUM stream works fine.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: activity monitor sample

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Attachment: release video capture device on MainThread (mac only)

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Attachment: release video capture device on MainThread (mac only)

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

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)

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 10

[Benjamin Smedberg]

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)

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Attachment: release video capture device on MainThread (mac only)

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/mozilla-central/rev/ebeaa97d710d

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

Some references:
http://stackoverflow.com/questions/5703754/addinput-method-of-qtcapturesession-not-returning
http://developer.apple.com/library/mac/#technotes/tn2125/_index.html
http://code.google.com/p/chromium/issues/detail?id=157725
http://lists.apple.com/archives/quicktime-api/2009/Apr/msg00101.html

Comment 15

[Jason Smith [:jsmith]]

A different bug is tracking the work already for implementing automation here.

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a581fbd356de

Comment 18

[Jason Smith [:jsmith]]

Not seeing a hang on shutdown on FF nightly on 2/22 build. Verified.