1399922 - Assertion failure: IsIdle(oldState) with testcase that exhausts the stack while adopting

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Testcase is attachment 8901223 [details] though I can't reproduce with that one, unfortunately.  The assertion stack, per bug 1393806 comment 1, is:

Assertion failure: IsIdle(oldState), at /home/worker/workspace/build/src/xpcom/ds/PLDHashTable.h:132
#01: PLDHashTable::RemoveEntry at xpcom/ds/PLDHashTable.cpp:644
#02: nsDOMAttributeMap::DropAttribute at dom/base/nsDOMAttributeMap.cpp:125
#03: mozilla::dom::Element::UnsetAttr at dom/base/Element.cpp:2877
#04: nsDOMAttributeMap::BlastSubtreeToPieces at dom/base/nsDocument.cpp:7754
#05: nsDOMAttributeMap::BlastSubtreeToPieces at dom/base/nsDocument.cpp:7762
#06: nsDOMAttributeMap::BlastSubtreeToPieces at dom/base/nsDocument.cpp:7762
#07: nsIDocument::AdoptNode at dom/base/nsDocument.cpp:7923
#08: mozilla::dom::DocumentBinding::adoptNode at s3:gecko-generated-sources:a746e4a05cf930d0a6e4dc0a4cde5ff89df2f552cf08c7024cc5a4c11cadc3e8aaaa0fcf690761bf828e83f5360beb173f542c5323fb238ace3a8bc4ff2547c9/dom/bindings/DocumentBinding.cpp::1424
#09: mozilla::dom::GenericBindingMethod at dom/bindings/BindingUtils.cpp:3055
#10: ??? (???:???)

Comment 1

[Boris Zbarsky [:bzbarsky]]

So in general, this indicates reentry into a hashtable, or is supposed to.  In particular, a write happening during either a read or a write.

The given stack certainly does not show such reentry, though it's possible there's more on the stack there past that jit stackframe, I guess...

I would really love a way to reproduce this.  Jason, what OS are you on?  If you're on Linux, do you think you could catch this in rr with a recent enough rr that it has portable traces?

Comment 2

[Jason Kratzer [:jkratzer]]

This was tested on Ubuntu 16.04 (4.4.0-78-generic x86_64).  Below is an asan-debug stacktrace.  If you still need more info, let me know and I'll capture this in rr.

Assertion failure: IsIdle(oldState), at /home/worker/workspace/build/src/xpcom/ds/PLDHashTable.h:132
#01: PLDHashTable::Add(void const*, mozilla::fallible_t const&) (/home/worker/workspace/build/src/xpcom/ds/PLDHashTable.cpp:546)
#02: nsBaseHashtable<nsCStringHashKey, nsAutoPtr<mozilla::media::OriginKeyStore::OriginKey>, mozilla::media::OriginKeyStore::OriginKey*>::Put(nsACString const&, mozilla::media::OriginKeyStore::OriginKey* const&, mozilla::fallible_t const&) (/home/worker/workspace/build/src/obj-firefox/dist/include/nsBaseHashtable.h:147)
#03: nsBaseHashtable<nsCStringHashKey, nsAutoPtr<mozilla::media::OriginKeyStore::OriginKey>, mozilla::media::OriginKeyStore::OriginKey*>::Put(nsACString const&, mozilla::media::OriginKeyStore::OriginKey* const&) (/home/worker/workspace/build/src/obj-firefox/dist/include/nsBaseHashtable.h:138)
#04: mozilla::media::OriginKeyStore::OriginKeysTable::GetPrincipalKey(mozilla::ipc::PrincipalInfo const&, nsCString&, bool) (/home/worker/workspace/build/src/dom/media/systemservices/MediaParent.cpp:80 (discriminator 3))
#05: mozilla::media::OriginKeyStore::OriginKeysLoader::GetPrincipalKey(mozilla::ipc::PrincipalInfo const&, nsCString&, bool) (/home/worker/workspace/build/src/dom/media/systemservices/MediaParent.cpp:176 (discriminator 1))
#06: operator() (/home/worker/workspace/build/src/dom/media/systemservices/MediaParent.cpp:470 (discriminator 1))
#07: nsThreadPool::Run() (/home/worker/workspace/build/src/xpcom/threads/nsThreadPool.cpp:227)
#08: non-virtual thunk to nsThreadPool::Run() (/home/worker/workspace/build/src/xpcom/threads/nsThreadPool.cpp:154)
#09: nsThread::ProcessNextEvent(bool, bool*) (/home/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1580)
#10: NS_ProcessNextEvent(nsIThread*, bool) (/home/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:530 (discriminator 3))
#11: mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) (/home/worker/workspace/build/src/ipc/glue/MessagePump.cpp:339 (discriminator 1))
#12: MessageLoop::RunInternal() (/home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:322)
#13: MessageLoop::Run() (/home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:295)
#14: nsThread::ThreadFunc(void*) (/home/worker/workspace/build/src/xpcom/threads/nsThread.cpp:509)
#15: _pt_root (/home/worker/workspace/build/src/nsprpub/pr/src/pthreads/ptthread.c:219)
#16: start_thread (/build/glibc-bfm8X4/glibc-2.23/nptl/pthread_create.c:333)
#17: __clone (/build/glibc-bfm8X4/glibc-2.23/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:111)
#18: ??? (???:???)
ASAN:DEADLYSIGNAL
=================================================================
==12031==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f82ae21d512 bp 0x7f828adfc6d0 sp 0x7f828adfc6c0 T47)
==12031==The signal is caused by a WRITE memory access.
==12031==Hint: address points to the zero page.
[12031] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file /home/worker/workspace/build/src/dom/fetch/FetchDriver.cpp, line 389
    #0 0x7f82ae21d511 in Checker::StartWriteOp() /home/worker/workspace/build/src/xpcom/ds/PLDHashTable.h:130:5
    #1 0x7f82ae204967 in PLDHashTable::Add(void const*, mozilla::fallible_t const&) /home/worker/workspace/build/src/xpcom/ds/PLDHashTable.cpp:542:15
    #2 0x7f82b2a88274 in nsBaseHashtable<nsCStringHashKey, nsAutoPtr<mozilla::media::OriginKeyStore::OriginKey>, mozilla::media::OriginKeyStore::OriginKey*>::Put(nsACString const&, mozilla::media::OriginKeyStore::OriginKey* const&, mozilla::fallible_t const&) /home/worker/workspace/build/src/obj-firefox/dist/include/nsBaseHashtable.h:146:28
    #3 0x7f82b2a881d5 in nsBaseHashtable<nsCStringHashKey, nsAutoPtr<mozilla::media::OriginKeyStore::OriginKey>, mozilla::media::OriginKeyStore::OriginKey*>::Put(nsACString const&, mozilla::media::OriginKeyStore::OriginKey* const&) /home/worker/workspace/build/src/obj-firefox/dist/include/nsBaseHashtable.h:138:10
    #4 0x7f82b2a86de5 in mozilla::media::OriginKeyStore::OriginKeysTable::GetPrincipalKey(mozilla::ipc::PrincipalInfo const&, nsCString&, bool) /home/worker/workspace/build/src/dom/media/systemservices/MediaParent.cpp:79:15
    #5 0x7f82b2a86fa3 in mozilla::media::OriginKeyStore::OriginKeysLoader::GetPrincipalKey(mozilla::ipc::PrincipalInfo const&, nsCString&, bool) /home/worker/workspace/build/src/dom/media/systemservices/MediaParent.cpp:176:38
    #6 0x7f82b2a8b36c in mozilla::media::Parent<mozilla::media::NonE10s>::RecvGetPrincipalKey(unsigned int const&, mozilla::ipc::PrincipalInfo const&, bool const&)::{lambda()#1}::operator()() const /home/worker/workspace/build/src/dom/media/systemservices/MediaParent.cpp:470:41
    #7 0x7f82ae30f1ae in nsThreadPool::Run() /home/worker/workspace/build/src/xpcom/threads/nsThreadPool.cpp:225:14
    #8 0x7f82ae30f62c in non-virtual thunk to nsThreadPool::Run() /home/worker/workspace/build/src/xpcom/threads/nsThreadPool.cpp:154:15
    #9 0x7f82ae306a1c in nsThread::ProcessNextEvent(bool, bool*) /home/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1579:14
    #10 0x7f82ae30c780 in NS_ProcessNextEvent(nsIThread*, bool) /home/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:530:10
    #11 0x7f82aee78a54 in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) /home/worker/workspace/build/src/ipc/glue/MessagePump.cpp:339:20
    #12 0x7f82aedc3a87 in MessageLoop::RunInternal() /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:321:10
    #13 0x7f82aedc3919 in MessageLoop::Run() /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:294:3
    #14 0x7f82ae2fdacb in nsThread::ThreadFunc(void*) /home/worker/workspace/build/src/xpcom/threads/nsThread.cpp:507:11
    #15 0x7f82c956c5ad in _pt_root /home/worker/workspace/build/src/nsprpub/pr/src/pthreads/ptthread.c:216:5
    #16 0x7f82ccb756b9 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76b9)
    #17 0x7f82cbbfe3dc in clone /build/glibc-bfm8X4/glibc-2.23/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:109

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /home/worker/workspace/build/src/xpcom/ds/PLDHashTable.h:130:5 in Checker::StartWriteOp()
Thread T47 (StreamTrans #2) created by T0 here:
    #0 0x4a4d96 in __interceptor_pthread_create /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_interceptors.cc:245:3
    #1 0x7f82c9568d72 in _PR_CreateThread /home/worker/workspace/build/src/nsprpub/pr/src/pthreads/ptthread.c:457:14
    #2 0x7f82c956892e in PR_CreateThread /home/worker/workspace/build/src/nsprpub/pr/src/pthreads/ptthread.c:548:12
    #3 0x7f82ae2ffce4 in nsThread::Init(nsACString const&) /home/worker/workspace/build/src/xpcom/threads/nsThread.cpp:689:8
    #4 0x7f82ae30baf0 in nsThreadManager::NewNamedThread(nsACString const&, unsigned int, nsIThread**) /home/worker/workspace/build/src/xpcom/threads/nsThreadManager.cpp:275:22
    #5 0x7f82ae30e609 in NS_NewNamedThread(nsACString const&, nsIThread**, nsIRunnable*, unsigned int) /home/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:154:45
    #6 0x7f82ae30e255 in nsThreadPool::PutEvent(already_AddRefed<nsIRunnable>, unsigned int) /home/worker/workspace/build/src/xpcom/threads/nsThreadPool.cpp:107:17
    #7 0x7f82ae30f92b in nsThreadPool::Dispatch(already_AddRefed<nsIRunnable>, unsigned int) /home/worker/workspace/build/src/xpcom/threads/nsThreadPool.cpp:274:5
    #8 0x7f82ae54b70f in mozilla::net::nsStreamTransportService::Dispatch(already_AddRefed<nsIRunnable>, unsigned int) /home/worker/workspace/build/src/netwerk/base/nsStreamTransportService.cpp:490:18
    #9 0x7f82b2a67ced in mozilla::media::Parent<mozilla::media::NonE10s>::RecvGetPrincipalKey(unsigned int const&, mozilla::ipc::PrincipalInfo const&, bool const&) /home/worker/workspace/build/src/dom/media/systemservices/MediaParent.cpp:460:13
    #10 0x7f82b2a6633f in mozilla::media::GetPrincipalKey(mozilla::ipc::PrincipalInfo const&, bool) /home/worker/workspace/build/src/dom/media/systemservices/MediaChild.cpp:32:30
    #11 0x7f82b266b840 in mozilla::MediaManager::EnumerateDevicesImpl(unsigned long, mozilla::dom::MediaSourceEnum, mozilla::dom::MediaSourceEnum, bool) /home/worker/workspace/build/src/dom/media/MediaManager.cpp:2649:33
    #12 0x7f82b2669198 in mozilla::MediaManager::GetUserMedia(nsPIDOMWindowInner*, mozilla::dom::MediaStreamConstraints const&, nsIDOMGetUserMediaSuccessCallback*, nsIDOMGetUserMediaErrorCallback*, mozilla::dom::CallerType) /home/worker/workspace/build/src/dom/media/MediaManager.cpp:2416:31
    #13 0x7f82b058a072 in mozilla::dom::Navigator::MozGetUserMedia(mozilla::dom::MediaStreamConstraints const&, mozilla::dom::NavigatorUserMediaSuccessCallback&, mozilla::dom::NavigatorUserMediaErrorCallback&, mozilla::dom::CallerType, mozilla::ErrorResult&) /home/worker/workspace/build/src/dom/base/Navigator.cpp:1305:18
    #14 0x7f82b0c49583 in mozilla::dom::NavigatorBinding::mozGetUserMedia(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Navigator*, JSJitMethodCallArgs const&) /home/worker/workspace/build/src/obj-firefox/dom/bindings/NavigatorBinding.cpp:1571:9
    #15 0x7f82b1e4ef4e in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /home/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:3060:13
    #16 0x7f82b6d7ab11 in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) /home/worker/workspace/build/src/js/src/jscntxtinlines.h:293:15
    #17 0x7f82b6d7a6bd in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:470:16
    #18 0x7f82b6d7b555 in InternalCall(JSContext*, js::AnyInvokeArgs const&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:515:12
    #19 0x7f82b6d6ffaa in Interpret(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3066:18
    #20 0x7f82b6d5bc48 in js::RunScript(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:410:12
    #21 0x7f82b6d7a640 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:488:15
    #22 0x7f82b6d7b555 in InternalCall(JSContext*, js::AnyInvokeArgs const&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:515:12
    #23 0x7f82b6d7b76c in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:534:10
    #24 0x7f82b75d75cb in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/jsapi.cpp:2948:12
    #25 0x7f82b1943109 in mozilla::dom::EventListener::HandleEvent(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, mozilla::ErrorResult&) /home/worker/workspace/build/src/obj-firefox/dom/bindings/EventListenerBinding.cpp:47:8
    #26 0x7f82b2168a81 in void mozilla::dom::EventListener::HandleEvent<mozilla::dom::EventTarget*>(mozilla::dom::EventTarget* const&, mozilla::dom::Event&, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JSCompartment*) /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/EventListenerBinding.h:65:12
    #27 0x7f82b2168606 in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) /home/worker/workspace/build/src/dom/events/EventListenerManager.cpp:1138:9
    #28 0x7f82b216995d in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) /home/worker/workspace/build/src/dom/events/EventListenerManager.cpp:1311:20
    #29 0x7f82b215552b in mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) /home/worker/workspace/build/src/dom/events/EventDispatcher.cpp:315:17
    #30 0x7f82b2154d3f in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) /home/worker/workspace/build/src/dom/events/EventDispatcher.cpp:488:14
    #31 0x7f82b21566cd in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) /home/worker/workspace/build/src/dom/events/EventDispatcher.cpp:824:9
    #32 0x7f82b2137ae0 in mozilla::EventDispatcher::DispatchDOMEvent(nsISupports*, mozilla::WidgetEvent*, nsIDOMEvent*, nsPresContext*, nsEventStatus*) /home/worker/workspace/build/src/dom/events/EventDispatcher.cpp:890:12
    #33 0x7f82b077394e in nsINode::DispatchEvent(nsIDOMEvent*, bool*) /home/worker/workspace/build/src/dom/base/nsINode.cpp:1345:5
    #34 0x7f82b03a3a04 in nsContentUtils::DispatchEvent(nsIDocument*, nsISupports*, nsAString const&, bool, bool, bool, bool*, bool) /home/worker/workspace/build/src/dom/base/nsContentUtils.cpp:4466:18
    #35 0x7f82b03a37ab in nsContentUtils::DispatchTrustedEvent(nsIDocument*, nsISupports*, nsAString const&, bool, bool, bool*) /home/worker/workspace/build/src/dom/base/nsContentUtils.cpp:4434:10
    #36 0x7f82b06bf9d3 in nsDocument::DispatchContentLoadedEvents() /home/worker/workspace/build/src/dom/base/nsDocument.cpp:5283:3
    #37 0x7f82b0736ff5 in mozilla::detail::RunnableMethodImpl<nsDocument*, void (nsDocument::*)(), true, (mozilla::RunnableKind)0>::Run() /home/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:1207:13
    #38 0x7f82ae306a1c in nsThread::ProcessNextEvent(bool, bool*) /home/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1579:14
    #39 0x7f82ae30c780 in NS_ProcessNextEvent(nsIThread*, bool) /home/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:530:10
    #40 0x7f82aee77915 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /home/worker/workspace/build/src/ipc/glue/MessagePump.cpp:97:21
    #41 0x7f82aedc3a87 in MessageLoop::RunInternal() /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:321:10
    #42 0x7f82aedc3919 in MessageLoop::Run() /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:294:3
    #43 0x7f82b36671fa in nsBaseAppShell::Run() /home/worker/workspace/build/src/widget/nsBaseAppShell.cpp:156:27
    #44 0x7f82b67fd8f1 in nsAppStartup::Run() /home/worker/workspace/build/src/toolkit/components/startup/nsAppStartup.cpp:287:30
    #45 0x7f82b695ca72 in XREMain::XRE_mainRun() /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4596:22
    #46 0x7f82b695e6cf in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4779:8
    #47 0x7f82b695f5b8 in XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4874:21
    #48 0x4ecaf8 in do_main(int, char**, char**) /home/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:236:22
    #49 0x4ec410 in main /home/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:309:16
    #50 0x7f82cbb1782f in __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291

Comment 3

[Jason Kratzer [:jkratzer]]

Attachment: prefs-default.js

Also, these are the prefs I'm currently using.

Comment 4

[Boris Zbarsky [:bzbarsky]]

That stack makes a heck of a lot more sense than the one in comment 0!

And looking at the relevant code, I simply don't understand how it's supposed to make sense.  In MediaParent.cpp we have Parent<Super>::RecvGetPrincipalKey which dispatches a runnable to run on the stream transport service threadpool.  This runnable calls OriginKeysTable::GetPrincipalKey on either mOriginKeyStore->mPrivateBrowsingOriginKeys or mOriginKeyStore->mOriginKeys, depending.  Now mOriginKeyStore is actually the same thing as sOriginKeyStore; there's only one of them.  And GetPrincipalKey does lazy addition to those hashtables, like so:

  mKeys.Put(principalString, key);

I see nothing preventing multiple runnables being queued up by RecvGetPrincipalKey.  I also see nothing preventing them from running in parallel, on different threads in the STS threadpool.  Hence I see nothing preventing racing writes to these hashtables...  What am I missing?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Let's try needinfoing some people who know about this code.

Comment 6

[Boris Zbarsky [:bzbarsky]]

This code seems to date back to https://hg.mozilla.org/mozilla-central/rev/76f9de2a6a4fd5d60f3f2672bde9231a6383fcd4 as far as I can tell.  Before that we ran the code on whatever thread the RecvGetPrincipalKey happened on, which was presumably always the same thread, so it couldn't race...

Comment 7

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

(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #4)
> I see nothing preventing multiple runnables being queued up by
> RecvGetPrincipalKey.  I also see nothing preventing them from running in
> parallel, on different threads in the STS threadpool.  Hence I see nothing
> preventing racing writes to these hashtables...  What am I missing?

'STS' is a bad TLA... it has bit us before.  In webrtc, STS is *always* SocketTransportService.  This thread in the ASAN report is StreamTrans#1 -- the StreamTransportService's threadpool.  (SocketTransportService is *not* a pool, and would horribly break webrtc if our stuff ran in pools (without some larger grouping construct like nsTaskQueue's)).

MediaParent dispatches to StreamTransportService, which is a pool.  If the runnables in RecvGetPrincipleKey() and called from it access any shared state, they need locks or atomics (or TaskQueues), etc.

NI to jib for MediaParent/etc

I kinda-strongly suspect that it should be using *Socket*TransportService, not StreamTransportService - we've had this bug before.  And it was made StreamTransportService in bug 1046245 (by jib)

Comment 8

[Tyson Smith [:tsmith]]

Attachment: test_case.html

Comment 9

[Boris Zbarsky [:bzbarsky]]

Why is this P2?  This seems like it would lead to memory corruption: we're modifying a hashtable in a race with reads or writes on it...

Comment 10

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

This is rank 8 and should be a P1-fix-ASAP bug per our rank->priority mapping (and I made it P1 on purpose initially)

Comment 11

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Bug 1399922 - Use Socket (not Stream) Transport Service thread for getting deviceId keys in MediaParent.

Review commit: https://reviewboard.mozilla.org/r/182014/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/182014/

Comment 12

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

Comment on attachment 8910570 [details]
Bug 1399922 - Use Socket (not Stream) Transport Service thread for getting deviceId keys in MediaParent.

https://reviewboard.mozilla.org/r/182014/#review187412

Comment 13

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

Comment on attachment 8910570 [details]
Bug 1399922 - Use Socket (not Stream) Transport Service thread for getting deviceId keys in MediaParent.

https://reviewboard.mozilla.org/r/182014/#review187500

We were using the wrong STS, this patch corrects that -- however, we shouldn't be writing files on SocketTransportService either - blocking SocketTransportService isn't good.  Typically that's done on the GeckoIO thread, I believe

Comment 14

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

So, after checking -- StreamTransportService (poorly named?) is where we (mostly) write files from to keep it off MainThread.  See Preferences.cpp and the PWRunnable class.  However, as bz points out, this is a ThreadPool, so multiple can execute at once.  The choices are to either use a singleton mutex to lock the access to the hashtable (at least), or to dispatch them through a TaskQueue (xpcom/threads/TaskQueue.h) which will guarantee in-order execution and non-concurrency.  For this case, I don't know ordering is required -- if it isn't, then a lock on hashtable access (including anywhere else the hashtable is touched! or around the entire call from the runnable) may well be fine.

Looking at the code, there are a number of problematic calls that could clash across concurrent access (SetProfileDir()/Load, etc), so I think if you lock, it should be right at the start of the two lambdas, so it's held for the entire run of each runnable.  I don't think these runnables care about preserving ordering much, except I suppose for Sanitize should mean subsequent gets don't get the old data; theoretically if a Sanitize and a Read are racing, it's wrong for the read to jump ahead of the sanitize.  Mostly that's theory though, because if they're racing here, they could have been submitted in the other order anyways (i.e. they just race elsewhere).

To be canonically correct for Sanitize we'd use a TaskQueue, and perhaps we simply should do that, but they require a bit more shutdown-handling code (BeginShutdown()/AwaitShutdownAndIdle(), perhaps from ~Parent()?).  However, for an upliftable patch I'd find a mutex lock around the entirety of the lambda bodies is very easy to reason about.

Comment 15

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Bug 1399922 - Use a static mutex for getting deviceId keys in MediaParent.

Review commit: https://reviewboard.mozilla.org/r/182252/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/182252/

Comment 16

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Ok that makes sense. I agree ordering rarely ever matters in practice here. Reverting to StreamTransportService and adding a mutex.

I opted for locking inside the first (sts) runnable only, since that seems sufficient to prevent competition in the pool (locking ahead of dispatching would only work for sync-runnables), provided I change it to access sOriginKeyStore directly under lock, which I've done. Once we have the result, there's very little happening on the way out (second runnable).

Comment 17

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

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #16)
> Ok that makes sense. I agree ordering rarely ever matters in practice here.
> Reverting to StreamTransportService and adding a mutex.
> 
> I opted for locking inside the first (sts) runnable only, since that seems
> sufficient to prevent competition in the pool (locking ahead of dispatching
> would only work for sync-runnables), provided I change it to access
> sOriginKeyStore directly under lock, which I've done. Once we have the
> result, there's very little happening on the way out (second runnable).

You must lock in the RecvSanitizeOriginKeys() lambda as well - it touches the keystore; you can't have it racing with the RecvGetPrincipalKey lamdba

Comment 18

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

Comment on attachment 8910788 [details]
Bug 1399922 - Use a static mutex for getting deviceId keys in MediaParent.

https://reviewboard.mozilla.org/r/182252/#review187690

mOriginKeyStore should be unused now, right?  If so please remove it and r+

::: dom/media/systemservices/MediaParent.cpp:528
(Diff revision 1)
>    nsresult rv = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR,
>                                           getter_AddRefs(profileDir));
>    if (NS_WARN_IF(NS_FAILED(rv))) {
>      return IPCResult(this, false);
>    }
>    // Over to stream-transport thread to do the file io.

Comment that StreamTransportService is a threadpool

Comment 19

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

(In reply to Randell Jesup [:jesup] from comment #18)
> Comment on attachment 8910788 [details]
> Bug 1399922 - Use a static mutex for getting deviceId keys in MediaParent.
> 
> https://reviewboard.mozilla.org/r/182252/#review187690
> 
> mOriginKeyStore should be unused now, right?  If so please remove it and r+

You'll still need to call OriginKeyStore::Get() in Parent()

Comment 20

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8910788 [details]
Bug 1399922 - Use a static mutex for getting deviceId keys in MediaParent.

https://reviewboard.mozilla.org/r/182252/#review187690

No, mOriginKeyStore's are is what collectively keep sOriginKeyStore alive, by ref-count.

Comment 21

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8910788 [details]
Bug 1399922 - Use a static mutex for getting deviceId keys in MediaParent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/182252/diff/1-2/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b030607a3ddb
Use a static mutex for getting deviceId keys in MediaParent. r=jesup

Comment 23

[Phil Ringnalda (:philor)]

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

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Too late for 56 but maybe we want to take a fix in 57. 
Jib, what do you think? THanks

Comment 25

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Jason, could you verify that this is fixed in Nightly?

Comment 26

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

Restricting since this is a minor security issue - the risk of misbehavior is mitigated by the fact that you need to get multiple tabs to coordinate (tightly), and you only get one shot at it, so you can't repeat the attempt, and then if you hit it it's very unsure that you can actually make something bad enough to be a problem happen.  Given that, sec-moderate.

Comment 27

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8910788 [details]
Bug 1399922 - Use a static mutex for getting deviceId keys in MediaParent.

Approval Request Comment for 57
[Feature/Bug causing the regression]: bug 1046245
[User impact if declined]:

First-time use of getUserMedia() or enumerateDevices() from a new origin from two or more tabs concurrently (doesn't have to be the same origin) may race where two threads write to the same hashtable without locking, which in theory could corrupt state or crash in the master process.

By "first-time" I mean in this run of the browser from sites that haven't ever been granted gUM before, where "ever" means since cookies were cleared.

The chances of this happening by accident seem quite rare, but not zero.

It poses a trick-shot window for non-accidental use, meaning an exploit only gets one attempt per browser start.

[Is this code covered by automated tests?]: Normal (non-concurrent) use of affected code is covered by existing tests.

[Has the fix been verified in Nightly?]:

Nightly appears to work normally, as before, but I was never able to reproduce a symptom from this problem in the first place. Problem was found with asan and code inspection.

[Needs manual test from QE? If yes, steps to reproduce]: 

Don't have steps to reproduce. Replicating work done to find this initially (asan) seem best. See comment 25.

[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: Not very
[Why is the change risky/not risky?]:

Patch adds a lock to guard against concurrent access of this hashtable from the sts thread pool. Any contention should be as rare as the original problem.

[String changes made/needed]: none

Comment 28

[Jason Kratzer [:jkratzer]]

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #25)
> Jason, could you verify that this is fixed in Nightly?

Yes, this appears to have been fixed.

Comment 29

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8910788 [details]
Bug 1399922 - Use a static mutex for getting deviceId keys in MediaParent.

OK, let's take it in 57 then.
Should be in 57b3

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/eea4ead4541f

Comment 31

[Tyson Smith [:tsmith]]

Attachment: test_2.html

I found another test that can trigger the issue. If this is a different issue let me know and I can open a new bug for it.

Comment 32

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Yes please open a new issue. We'll leave this closed as the fix for the stack trace in comment 2, which was uplifted.

There were two at least stacks here (comment 0 vs. comment 2) so it's likely a different problem with the same symptom.

Comment 33

[Tyson Smith [:tsmith]]

Done. Created bug 1403377