Closed Bug 1116284 Opened 9 years ago Closed 9 years ago

crash in mozilla::MP4Reader::Update(mp4_demuxer::TrackType)


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

37 Branch
Windows 8.1



Tracking Status
firefox36 --- verified
firefox37 --- verified


(Reporter: marcia, Assigned: mattwoodrow)


(Blocks 1 open bug, )


(Keywords: crash, regression)

Crash Data


(1 file)

This bug was filed from the Socorro interface and is 
report bp-2b51c12e-1969-453a-9e5a-353412141229.

Seen while reviewing crash stats. More reports here:

Crashes started showing up using 2014121803 and have continued up until 12-29. These crashes are primarily seen on Windows 7 and 8.1 and almost all URLs involve

Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::MP4Reader::Update(mp4_demuxer::TrackType) 	dom/media/fmp4/MP4Reader.cpp
1 	xul.dll 	nsRunnableMethodImpl<void ( mozilla::MP4Reader::*)(mp4_demuxer::TrackType), mp4_demuxer::TrackType, 1>::Run() 	xpcom/glue/nsThreadUtils.h
2 	xul.dll 	mozilla::MediaTaskQueue::Runner::Run() 	dom/media/MediaTaskQueue.cpp
3 	xul.dll 	nsThreadPool::Run() 	xpcom/threads/nsThreadPool.cpp
4 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp
5 	xul.dll 	NS_ProcessNextEvent(nsIThread*, bool) 	xpcom/glue/nsThreadUtils.cpp
6 	xul.dll 	mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp
7 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/
8 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/
9 	xul.dll 	nsThread::ThreadFunc(void*) 	xpcom/threads/nsThread.cpp
10 	nss3.dll 	PR_NativeRunThread 	nsprpub/pr/src/threads/combined/pruthr.c
11 	nss3.dll 	pr_root 	nsprpub/pr/src/md/windows/w95thred.c
12 	msvcr120.dll 	_callthreadstartex 	f:\dd\vctools\crt\crtw32\startup\threadex.c:376
13 	msvcr120.dll 	_threadstartex 	f:\dd\vctools\crt\crtw32\startup\threadex.c:354
14 	kernel32.dll 	BaseThreadInitThunk 	
15 	msvcr120.dll 	endthreadex 	f:\dd\vctools\crt\crtw32\startup\threadex.c:431
16 	msvcr120.dll 	endthreadex 	f:\dd\vctools\crt\crtw32\startup\threadex.c:431
Ø 17 	ntdll.dll 	ntdll.dll@0x703c3 	
18 	kernel32.dll 	GetThreadPriorityStub 	
Ø 19 	kernelbase.dll 	kernelbase.dll@0xe1acf
Still a fairly low volume crash, with some signatures as recent as yesterday's build -> 2015010403.
I think this is a regression from the async shutdown changes.

MP4Reader::Shutdown synchronously shuts down the mVideo/mAudio.mDecoders, and then we call into MediaDecoderReader::Shutdown which asynchronously waits for the task queue to be shut down.

That means we can still get a call to MP4Reader::Update after we've shut down the MP4Reader (if one was waiting in the queue), so mVideo/mAudio.mDecoder could be nullptr.

The simple fix is to just null check and avoid dereferencing it, but I'm having a hard time convincing myself that we'll avoid messing up promises by doing this.

One alternative is to have a 'post shutdown' method, and do the MP4Reader synchronous shutdown pieces in that.

Any thoughts on that Chris?
Flags: needinfo?(cpearce)
We could make MediaDecoderReader::mShutdown protected (or add a protected accessor function) and just check that inside MP4Reader::Update() instead of null checking. It's the only dispatch on the MP4Reader that I can see. As long as we only call Update() on the reader's task queue (which I think it is), this should be threadsafe.

Why would we mess-up the promises by doing this?
Flags: needinfo?(cpearce)
Attached patch shutdown-updateSplinter Review
Assignee: nobody → matt.woodrow
Attachment #8544903 - Flags: review?(cpearce)
Comment on attachment 8544903 [details] [diff] [review]

Review of attachment 8544903 [details] [diff] [review]:

::: dom/media/fmp4/MP4Reader.cpp
@@ +544,5 @@
>  MP4Reader::Update(TrackType aTrack)
>  {
>    MOZ_ASSERT(GetTaskQueue()->IsCurrentThreadIn());
> +  if (mShutdown) {

Should probably have a comment here explaining what you're guarding against.
Attachment #8544903 - Flags: review?(cpearce) → review+
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Comment on attachment 8544903 [details] [diff] [review]

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Users exposed to more crashes in video playback.
[Describe test coverage new/current, TBPL]: Landed on m-c.
[Risks and why]: Low, we just avoid doing work on shutdown decoders. Small, clean change.
[String/UUID change made/needed]: None.
Attachment #8544903 - Flags: approval-mozilla-beta?
Attachment #8544903 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
No crash reports in Socorro after 2015-01-14 builds. Marking as verified.
You need to log in before you can comment on or make changes to this bug.