Testcase found while fuzzing mozilla-central rev 20170925-5f3f19824efa. Please note the testcase must be served up by a local webserver (i.e. python -m SimpleHTTPServer) ==8208==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f97b6ce0362 bp 0x7ffc15f29a90 sp 0x7ffc15f299e0 T0) ==8208==The signal is caused by a WRITE memory access. ==8208==Hint: address points to the zero page. #0 0x7f97b6ce0361 in fetch_add /builds/worker/workspace/build/src/clang/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.9.4/../../../../include/c++/4.9.4/bits/atomic_base.h:618:16 #1 0x7f97b6ce0361 in operator++ /builds/worker/workspace/build/src/obj-firefox/dist/include/nsISupportsImpl.h:346 #2 0x7f97b6ce0361 in AddRef /builds/worker/workspace/build/src/dom/media/MediaStreamGraph.h:254 #3 0x7f97b6ce0361 in AddRef /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:38 #4 0x7f97b6ce0361 in AddRef /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:395 #5 0x7f97b6ce0361 in RefPtr /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:112 #6 0x7f97b6ce0361 in mozilla::SourceListener::Stop() /builds/worker/workspace/build/src/dom/media/MediaManager.cpp:3621 #7 0x7f97b6ce21c0 in NotifyFinished /builds/worker/workspace/build/src/dom/media/MediaManager.cpp:3850:3 #8 0x7f97b6ce21c0 in mozilla::SourceListener::NotifyRemoved() /builds/worker/workspace/build/src/dom/media/MediaManager.cpp:3862 #9 0x7f97b6cfbc23 in mozilla::GetUserMediaWindowListener::~GetUserMediaWindowListener() /builds/worker/workspace/build/src/dom/media/MediaManager.cpp:614:10 #10 0x7f97b6d18e28 in Release /builds/worker/workspace/build/src/dom/media/MediaManager.cpp:321:3 #11 0x7f97b6d18e28 in Release /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:41 #12 0x7f97b6d18e28 in Release /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:398 #13 0x7f97b6d18e28 in ~RefPtr /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:79 #14 0x7f97b6d18e28 in mozilla::GetUserMediaTask::~GetUserMediaTask() /builds/worker/workspace/build/src/dom/media/MediaManager.cpp:1473 #15 0x7f97b6d18ffd in mozilla::GetUserMediaTask::~GetUserMediaTask() /builds/worker/workspace/build/src/dom/media/MediaManager.cpp:1472:23 #16 0x7f97b1bee30b in mozilla::Runnable::Release() /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:44:1 #17 0x7f97b6d11efd in Release /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:41:11 #18 0x7f97b6d11efd in Release /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:398 #19 0x7f97b6d11efd in ~RefPtr /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:79 #20 0x7f97b6d11efd in ~nsBaseHashtableET /builds/worker/workspace/build/src/obj-firefox/dist/include/nsBaseHashtable.h:431 #21 0x7f97b6d11efd in nsTHashtable<nsBaseHashtableET<nsStringHashKey, RefPtr<mozilla::GetUserMediaTask> > >::s_ClearEntry(PLDHashTable*, PLDHashEntryHdr*) /builds/worker/workspace/build/src/obj-firefox/dist/include/nsTHashtable.h:448 #22 0x7f97b1ac633b in ~PLDHashTable /builds/worker/workspace/build/src/xpcom/ds/PLDHashTable.cpp:325:7 #23 0x7f97b1ac633b in PLDHashTable::ClearAndPrepareForLength(unsigned int) /builds/worker/workspace/build/src/xpcom/ds/PLDHashTable.cpp:340 #24 0x7f97b6cd9b3e in Clear /builds/worker/workspace/build/src/obj-firefox/dist/include/nsTHashtable.h:277:12 #25 0x7f97b6cd9b3e in Clear /builds/worker/workspace/build/src/obj-firefox/dist/include/nsBaseHashtable.h:373 #26 0x7f97b6cd9b3e in mozilla::MediaManager::Shutdown() /builds/worker/workspace/build/src/dom/media/MediaManager.cpp:3067 #27 0x7f97b6cf89a4 in mozilla::MediaManager::Get()::Blocker::BlockShutdown(nsIAsyncShutdownClient*) /builds/worker/workspace/build/src/dom/media/MediaManager.cpp:1929:38 #28 0x7f97b1c0bce1 in NS_InvokeByIndex /builds/worker/workspace/build/src/xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_x86_64_unix.S:129 #29 0x7f97b33a7a60 in Invoke /builds/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNative.cpp:1996:12 #30 0x7f97b33a7a60 in Call /builds/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNative.cpp:1315 #31 0x7f97b33a7a60 in XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) /builds/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNative.cpp:1282 #32 0x7f97b33aea2f in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:929:12 #33 0x7f97bc8c2584 in CallJSNative /builds/worker/workspace/build/src/js/src/jscntxtinlines.h:293:15 #34 0x7f97bc8c2584 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:495 #35 0x7f97bc8ac3e6 in CallFromStack /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:546:12 #36 0x7f97bc8ac3e6 in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3084 #37 0x7f97bc8937ab in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:435:12 #38 0x7f97bc8c271c in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:513:15 #39 0x7f97bc8c3072 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:559:10 #40 0x7f97bc9a6e17 in js::PromiseObject::create(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, bool) /builds/worker/workspace/build/src/js/src/builtin/Promise.cpp:1655:19 #41 0x7f97bca7cf14 in PromiseConstructor(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/js/src/builtin/Promise.cpp:1585:40 #42 0x7f97bc8c371e in CallJSNative /builds/worker/workspace/build/src/js/src/jscntxtinlines.h:293:15 #43 0x7f97bc8c371e in CallJSNativeConstructor /builds/worker/workspace/build/src/js/src/jscntxtinlines.h:326 #44 0x7f97bc8c371e in InternalConstruct(JSContext*, js::AnyConstructArgs const&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:586 #45 0x7f97bc8ac587 in ConstructFromStack /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:624:12 #46 0x7f97bc8ac587 in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3076 #47 0x7f97bc8937ab in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:435:12 #48 0x7f97bc8c271c in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:513:15 #49 0x7f97bcaf78ec in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/jit/BaselineIC.cpp:2589:14 #50 0x1267892e5306 (<unknown module>)

jib, pehrsons, one of you probably want to have a look at this one.

I tried to reproduce this in both opt and debug, with the pref "media.navigator.permission.disabled" both set and unset, but to no avail. However, from the stack trace in comment 0 this is to Stop() with mStream == nullptr. mStream will only be null until Activate() is called, hence we need to allow and protect from Stop()s before Activate().

> #21 0x7f97b6d11efd in nsTHashtable<nsBaseHashtableET<nsStringHashKey, RefPtr<mozilla::GetUserMediaTask> > It's odd this isn't reproducible. We're in shutdown cleanup of mActiveCallbacks. GetUserMediaTasks are queued here only while gUM permission requests are outstanding (e.g. while a prompt is left up unanswered). GetUserMediaTasks are expressly removed from here before anything exciting (MSG) happens. From the stack, I'd naively expect simply quitting the browser with a gUM prompt open to provoke this, but I'm not able to reproduce it. Maybe MediaManager::OnNavigation(aWindowId) normally cleans these up before MediaManager::Shutdown() does, and one escaped somehow?

Comment on attachment 8914289 [details] [diff] [review] Add guards and sanity checks for mActivated vs mStopped state Review of attachment 8914289 [details] [diff] [review]: ----------------------------------------------------------------- Maybe we should avoid Stop() instead? ::: dom/media/MediaManager.cpp @@ +3582,5 @@ > > LOG(("SourceListener %p activating audio=%p video=%p", this, aAudioDevice, aVideoDevice)); > > + if (mStopped) { > + NS_ERROR("Cannot activate stopped source listener"); When is this not a programming error? Shouldn't we use MOZ_ASSERT here like below? Also, this change seems unrelated to this bug. @@ +3617,4 @@ > > mStopped = true; > > + if (!mActivated) { This might work, but it seems wrong to me that a listener that has never been "activated" can be "stopped" and "finished". The description of "mFinished" says [1]: "true after the stream this listener is listening to has finished in the MediaStreamGraph." We set mFinished to true in NotifyFinished(), which is fine, but we call that from SourceListener::NotifyRemoved [2], which I think is the better place to fix this: SourceListener::NotifyRemoved() { MOZ_ASSERT(NS_IsMainThread()); LOG(("SourceListener removed, mFinished = %d", (int) mFinished)); mRemoved = true; - if (!mFinished) { + if (mActivated && !mFinished) { NotifyFinished(); } mWindowListener = nullptr; } With that in place, I don't see that we need to alter how Stop() works here. If anything, we might want to declare some state combinations illegal, and MOZ_ASSERT against them, e.g. that mInactiveListeners aren't mStopped on cleanup. [1] http://searchfox.org/mozilla-central/rev/298033405057ca7aa5099153797467eceeaa08b5/dom/media/MediaManager.cpp#276-278 [2] http://searchfox.org/mozilla-central/rev/298033405057ca7aa5099153797467eceeaa08b5/dom/media/MediaManager.cpp#3847 @@ +3632,2 @@ > RefPtr<SourceMediaStream> source = GetSourceStream(); > + if (!source) { Even if we undo the if (!mActivated) { above, I can't see when source would ever be null here. MOZ_ASSERT instead? @@ +3667,4 @@ > RefPtr<MediaDevice> device; > RefPtr<SourceMediaStream> source; > > + if (!mActivated) { MOZ_ASSERT instead.

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #5) > Comment on attachment 8914289 [details] [diff] [review] > Add guards and sanity checks for mActivated vs mStopped state > > Review of attachment 8914289 [details] [diff] [review]: > ----------------------------------------------------------------- > > Maybe we should avoid Stop() instead? > > ::: dom/media/MediaManager.cpp > @@ +3582,5 @@ > > > > LOG(("SourceListener %p activating audio=%p video=%p", this, aAudioDevice, aVideoDevice)); > > > > + if (mStopped) { > > + NS_ERROR("Cannot activate stopped source listener"); > > When is this not a programming error? Shouldn't we use MOZ_ASSERT here like > below? > > Also, this change seems unrelated to this bug. It's not part of a minimum fix, sure, but "unrelated" is far from that. I mean that this is related to mActivated vs mStopped state and which combinations of those are sane and which aren't, which is what this patch is fixing. > @@ +3617,4 @@ > > > > mStopped = true; > > > > + if (!mActivated) { > > This might work, but it seems wrong to me that a listener that has never > been "activated" can be "stopped" and "finished". > > The description of "mFinished" says [1]: "true after the stream this > listener is listening to has finished in the MediaStreamGraph." > > We set mFinished to true in NotifyFinished(), which is fine, but we call > that from SourceListener::NotifyRemoved [2], which I think is the better > place to fix this: > > SourceListener::NotifyRemoved() > { > MOZ_ASSERT(NS_IsMainThread()); > LOG(("SourceListener removed, mFinished = %d", (int) mFinished)); > mRemoved = true; > > - if (!mFinished) { > + if (mActivated && !mFinished) { > NotifyFinished(); > } > > mWindowListener = nullptr; > } > > With that in place, I don't see that we need to alter how Stop() works here. > If anything, we might want to declare some state combinations illegal, and > MOZ_ASSERT against them, e.g. that mInactiveListeners aren't mStopped on > cleanup. That's a good catch. I missed that NotifyRemoved() was called from the window listener destructor, even though it was staring at me from that stack trace. > [1] > http://searchfox.org/mozilla-central/rev/ > 298033405057ca7aa5099153797467eceeaa08b5/dom/media/MediaManager.cpp#276-278 > [2] > http://searchfox.org/mozilla-central/rev/ > 298033405057ca7aa5099153797467eceeaa08b5/dom/media/MediaManager.cpp#3847 > > @@ +3632,2 @@ > > RefPtr<SourceMediaStream> source = GetSourceStream(); > > + if (!source) { > > Even if we undo the > > if (!mActivated) { > > above, I can't see when source would ever be null here. MOZ_ASSERT instead? I prefer the extra suspenders. Obscure bugs are a thing. > @@ +3667,4 @@ > > RefPtr<MediaDevice> device; > > RefPtr<SourceMediaStream> source; > > > > + if (!mActivated) { > > MOZ_ASSERT instead.

Comment on attachment 8914696 [details] [diff] [review] Add guards and sanity checks for mActivated vs mStopped state Review of attachment 8914696 [details] [diff] [review]: ----------------------------------------------------------------- That's fine. It just took me a while to review these few lines. I found myself wishing for more invariants to grab hold of to understand what can and can't happen. ::: dom/media/MediaManager.cpp @@ +3632,3 @@ > RefPtr<SourceMediaStream> source = GetSourceStream(); > + if (!source) { > + MOZ_ASSERT(false, "Can't end tracks. No source stream."); OK, just an observation that mActivated and mStream seem 1-1. Unless I'm missing something about cleanup, they could almost be the same, e.g. mActivateStream maybe? With the check against !mActivated above, the only time GetSourceStream() would return null here would seem to be if mStream were of the wrong type, an invariant we probably could test for earlier (i.e. fail Activate())?

Also please let me know if you have any ideas about comment 4.

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #10) > Also please let me know if you have any ideas about comment 4. So looking into that I noticed how when I repro from devtools I get errors on window.close() (only being allowed from script when the window was created from script). However, there's a pref to work around that, "dom.allow_scripts_to_close_windows". With that set to true, and executing the triggering code in devtools calls OnNavigation() before the ShutdownBlocker, and so doesn't crash. On the other hand, when navigating to a page with the code (i.e., not using devtools) I can indeed repro. Even so in rr. I'll see if this can give any deeper analysis and a test case. FWIW to repro in a local build: 1 > ./mach run 2 Set the dom.allow_scripts_to_close_windows pref to true 3 close the browser 4 > cd /path/with/trigger-as-bug_1403186.html 5 > python -m SimpleHTTPServer & 6 > cd - 7 > ./mach run --disable-e10s --debugger rr localhost:8000/bug_1403186.html

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #4) > Maybe MediaManager::OnNavigation(aWindowId) normally cleans these up before > MediaManager::Shutdown() does, and one escaped somehow? Yes, OnNavigation normally cleans them up. Investigating the case where one escaped led to bug 1405599.

Jason, do these patches fix the original issue with your testcase?

(In reply to Al Billings [:abillings] from comment #13) > Jason, do these patches fix the original issue with your testcase? I tested both, with the patches, and the latest mozilla-central nightly. I cannot reproduce the issue with either.

This also simplifies the activated state in SourceListener.

Comment on attachment 8918322 [details] [diff] [review] Add guards and sanity checks for Activated vs Stopped state Review of attachment 8918322 [details] [diff] [review]: ----------------------------------------------------------------- lgtm ::: dom/media/MediaManager.cpp @@ +3764,3 @@ > if (!mStream) { > return nullptr; > } redundant

This carries forward jib's r+

Pushed by pehrsons@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/990a35e2cd48 Don't call NotifyFinished if not activated. r=jib https://hg.mozilla.org/integration/mozilla-inbound/rev/5a38cff4b89f Add guards and sanity checks for Activated vs Stopped state. r=jib

https://hg.mozilla.org/mozilla-central/rev/990a35e2cd48 https://hg.mozilla.org/mozilla-central/rev/5a38cff4b89f