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

Status: VERIFIED FIXED

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

Description

[Marcia Knous [:marcia]]

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: https://crash-stats.mozilla.com/report/list?productFirefox&signature=mozilla::MP4Reader::Update%28mp4_demuxer::TrackType%29

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 youtube.com.

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/message_loop.cc
8 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc
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

Comment 1

[Marcia Knous [:marcia]]

Still a fairly low volume crash, with some signatures as recent as yesterday's build -> 2015010403.

Comment 2

[Matt Woodrow (:mattwoodrow)]

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?

Comment 3

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

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?

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: shutdown-update

Comment 5

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

Comment on attachment 8544903 [details] [diff] [review]
shutdown-update

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.

Comment 6

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c9177ec839af

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c9177ec839af

Comment 8

[Ralph Giles (:rillian)]

Comment on attachment 8544903 [details] [diff] [review]
shutdown-update

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.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2fd2c6de0a87

Comment 10

[Petruta Horea [:phorea], Desktop Test Engineering]

No crash reports in Socorro after 2015-01-14 builds. Marking as verified.