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

VERIFIED FIXED in Firefox 36

Status

()

P1
critical
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: marcia, Assigned: mattwoodrow)

Tracking

(Blocks: 1 bug, {crash, regression})

37 Branch
mozilla37
All
Windows 8.1
crash, regression
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox36 verified, firefox37 verified)

Details

(crash signature, URL)

Attachments

(1 attachment)

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
Still a fairly low volume crash, with some signatures as recent as yesterday's build -> 2015010403.
(Assignee)

Comment 2

4 years ago
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)
(Assignee)

Comment 4

4 years ago
Assignee: nobody → matt.woodrow
Attachment #8544903 - Flags: review?(cpearce)
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.
Attachment #8544903 - Flags: review?(cpearce) → review+
https://hg.mozilla.org/mozilla-central/rev/c9177ec839af
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
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.
Attachment #8544903 - Flags: approval-mozilla-beta?
status-firefox36: --- → affected
status-firefox37: --- → fixed
Attachment #8544903 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
Keywords: regressionwindow-wanted
No crash reports in Socorro after 2015-01-14 builds. Marking as verified.
Status: RESOLVED → VERIFIED
status-firefox36: fixed → verified
status-firefox37: fixed → verified
You need to log in before you can comment on or make changes to this bug.