Closed Bug 468721 Opened 16 years ago Closed 16 years ago

Potential crash in nsOggDecoder::Load()

Categories

(Core :: Audio/Video, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: cpearce, Assigned: cpearce)

References

Details

(Keywords: crash, fixed1.9.1)

Attachments

(1 file, 2 obsolete files)

I've not been able to reproduce this reliably, but I've ran into it a lot while chasing intermittent failures.

In nsOggDecoder::Load():1261 - http://mxr.mozilla.org/mozilla-central/source/content/media/video/src/nsOggDecoder.cpp#1261 - we recreate the reader. mReader is an nsAutoPtr, so this will delete the old nsChannelReader.

Then at nsOggDecoder::Load():1270 - 
http://mxr.mozilla.org/mozilla-central/source/content/media/video/src/nsOggDecoder.cpp#1270 - we recreate the nsOggDecoderStateMachine. This is a ref counted object, and so the old nsOggDecoderStateMachine will die after the corresponding nsChannelReader. The nsOggDecoderStateMachine has a reference to the now destroyed nsChannelReader, and the nsOggDecoderStateMachine outlives the nsChannelReader.

In ~nsOggDecoderStateMachine() we call oggplay_close() - http://mxr.mozilla.org/mozilla-central/source/content/media/video/src/nsOggDecoder.cpp#489 - which calls the reader's destroy() method - http://mxr.mozilla.org/mozilla-central/source/media/liboggplay/src/liboggplay/oggplay.c#590 - which calls Close() on the reader's nsMediaStream - http://mxr.mozilla.org/mozilla-central/source/content/media/video/src/nsChannelReader.cpp#73 . But that nsMediaStream will have been destroyed, as its owning nsChannelReader should have been destroyed.

~nsOggDecoderStateMachine() must not use stale references to dead nsChannelReaders, we should destroy the nsOggDecoderStateMachine before destroying the nsChannelReader.
It looks like this is OK. The only way we can get into Load() in this case is through ChangeState() when mState==ENDED, and state can only be ENDED when Stop() has been called, and Stop() nulls the mDecodeStateMachine reference, so the nsOggDecoderStateMachine is dead and it can't use the nsChannelReader.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → INVALID
Actually this crash can still happen. If we're running a nsOggDecoder::PlaybackEnded() callback on the main thread, which calls through to nsOggDecoder::Stop(), it will transition to ENDED state and then call Shutdown() on the decode nsIThread. This will block until the decode thread has exited from the nsOggDecodeStateMachine::Run() method. While the main thread is waiting, it can run other events. If one of these events initiates a Seek from JS, it will start a new Load (since we're ENDED). We'll then be in the situation as described in comment 1. E.g. in Load() with non-null mReader/mDecodeStateMachine fields, and we'll destroy the reader, and then the destructor for nsOggStateMachine will call into liboggplay which will use the now destroyed reader. Additionally, even if we didn't crash, when we eventually got back to running nsOggDecoder::Stop(), we'd null out mReader and mDecodeStateMachine in nsOggDecoder::Stop(), (probably) undoing the just-started load.

Here's a call stack for this case:

00000002() - Crash!
gklayout.dll!oggplay_close(_OggPlay * me=0x04730540)  Line 590 + 0x10 bytes	C
gklayout.dll!nsOggDecodeStateMachine::~nsOggDecodeStateMachine()  Line 489 + 0xc bytes	C++
gklayout.dll!nsOggDecodeStateMachine::`scalar deleting destructor'()  + 0xf bytes	C++
xpcom_core.dll!nsRunnable::Release()  Line 51 + 0x90 bytes	C++
gklayout.dll!nsCOMPtr<nsOggDecodeStateMachine>::assign_assuming_AddRef(nsOggDecodeStateMachine * newPtr=0x06232c58)  Line 496	C++
gklayout.dll!nsCOMPtr<nsOggDecodeStateMachine>::assign_with_AddRef(nsISupports * rawPtr=0x06232c58)  Line 1172	C++
gklayout.dll!nsCOMPtr<nsOggDecodeStateMachine>::operator=(nsOggDecodeStateMachine * rhs=0x06232c58)  Line 641	C++
>	gklayout.dll!nsOggDecoder::Load(nsIURI * aURI=0x0455fd18, nsIChannel * aChannel=0x00000000, nsIStreamListener * * aStreamListener=0x00000000)  Line 1283	C++
gklayout.dll!nsOggDecoder::ChangeState(nsOggDecoder::PlayState aState=PLAY_STATE_SEEKING)  Line 1623	C++
gklayout.dll!nsOggDecoder::Seek(float aTime=1.0000000)  Line 1327	C++
gklayout.dll!nsHTMLMediaElement::SetCurrentTime(float aCurrentTime=1.0000000)  Line 239 + 0x22 bytes	C++
gklayout.dll!nsHTMLVideoElement::SetCurrentTime(float aCurrentTime=1.0000000)  Line 61 + 0x13 bytes	C++
xpcom_core.dll!NS_InvokeByIndex_P(nsISupports * that=0x0000003f, unsigned int methodIndex=0x00000001, unsigned int paramCount=0x0012e0b8, nsXPTCVariant * params=0x00cd0000)  Line 102	C++
xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...}, XPCWrappedNative::CallMode mode=0x0000003f)  Line 2422 + 0x21 bytes	C++
xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...}, XPCWrappedNative::CallMode mode=CALL_SETTER)  Line 2422 + 0x21 bytes	C++
xpc3250.dll!XPCWrappedNative::SetAttribute(XPCCallContext & ccx={...})  Line 2299 + 0xe bytes	C++
xpc3250.dll!XPC_WN_GetterSetter(JSContext * cx=0x042cf488, JSObject * obj=0x059a2400, unsigned int argc=0x00000001, long * argv=0x03437588, long * vp=0x0012e39c)  Line 1501 + 0xc bytes	C++
js3250.dll!js_Invoke(JSContext * cx=0x042cf488, unsigned int argc=0x00000001, long * vp=0x03437580, unsigned int flags=0x00000002)  Line 1313 + 0x1a bytes	C++
js3250.dll!js_InternalInvoke(JSContext * cx=0x042cf488, JSObject * obj=0x059a2400, long fval=0x059b0aa0, unsigned int flags=0x00000000, unsigned int argc=0x00000001, long * argv=0x0012f36c, long * rval=0x0012f36c)  Line 1388 + 0x15 bytes	C++
js3250.dll!js_InternalGetOrSet(JSContext * cx=0x042cf488, JSObject * obj=0x059a2400, long id=0x04bb0c74, long fval=0x059b0aa0, JSAccessMode mode=JSACC_WRITE, unsigned int argc=0x00000001, long * argv=0x0012f36c, long * rval=0x0012f36c)  Line 1449 + 0x1f bytes	C++
js3250.dll!js_SetPropertyHelper(JSContext * cx=0x042cf488, JSObject * obj=0x059a2400, long id=0x04bb0c74, long * vp=0x0012f36c, JSPropCacheEntry * * entryp=0x0012f144)  Line 3952 + 0x32 bytes	C++
js3250.dll!js_Interpret(JSContext * cx=0x042cf488)  Line 4630 + 0x1f bytes	C++
js3250.dll!js_Invoke(JSContext * cx=0x042cf488, unsigned int argc=0x00000001, long * vp=0x034374d8, unsigned int flags=0x00000000)  Line 1331 + 0x9 bytes	C++
js3250.dll!js_InternalInvoke(JSContext * cx=0x042cf488, JSObject * obj=0x04aa2d00, long fval=0x04153640, unsigned int flags=0x00000000, unsigned int argc=0x00000001, long * argv=0x047b3e40, long * rval=0x0012f564)  Line 1388 + 0x15 bytes	C++
js3250.dll!JS_CallFunctionValue(JSContext * cx=0x042cf488, JSObject * obj=0x04aa2d00, long fval=0x04153640, unsigned int argc=0x00000001, long * argv=0x047b3e40, long * rval=0x0012f564)  Line 5245 + 0x1f bytes	C++
gklayout.dll!nsJSContext::CallEventHandler(nsISupports * aTarget=0x06283c80, void * aScope=0x04aa2d00, void * aHandler=0x04153640, nsIArray * aargv=0x047679a4, nsIVariant * * arv=0x0012f61c)  Line 1989 + 0x21 bytes	C++
gklayout.dll!nsGlobalWindow::RunTimeout(nsTimeout * aTimeout=0x047679e8)  Line 7661 + 0xab bytes	C++
gklayout.dll!nsGlobalWindow::TimerCallback(nsITimer * aTimer=0x04797cb0, void * aClosure=0x047679e8)  Line 7996	C++
xpcom_core.dll!nsTimerImpl::Fire()  Line 420 + 0xe bytes	C++
xpcom_core.dll!nsTimerEvent::Run()  Line 514	C++
xpcom_core.dll!nsThread::ProcessNextEvent(int mayWait=0x00000001, int * result=0x0012f784)  Line 511	C++
xpcom_core.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x00d06240, int mayWait=0x00000001)  Line 227 + 0x16 bytes	C++
xpcom_core.dll!nsThread::Shutdown()  Line 465 + 0xb bytes	C++
gklayout.dll!nsOggDecoder::Stop()  Line 1367	C++
gklayout.dll!nsOggDecoder::PlaybackEnded()  Line 1477	C++
gklayout.dll!nsRunnableMethod<nsOggDecoder>::Run()  Line 265	C++
xpcom_core.dll!nsThread::ProcessNextEvent(int mayWait=0x00000001, int * result=0x0012f848)  Line 511	C++
xpcom_core.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x00d06240, int mayWait=0x00000001)  Line 227 + 0x16 bytes	C++
gkwidget.dll!nsBaseAppShell::Run()  Line 170 + 0xc bytes	C++
tkitcmps.dll!nsAppStartup::Run()  Line 192 + 0x1c bytes	C++
xul.dll!XRE_main(int argc=0x00000004, char * * argv=0x00d00600, const nsXREAppData * aAppData=0x00cda328)  Line 3283 + 0x25 bytes	C++
firefox.exe!NS_internal_main(int argc=0x00000004, char * * argv=0x00d00600)  Line 156 + 0x12 bytes	C++
firefox.exe!wmain(int argc=0x00000004, wchar_t * * argv=0x00cda140)  Line 87 + 0xd bytes	C++
firefox.exe!__tmainCRTStartup()  Line 594 + 0x19 bytes	C
firefox.exe!wmainCRTStartup()  Line 414	C
kernel32.dll!_BaseProcessStart@4()  + 0x23 bytes	

I can reproduce this with the testcase for bug 469016 with the patch for bug 469016 applied.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
This changes nsOggDecoder::Stop() to create a new event which holds refs to the state machine, decode thread, and reader, which it destroys later on the main thread. Stop() can then null out those refs synchronously without worrying about events being fired in mDecodeThread->Shutdown().
Assignee: nobody → chris
Status: REOPENED → ASSIGNED
Attachment #352471 - Flags: review?(chris.double)
As Patch v1, except this changes nsOggDecoder::Shutdown() to call Stop() rather than dispatching an event to call Stop() which is going to dispatch an event to destroy stuff. Means we send one less event.

I also had to add a reference to the nsOggDecoder to the destroy event, since we can have references to nsOggDecoder in event runners on the stack in nsOggDecodeStateMachine::Run(), which could result in the nsOggDecoder being destroyed on a non-main thread, and its monitor complains about that.
Attachment #352471 - Attachment is obsolete: true
Attachment #352475 - Flags: review?(chris.double)
Attachment #352471 - Flags: review?(chris.double)
Attachment #352475 - Flags: review?(chris.double) → review+
Attachment #352475 - Flags: superreview?(roc)
Attachment #352475 - Flags: superreview?(roc) → superreview+
Comment on attachment 352475 [details] [diff] [review]
Patch v2 - call Stop() directly in Shutdown()

+  NS_ASSERTION(mReader==nsnull, "Didn't shutdown properly!");
+  NS_ASSERTION(mDecodeStateMachine==nsnull, "Didn't shutdown properly!");
+  NS_ASSERTION(mDecodeThread==nsnull, "Didn't shutdown properly!");

"!mReader", etc
Attached patch Patch v2.1Splinter Review
As Patch v2 with Roc's comment addressed. r+ chris.double sr+ roc.
Attachment #352475 - Attachment is obsolete: true
Attachment #352659 - Flags: superreview+
Attachment #352659 - Flags: review+
Pushed to trunk as 71f89359e747
Status: ASSIGNED → RESOLVED
Closed: 16 years ago16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 landing]
Pushed 02c6712587b4 to 1.9.1
Keywords: fixed1.9.1
Whiteboard: [needs 191 landing]
Severity: normal → critical
Keywords: crash
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.2a1
Hi Chris, can we get a testcase added to this patch?  If that's too difficult, would verifying any playback to stop of an ogg video verify this patch?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: