493109 - "ASSERTION: Caller forgot to check ReadyToExecuteScripts()"

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jesse Ruderman]

I have no testcase, only a stack.

###!!! ASSERTION: Caller forgot to check ReadyToExecuteScripts(): 'ReadyToExecuteScripts() && nsContentUtils::IsSafeToRunScript()', file /Users/jruderman/central/content/base/src/nsScriptLoader.cpp, line 581
nsScriptLoader::ProcessRequest(nsScriptLoadRequest*) (nsUnicharUtils.cpp:)
nsScriptLoader::ProcessPendingRequests() (nsUnicharUtils.cpp:)
nsScriptLoader::OnStreamComplete(nsIStreamLoader*, nsISupports*, unsigned int, unsigned int, unsigned char const*) (nsUnicharUtils.cpp:)
nsStreamLoader::OnStopRequest(nsIRequest*, nsISupports*, unsigned int) (nsUnicharUtils.cpp:)
nsHttpChannel::DoNotifyListener() (nsUnicharUtils.cpp:)
nsHttpChannel::HandleAsyncNotifyListener() (nsUnicharUtils.cpp:)
nsRunnableMethod<nsHttpChannel, void>::Run() (nsUnicharUtils.cpp:)
nsThread::ProcessNextEvent(int, int*) (pldhash.c:)
NS_ProcessNextEvent_P(nsIThread*, int) (pldhash.c:)
nsThread::Shutdown() (pldhash.c:)
nsOggDecodeStateMachine::Shutdown() (nsUnicharUtils.cpp:)
nsOggDecoder::Stop() (nsUnicharUtils.cpp:)
nsOggDecoder::Shutdown() (nsUnicharUtils.cpp:)
nsHTMLMediaElement::~nsHTMLMediaElement() (nsUnicharUtils.cpp:)
nsHTMLAudioElement::~nsHTMLAudioElement() (nsUnicharUtils.cpp:)
nsNodeUtils::LastRelease(nsINode*) (nsUnicharUtils.cpp:)
nsGenericElement::Release() (nsUnicharUtils.cpp:)
nsHTMLMediaElement::Release() (nsUnicharUtils.cpp:)
nsHTMLAudioElement::Release() (nsUnicharUtils.cpp:)
nsAttrAndChildArray::Clear() (nsUnicharUtils.cpp:)
nsAttrAndChildArray::~nsAttrAndChildArray() (nsUnicharUtils.cpp:)
nsAttrAndChildArray::~nsAttrAndChildArray() (nsUnicharUtils.cpp:)
nsGenericElement::~nsGenericElement() (nsUnicharUtils.cpp:)
nsStyledElement::~nsStyledElement() (nsUnicharUtils.cpp:)
nsMappedAttributeElement::~nsMappedAttributeElement() (nsUnicharUtils.cpp:)
nsGenericHTMLElement::~nsGenericHTMLElement() (nsUnicharUtils.cpp:)
nsHTMLAnchorElement::~nsHTMLAnchorElement() (nsUnicharUtils.cpp:)
nsNodeUtils::LastRelease(nsINode*) (nsUnicharUtils.cpp:)
nsGenericElement::Release() (nsUnicharUtils.cpp:)
nsHTMLAnchorElement::Release() (nsUnicharUtils.cpp:)
void DoDeferredRelease<nsISupports*>(nsTArray<nsISupports*>&) (/Users/jruderman/central/obj-ff/js/src/xpconnect/src/dom_quickstubs.h:)
XPCJSRuntime::GCCallback(JSContext*, JSGCStatus) (/Users/jruderman/central/obj-ff/js/src/xpconnect/src/dom_quickstubs.h:)
DOMGCCallback(JSContext*, JSGCStatus) (nsUnicharUtils.cpp:)
js_GC (/Users/jruderman/central/obj-ff/js/src/jsautokw.h:)
JS_GC (/Users/jruderman/central/obj-ff/js/src/jsautokw.h:)
nsXREDirProvider::DoShutdown() (nsProfileLock.cpp:)
ScopedXPCOMStartup::~ScopedXPCOMStartup() (nsProfileLock.cpp:)
ScopedXPCOMStartup::~ScopedXPCOMStartup() (nsProfileLock.cpp:)
XRE_main (nsProfileLock.cpp:)
main (/Users/jruderman/central/browser/app/nsBrowserApp.cpp:)
_start (/Users/jruderman/central/browser/app/nsBrowserApp.cpp:)
start (/Users/jruderman/central/browser/app/nsBrowserApp.cpp:)

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This comment is a lie:

    // nsOggDecodeStateMachine::Shutdown is called at a safe
    // time to spin the event loop. This makes the following call
    // also safe.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Shut down the mStepDecodeThread via the DECODER_STATE_SHUTDOWN phase of nsOggDecodeStateMachine::Run instead of doing it synchronously. This is a safe place to do it and it keeps the control of that thread in one place.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This should actually block IMHO since it's a pretty huge violation of Gecko invariants around when events and scripts can run.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/4b86cbae2846

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I backed this out
http://hg.mozilla.org/mozilla-central/rev/4c5b7e7b53a1

because it seemed to be causing crashes on Linux tests, e.g.
http://tinderbox.mozilla.org/showlog.cgi?tree=Firefox&errorparser=unittest&logfile=1242721419.1242727127.14325.gz&buildtime=1242721419&buildname=Linux%20mozilla-central%20unit%20test&fulltext=1#err1

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v2

OK, this patch does a few things:

-- Make control of the step decode thread clearer, mainly by splitting mDecodingComplete into mDecodingComplete (only set to true when the step decode event finishes) and mExitStepDecodeThread (only set to true by the decoder thread). I also factored out the logic to stop the step decode thread into StopStepDecodeThread and call it in the SHUTDOWN state, like the previous patch did.

-- Simplify/clean up the shutdown logic for media decoders in general. Now that we don't reuse a decoder for more than one stream, we can improve the code. In particular there is no need to distinguish "Stop" from "Shutdown", so I've made the places that were calling Stop call Shutdown instead and inlined the old Stop into Shutdown. This means mStopping can go away since we already have mShutdown. To shut down the decoder thread and associated resources, we queue an event that runs Stop on the decoder, which shuts down the decoder thread.

-- Also since we don't need to reuse the decoder for another stream, we don't need to null out mReader or other members synchronously during Shutdown(). We now null them out asynchronously after the decoder thread has stopped. This is the actual fix for this crash, because we can now guarantee that mReader is non-null for the lifetime of the decoder thread (which includes the lifetime of the step decode thread).

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v3

Ooops, need to fold patches together properly.

Comment 8

[cajbir (:cajbir)]

Comment on attachment 378518 [details] [diff] [review]
fix v3

+  nsCOMPtr<nsIRunnable> event = new nsDestroyStateMachine(this);

Can this be an NS_RUNNABLE_METHOD and avoid the need for a new class?

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/2ed480ad1180

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/10eba183de05