Closed Bug 798829 Opened 8 years ago Closed 8 years ago

"ASSERTION: Using observer service off the main thread!" with getUserMedia

Categories

(Core :: WebRTC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19
Tracking Status
firefox17 --- unaffected
firefox18 + fixed
firefox19 + fixed

People

(Reporter: jruderman, Assigned: jesup)

References

Details

(Keywords: assertion, testcase, Whiteboard: [getUserMedia], [blocking-gum+], [qa-])

Attachments

(2 files, 1 obsolete file)

Attached file testcase
1. Set
     user_pref("media.navigator.enabled", true);
2. Load the testcase.
3. Quit Firefox.

Result:

###!!! ASSERTION: Using observer service off the main thread!: 'Error', file xpcom/ds/nsObserverService.cpp, line 95

mozilla::MediaManager::Get [nsCOMPtr.h:490]
mozilla::GetUserMediaRunnable::Run [MediaManager.cpp:339]
nsThread::ProcessNextEvent [nsThread.cpp:612]
NS_ProcessNextEvent_P [nsThreadUtils.cpp:220]
nsThread::ThreadFunc [nsThread.h:58]
_pt_root [ptthread.c:159]
libsystem_c.dylib + 0x4e8bf

(With a smaller loop bound, you might get memory leaks instead?)
Whiteboard: [getUserMedia], [blocking-gum+]
Need to get this fixed or make sure getUserMedia can't be enabled in FF18.
Randell, Tracking this for 18 as per comment 1 . Can you please help find an assignee for this ?
We aren't planning on enabling getUserMedia for FF 18 but this needs to be fixed nevertheless.
Assignee: nobody → anant
>> Need to get this fixed or make sure getUserMedia can't be enabled in FF18.

> We aren't planning on enabling getUserMedia for FF 18

Or did Smaug mean something stronger, like "ensure users can't even flip a pref to enable it"?
I did mean that. We should not advertize that one can enable media stuff on aurora if there is a
major bug like this. So better to either fix this or make sure the media stuff can't be used at all.
Also, this looks like something reasonable easy to fix.
I tried to repro this after writing bug 802982 comment 8, and got

Assertion failure: ((bool)(__builtin_expect(!!(!NS_FAILED_impl(rv)), 1))), at ../../../../toolkit/components/downloads/nsDownloadManager.cpp:299

I guess everything starts breaking when the process hits ~2048 threads (bug 798323).
http://mxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.h#196 - MediaManager::Get is called in a runnable off the main thread, and it performs observer service manipulation.
This singleton needs to be inited earlier on (and shut down as well, perhaps from nsLayoutStatics.cpp) so we don't have to create the object in ::Get(), OR ::Get() needs to send a runnable to MainThread and get a result before returning (when creating MediaManager).

Also, the singleton will inherently leak at shutdown along with its thread as currently implemented; if it returns an already_addrefed pointer (such as in NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR()) we can track references and we have a chance to cleaning it up (knowing there are no refs) on shutdown.

It doesn't need to be an XPCOM service obtainable by GetService, though I suppose it could be.

I'll throw together a patch that makes this safe for now until this work can get done.  It still will leak MediaManager (and thread) at shutdown.
Attachment #673768 - Flags: review?(anant)
Assignee: anant → rjesup
OS: Mac OS X → All
Hardware: x86_64 → All
Comment on attachment 673768 [details] [diff] [review]
Force MediaManager to be created from MainThread

We could move this to Navigator.cpp:MozGetUserMedia as well (which will always be called on the main thread), but this is good for now as well.
Attachment #673768 - Flags: review?(anant) → review+
https://hg.mozilla.org/mozilla-central/rev/6acf30e09a34
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Keywords: verifyme
Keywords: verifyme
Whiteboard: [getUserMedia], [blocking-gum+] → [getUserMedia], [blocking-gum+], [qa verification failed]
If verification failed, does the assertion occur?  I've tried this ~30 times, and do not see the assertion.
(In reply to Randell Jesup [:jesup] from comment #15)
> If verification failed, does the assertion occur?  I've tried this ~30
> times, and do not see the assertion.

Ah, I think an issue unrelated to this bug. So verification actually didn't fail in that case.
Whiteboard: [getUserMedia], [blocking-gum+], [qa verification failed] → [getUserMedia], [blocking-gum+], [qa-]
(In reply to Randell Jesup [:jesup] from comment #12)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/6acf30e09a34

Please uplift the patch for aurora fixing this bug if we are happy with the bake time/testing. Else may be we can backout patches related to getUserMedia or disable it completely for FF18 as per comment 1 & 3
Comment on attachment 673768 [details] [diff] [review]
Force MediaManager to be created from MainThread

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 752353 (or so)
User impact if declined: Crashes or worse *if* the feature is turned on by the user.
Testing completed (on m-c, etc.): Well-baked on m-c
Risk to taking this patch (and alternatives if risky): Almost no risk; well bakes and simple patch.  Also turned off by default, so there's no risk to normal users if left alone.
String or UUID changes made by this patch: none
Attachment #673768 - Flags: approval-mozilla-aurora?
Attachment #673768 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
trivial unbitrot for Aurora
Attachment #673768 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.