Closed
Bug 1116284
Opened 9 years ago
Closed 9 years ago
crash in mozilla::MP4Reader::Update(mp4_demuxer::TrackType)
Categories
(Core :: Audio/Video, defect, P1)
Tracking
()
VERIFIED
FIXED
mozilla37
People
(Reporter: marcia, Assigned: mattwoodrow)
References
(Blocks 1 open bug, )
Details
(Keywords: crash, regression)
Crash Data
Attachments
(1 file)
2.12 KB,
patch
|
cpearce
:
review+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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
Updated•9 years ago
|
Priority: -- → P1
Reporter | ||
Comment 1•9 years ago
|
||
Still a fairly low volume crash, with some signatures as recent as yesterday's build -> 2015010403.
Assignee | ||
Comment 2•9 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)
Comment 3•9 years ago
|
||
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•9 years ago
|
||
Assignee: nobody → matt.woodrow
Attachment #8544903 -
Flags: review?(cpearce)
Comment 5•9 years ago
|
||
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+
Assignee | ||
Comment 6•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9177ec839af
Comment 7•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c9177ec839af
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Comment 8•9 years ago
|
||
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?
Updated•9 years ago
|
status-firefox36:
--- → affected
status-firefox37:
--- → fixed
Updated•9 years ago
|
Attachment #8544903 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•9 years ago
|
Flags: qe-verify+
Reporter | ||
Updated•9 years ago
|
Keywords: regressionwindow-wanted
Comment 10•9 years ago
|
||
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.
Description
•