1116626 - crash in mozilla::AbstractMediaDecoder::AutoNotifyDecoded::~AutoNotifyDecoded()

Status: VERIFIED FIXED

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

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is 
report bp-dca46c0a-b58c-4b52-9050-f138f2141230.
=============================================================

Seen while looking at crash stats. Link to all crashes: https://crash-stats.mozilla.com/report/list?productFirefox&signature=mozilla::AbstractMediaDecoder::AutoNotifyDecoded::~AutoNotifyDecoded%28%29

Started showing up with build ID=2014112503 and have continued up to 20141230030214. Not a high volume crash but all the URLs seem to be youtube.com. The majority of crashes appear to be Win 7.

Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::AbstractMediaDecoder::AutoNotifyDecoded::~AutoNotifyDecoded() 	dom/media/AbstractMediaDecoder.h
1 	xul.dll 	mozilla::MP4Reader::Update(mp4_demuxer::TrackType) 	dom/media/fmp4/MP4Reader.cpp
2 	xul.dll 	nsRunnableMethodImpl<void ( mozilla::CDMProxy::*)(unsigned int), unsigned int, 1>::Run() 	xpcom/glue/nsThreadUtils.h
3 	xul.dll 	mozilla::MediaTaskQueue::Runner::Run() 	dom/media/MediaTaskQueue.cpp
4 	xul.dll 	nsThreadPool::Run() 	xpcom/threads/nsThreadPool.cpp
5 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp
6 	xul.dll 	NS_ProcessNextEvent(nsIThread*, bool) 	xpcom/glue/nsThreadUtils.cpp
7 	xul.dll 	mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp
8 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc
9 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc
10 	xul.dll 	nsThread::ThreadFunc(void*) 	xpcom/threads/nsThread.cpp
11 	nss3.dll 	PR_NativeRunThread 	nsprpub/pr/src/threads/combined/pruthr.c
12 	nss3.dll 	pr_root 	nsprpub/pr/src/md/windows/w95thred.c
13 	msvcr120.dll 	_callthreadstartex 	f:\dd\vctools\crt\crtw32\startup\threadex.c:376
14 	msvcr120.dll 	_threadstartex 	f:\dd\vctools\crt\crtw32\startup\threadex.c:354
15 	kernel32.dll 	BaseThreadInitThunk 	
16 	ntdll.dll 	RtlUserThreadStart 	
17 	kernel32.dll 	BasepReportFault 	
18 	kernel32.dll 	BasepReportFault

Comment 1

[Marcia Knous [:marcia]]

This crash is still occurring as of yesterday's build id 2015010403.

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Null check mDecoder

Looks like we can get a callback to Update() after we've tried to shutdown and called ClearDecoder() (so mDecoder == nullptr).

I haven't been able to reproduce this, so not sure what else we can do other than preventing the crash in this case.

Comment 3

[Karl Tomlinson (:karlt)]

Comment on attachment 8544265 [details] [diff] [review]
Null check mDecoder

Yes, we need to null check.

MP4Reader::Update() has some other mDecoder dereferences.  It's not clear to
me from a quick look at the code that they will be avoided after reader
shutdown.  I also don't know how much of Update() needs to run if after reader shutdown.

(I thought about delaying setting mDecoder to null until the main thread task,
but that would just make things race.  Setting it to null on the decode task
queue seems the right place as that is the task queue where it will be accessed.)

Comment 4

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0aaec62e6684

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0aaec62e6684

Comment 6

[Ralph Giles (:rillian)]

Comment on attachment 8544265 [details] [diff] [review]
Null check mDecoder

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Youtube users will experience more crashes, less consistent testing.
[Describe test coverage new/current, TBPL]: Landed on m-c.
[Risks and why]: Risk is low, we just protect against an ininitialized access.
[String/UUID change made/needed]: None.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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

Comment 8

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

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