Closed Bug 493109 Opened 10 years ago Closed 10 years ago

"ASSERTION: Caller forgot to check ReadyToExecuteScripts()"

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: roc)

Details

(Keywords: assertion, fixed1.9.1)

Attachments

(1 file, 2 obsolete files)

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:)
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.
Assignee: nobody → roc
Flags: wanted1.9.1+
Attached patch fix (obsolete) — Splinter Review
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.
Attachment #378039 - Flags: review?(chris.double)
Whiteboard: [needs review]
Attachment #378039 - Flags: review?(chris.double) → review+
This should actually block IMHO since it's a pretty huge violation of Gecko invariants around when events and scripts can run.
Flags: wanted1.9.1+ → blocking1.9.1+
Whiteboard: [needs review] → [needs landing]
http://hg.mozilla.org/mozilla-central/rev/4b86cbae2846
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 landing]
Attached patch fix v2 (obsolete) — Splinter Review
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).
Attachment #378039 - Attachment is obsolete: true
Attachment #378512 - Flags: review?(chris.double)
Whiteboard: [needs review]
Attached patch fix v3Splinter Review
Ooops, need to fold patches together properly.
Attachment #378512 - Attachment is obsolete: true
Attachment #378518 - Flags: review?(chris.double)
Attachment #378512 - Flags: review?(chris.double)
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?
Attachment #378518 - Flags: review?(chris.double) → review+
http://hg.mozilla.org/mozilla-central/rev/2ed480ad1180
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Whiteboard: [needs review] → [needs 191 landing]
You need to log in before you can comment on or make changes to this bug.