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

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[[:philipp]]

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?

Comment 1

[Jean-Yves Avenard [:jya]]

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!

Comment 2

[Jean-Yves Avenard [:jya]]

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?

Comment 3

[Jean-Yves Avenard [:jya]]

JW would you have some suggestions?

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

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?

Comment 5

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked for FF41 as this was mentioned in the channel meeting today, we are hitting this crash at ~1%

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: Always check that track demuxer was successfully created.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/28fb66c200e4

Comment 8

[Jean-Yves Avenard [:jya]]

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)

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/28fb66c200e4

Comment 10

[Jean-Yves Avenard [:jya]]

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

Comment 11

[Sylvestre Ledru [:Sylvestre]]

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).

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8652671 [details] [diff] [review]
Always check that track demuxer was successfully created.

Fix a crash, taking it.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6929efd5d98e

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cc5c3cf9f4d6