885185 - Web Audio uses WMFReader on the main thread with the non-recommended (and possibly dangerous) COM apartment/threading model

Status: RESOLVED WORKSFORME

(Core :: Web Audio, defect)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

This is a spin off from bug 878449. In particular see bug 878449 comment 41, 

Web Audio is calling WMFReader::OnDecodeThreadStart() on the main thread, nd that's calling which is trying to call CoInitializeEx(0, COINIT_MULTITHREADED), but that's failing because the main thread has already been initialized with single threaded apartment COM (by Gecko).

Call stack:
> xul.dll!mozilla::WMFReader::OnDecodeThreadStart()  Line 83	C++
> xul.dll!mozilla::MediaDecodeTask::Decode()  Line 482	C++
> xul.dll!mozilla::MediaDecodeTask::Run()  Line 382	C++
> xul.dll!mozilla::MediaBufferDecoder::SyncDecodeMedia(const char * aContentType, unsigned char * aBuffer, unsigned int aLength, mozilla::WebAudioDecodeJob & aDecodeJob)  Line 769	C++
> xul.dll!mozilla::dom::AudioContext::CreateBuffer(JSContext * aJSContext, mozilla::dom::TypedArray<unsigned char,&JS_GetArrayBufferData,&JS_GetObjectAsArrayBuffer,&JS_NewArrayBuffer> & aBuffer, bool aMixToMono, mozilla::ErrorResult & aRv)  Line 176 + 0x2d bytes	C++
> xul.dll!mozilla::dom::AudioContextBinding::createBuffer(JSContext * cx, JS::Handle<JSObject *> obj, mozilla::dom::AudioContext * self, const JSJitMethodCallArgs & args)  Line 219 + 0x25 bytes	C++
> xul.dll!mozilla::dom::AudioContextBinding::genericMethod(JSContext * cx, unsigned int argc, JS::Value * vp)  Line 911 + 0x26 bytes	C++
> mozjs.dll!js::CallJSNative(JSContext * cx, int (JSContext *, unsigned int, JS::Value *)* native, const JS::CallArgs & args)  Line 349 + 0x19 bytes	C++
> mozjs.dll!js::Invoke(JSContext * cx, JS::CallArgs args, js::MaybeConstruct construct)  Line 388 + 0x16 bytes	C++
> mozjs.dll!js::Interpret(JSContext * cx, js::StackFrame * entryFrame, js::InterpMode interpMode, bool useNewType)  Line 2212 + 0x2a bytes	C++
> mozjs.dll!js::RunScript(JSContext * cx, js::StackFrame * fp)  Line 345 + 0x11 bytes	C++
> mozjs.dll!js::Invoke(JSContext * cx, JS::CallArgs args, js::MaybeConstruct construct)  Line 401 + 0x12 bytes	C++
> mozjs.dll!js::Invoke(JSContext * cx, const JS::Value & thisv, const JS::Value & fval, unsigned int argc, JS::Value * argv, JS::Value * rval)  Line 434 + 0x33 bytes	C++
> mozjs.dll!JS_CallFunctionValue(JSContext * cx, JSObject * objArg, JS::Value fval, unsigned int argc, JS::Value * argv, JS::Value * rval)  Line 5886 + 0x36 bytes	C++
> xul.dll!mozilla::dom::EventHandlerNonNull::Call(JSContext * cx, JS::Handle<JSObject *> aThisObj, nsDOMEvent & event, mozilla::ErrorResult & aRv)  Line 41 + 0x46 bytes	C++
> xul.dll!mozilla::dom::EventHandlerNonNull::Call<nsISupports *>(nsISupports * const & thisObj, nsDOMEvent & event, mozilla::ErrorResult & aRv, mozilla::dom::CallbackObject::ExceptionHandling aExceptionHandling)  Line 58 + 0x2b bytes	C++
> xul.dll!nsJSEventListener::HandleEvent(nsIDOMEvent * aEvent)  Line 248	C++
> xul.dll!nsEventListenerManager::HandleEventSubType(nsListenerStruct * aListenerStruct, const mozilla::dom::CallbackObjectHolder<mozilla::dom::EventListener,nsIDOMEventListener> & aListener, nsIDOMEvent * aDOMEvent, mozilla::dom::EventTarget * aCurrentTarget, nsCxPusher * aPusher)  Line 937 + 0x1d bytes	C++
> xul.dll!nsEventListenerManager::HandleEventInternal(nsPresContext * aPresContext, nsEvent * aEvent, nsIDOMEvent * * aDOMEvent, mozilla::dom::EventTarget * aCurrentTarget, nsEventStatus * aEventStatus, nsCxPusher * aPusher)  Line 1009 + 0x1e bytes	C++
> xul.dll!nsEventListenerManager::HandleEvent(nsPresContext * aPresContext, nsEvent * aEvent, nsIDOMEvent * * aDOMEvent, mozilla::dom::EventTarget * aCurrentTarget, nsEventStatus * aEventStatus, nsCxPusher * aPusher)  Line 329	C++
> xul.dll!nsEventTargetChainItem::HandleEvent(nsEventChainPostVisitor & aVisitor, bool aMayHaveNewListenerManagers, nsCxPusher * aPusher)  Line 204	C++
> xul.dll!nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor & aVisitor, nsDispatchingCallback * aCallback, bool aMayHaveNewListenerManagers, nsCxPusher * aPusher)  Line 331	C++
> xul.dll!nsEventDispatcher::Dispatch(nsISupports * aTarget, nsPresContext * aPresContext, nsEvent * aEvent, nsIDOMEvent * aDOMEvent, nsEventStatus * aEventStatus, nsDispatchingCallback * aCallback, nsCOMArray<mozilla::dom::EventTarget> * aTargets)  Line 635 + 0x19 bytes	C++
> xul.dll!nsEventDispatcher::DispatchDOMEvent(nsISupports * aTarget, nsEvent * aEvent, nsIDOMEvent * aDOMEvent, nsPresContext * aPresContext, nsEventStatus * aEventStatus)  Line 693 + 0x1d bytes	C++
> xul.dll!nsDOMEventTargetHelper::DispatchDOMEvent(nsEvent * aEvent, nsIDOMEvent * aDOMEvent, nsPresContext * aPresContext, nsEventStatus * aEventStatus)  Line 331 + 0x19 bytes	C++
> xul.dll!nsXHREventTarget::DispatchDOMEvent(nsEvent * aEvent, nsIDOMEvent * aDOMEvent, nsPresContext * aPresContext, nsEventStatus * aEventStatus)  Line 71 + 0x1f bytes	C++
> xul.dll!nsXMLHttpRequest::DispatchDOMEvent(nsEvent * aEvent, nsIDOMEvent * aDOMEvent, nsPresContext * aPresContext, nsEventStatus * aEventStatus)  Line 232 + 0x1f bytes	C++
> xul.dll!nsXMLHttpRequest::DispatchProgressEvent(nsDOMEventTargetHelper * aTarget, const nsAString_internal & aType, bool aLengthComputable, unsigned __int64 aLoaded, unsigned __int64 aTotal)  Line 1486	C++
> xul.dll!nsXMLHttpRequest::ChangeStateToDone()  Line 2226	C++
> xul.dll!nsXMLHttpRequest::OnStopRequest(nsIRequest * request, nsISupports * ctxt, tag_nsresult status)  Line 2182	C++
> xul.dll!nsCORSListenerProxy::OnStopRequest(nsIRequest * aRequest, nsISupports * aContext, tag_nsresult aStatusCode)  Line 579 + 0x28 bytes	C++
> xul.dll!nsStreamListenerTee::OnStopRequest(nsIRequest * request, nsISupports * context, tag_nsresult status)  Line 52 + 0x28 bytes	C++
> xul.dll!mozilla::net::nsHttpChannel::OnStopRequest(nsIRequest * request, nsISupports * ctxt, tag_nsresult status)  Line 5070	C++
> xul.dll!nsInputStreamPump::OnStateStop()  Line 556	C++
> xul.dll!nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream * stream)  Line 375 + 0xb bytes	C++
> xul.dll!nsInputStreamReadyEvent::Run()  Line 83	C++
> xul.dll!nsThread::ProcessNextEvent(bool mayWait, bool * result)  Line 626 + 0x19 bytes	C++
> xul.dll!NS_ProcessNextEvent(nsIThread * thread, bool mayWait)  Line 238 + 0x17 bytes	C++
> xul.dll!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate * aDelegate)  Line 82 + 0xe bytes	C++
> xul.dll!MessageLoop::RunInternal()  Line 220	C++
> xul.dll!MessageLoop::RunHandler()  Line 213	C++
> xul.dll!MessageLoop::Run()  Line 187	C++
> xul.dll!nsBaseAppShell::Run()  Line 165	C++
> xul.dll!nsAppShell::Run()  Line 113 + 0x9 bytes	C++
> xul.dll!nsAppStartup::Run()  Line 269 + 0x1c bytes	C++
> xul.dll!XREMain::XRE_mainRun()  Line 3856 + 0x25 bytes	C++

Windows Media Foundation's documentation says that some of WMF's innards relies on COM being initialized in Multi Threaded Apartment mode:
http://msdn.microsoft.com/en-us/library/windows/desktop/ee892371%28v=vs.85%29.aspx

I'm really not sure how much of a problem this is. bbondy reported in bug 878449 that Web Audio still works if we use WMF on the main thread when the main thread has been initialized in COM STA mode.

Someone needs to figure out if using WMF on the main thread in STA mode is a problem, and if so, we need to devise a way to ensure WMF is only used on a non-main thread in MTA mode.

(Note, the fix for bug 878449 should be landed/applied before debugging this, else you're liable to get crashes due to mismatched CoInitialize and CoUninitialize calls.)

Comment 1

[(no longer active)]

Chris, what are you suggesting that we should do here?

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Someone needs to figure out if using WMF from SyncDecodeMedia() on the main thread (which defaults to in STA COM mode) is a problem, and if so, we need to devise a way to ensure WMF is only used on a non-main thread in MTA mode. Or we need to switch the main thread to MTA mode.

Comment 3

[Brian R. Bondy [:bbondy]]

I think we should try to not use the main thread at all since MSDN says not to use it with STA. Or as you mentioned change the main thread to MTA but that may have unknown effects and slowdowns due to uneeded locking on the main thread.

Comment 4

[Paul Adenot (:padenot)]

Note that we are trying to get the method that forces us to use the main thread removed from the spec, because, well, this is a terrible API. Not sure we will succeed, though.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

We could not implement (i.e. remove) SyncDecodeMedia() in our implementation anyway.

If we end up keeping SyncDecodeMedia, we could have it create a thread and do a sync dispatch to run SyncDecodeMedia, then other runnables can run on the main thread while it waits for the sync dispatch/event to run to complete (I'm not sure if we pump the win32 message queue in this case though).

Comment 6

[(no longer active)]

(In reply to comment #5)
> We could not implement (i.e. remove) SyncDecodeMedia() in our implementation
> anyway.
> 
> If we end up keeping SyncDecodeMedia, we could have it create a thread and do a
> sync dispatch to run SyncDecodeMedia, then other runnables can run on the main
> thread while it waits for the sync dispatch/event to run to complete (I'm not
> sure if we pump the win32 message queue in this case though).

In that case we can just decode on a worker thread and block on it from the main thread (for example, by waiting on a monitor or something.)

But like Paul mentioned, let's see if we can remove the sync createBuffer function first.  We should know in a few days.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Roc tells me that SyncDecodeMedia() has been removed from the spec... Is being removed from our implementation in bug 889016, or somewhere else?

Comment 8

[Paul Adenot (:padenot)]

The only javascript function that was using this SyncDecodeMedia path has been removed from the spec a week or so ago. The function has not been removed from the code yet, but it now throws unless a pref is set ("media.webaudio.legacy.AudioContext").

I believe we should be able to remove this method and all the C++ code underneath, now. Ehsan, are we waiting for something to happen, here?

Comment 9

[(no longer active)]

I've decided to keep all of these legacy stuff hidden behind prefs so that we can use the prefs for testing Web Audio samples using them.

Let's not remove the code for now...  There is no way that regular content can trigger it unless somebody turns on the pref manually, which presumably means they know what they are doing...

Comment 10

[(no longer active)]

The API that exposed this is hidden behind a pref now, so, WFM.