Closed Bug 1196696 Opened 4 years ago Closed 4 years ago

crash in mozilla::MediaFormatReader::DecoderData::ResetDemuxer()

Categories

(Core :: General, defect, critical)

41 Branch
x86
Windows NT
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox40 --- unaffected
firefox41 + fixed
firefox42 --- fixed
firefox43 --- fixed

People

(Reporter: philipp, Assigned: jya)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-e1c75185-45f3-483c-b44d-3c5612150818.
=============================================================

Crashing Thread
Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::MediaFormatReader::DecoderData::ResetDemuxer() 	dom/media/MediaFormatReader.h
1 	xul.dll 	mozilla::MediaFormatReader::ResetDecode() 	dom/media/MediaFormatReader.cpp
2 	xul.dll 	mozilla::MediaSourceReader::ResetDecode() 	dom/media/mediasource/MediaSourceReader.cpp
3 	xul.dll 	nsRunnableMethodImpl<void ( mozilla::dom::indexedDB::`anonymous namespace'::DatabaseOfflineStorage::*)(void), 1>::Run() 	xpcom/glue/nsThreadUtils.h
4 	xul.dll 	mozilla::AutoTaskDispatcher::TaskGroupRunnable::Run() 	dom/media/TaskDispatcher.h
5 	xul.dll 	mozilla::MediaTaskQueue::Runner::Run() 	dom/media/MediaTaskQueue.cpp
6 	xul.dll 	nsThreadPool::Run() 	xpcom/threads/nsThreadPool.cpp
7 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp
8 	xul.dll 	NS_ProcessNextEvent(nsIThread*, bool) 	xpcom/glue/nsThreadUtils.cpp
9 	xul.dll 	mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp
10 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc
11 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc
12 	xul.dll 	nsThread::ThreadFunc(void*) 	xpcom/threads/nsThread.cpp
13 	nss3.dll 	_PR_NativeRunThread 	nsprpub/pr/src/threads/combined/pruthr.c
14 	nss3.dll 	pr_root 	nsprpub/pr/src/md/windows/w95thred.c
15 	msvcr120.dll 	_callthreadstartex 	f:\dd\vctools\crt\crtw32\startup\threadex.c:376
16 	msvcr120.dll 	msvcr120.dll@0x2c000 	
17 	kernel32.dll 	BaseThreadInitThunk 	
18 	ntdll.dll 	__RtlUserThreadStart 	
19 	ntdll.dll 	_RtlUserThreadStart

according to crash stats this crash shows up in firefox 41 builds on windows, but not in prior or subsequent versions. 
possibly related to http://hg.mozilla.org/releases/mozilla-beta/diff/5cf8087b6812/dom/media/MediaFormatReader.h and the landed code in bug 1163227?
And another crash that makes no sense. A null deref, but if HasAudio() returned true, then the audio demuxer demuxer must exist and not be null!
Assignee: nobody → jyavenard
HasAudio() returns true if mInfo.HasAudio() returns true.
mInfo.mAudio is created by the track demuxer, so for mInfo.HasAudio() to return true, mAudio.mDemuxer will always be set.

The only time in the lifetime of the MediaFormatReader that a demuxer is null, is after shutdown..

So that would mean MediaDecoderReader::ResetDecode() is called after Shutdown.

Chris, do you see a way this could be happening?
Flags: needinfo?(cpearce)
JW would you have some suggestions?
Flags: needinfo?(jwwang)
All seems unlikely to me:
1. ResetDecode() called after Shutdown
2. https://hg.mozilla.org/mozilla-central/file/29b2df16e961/dom/media/MediaFormatReader.cpp#l1208
mInfo.mAudio.mRate or mInfo.mAudio.mChannels are tampered so HasAudio() becomes true
3. memory overwrite at random position.

How is it reproducible?
Flags: needinfo?(jwwang)
Tracked for FF41 as this was mentioned in the channel meeting today, we are hitting this crash at ~1%
Attachment #8652671 - Flags: review?(jwwang) → review+
I still don't see how the crash could ever happen; the patch will get around the null deref ; but if we get in ResetDecode() after Shutdown() then it will continue to cause problem (but elsewhere)
Flags: needinfo?(cpearce)
https://hg.mozilla.org/mozilla-central/rev/28fb66c200e4
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Comment on attachment 8652671 [details] [diff] [review]
Always check that track demuxer was successfully created.

Approval Request Comment
[Feature/regressing bug #]: 1171379
[User impact if declined]: Crashes.
[Describe test coverage new/current, TreeHerder]: local test 
[Risks and why]: Very low. Used to assert, now returns an error
[String/UUID change made/needed]: None
Attachment #8652671 - Flags: approval-mozilla-beta?
Attachment #8652671 - Flags: approval-mozilla-aurora?
I see a single crash with 42 when it was in nightly. I guess this version is indeed affected (and will probably come back in 42 beta if we don't take the patch).
Flags: needinfo?(jyavenard)
Comment on attachment 8652671 [details] [diff] [review]
Always check that track demuxer was successfully created.

Fix a crash, taking it.
Attachment #8652671 - Flags: approval-mozilla-beta?
Attachment #8652671 - Flags: approval-mozilla-beta+
Attachment #8652671 - Flags: approval-mozilla-aurora?
Attachment #8652671 - Flags: approval-mozilla-aurora+
Flags: needinfo?(jyavenard)
Depends on: 1207441
You need to log in before you can comment on or make changes to this bug.