Benoit Jacob [:bjacob] (mostly away) Reporter
	Description • 13 years ago

I was just looking at the top crashers for Firefox 9.0a1 on Linux. This is ranking 17th.

https://crash-stats.mozilla.com/report/index/b0c27c54-85d5-4b8f-8d7c-4f6612110826

This is a segfault at address 0, at this line of code:

http://hg.mozilla.org/mozilla-central/annotate/6009974c1e1c/content/media/nsBuiltinDecoderStateMachine.cpp#l1869

Which at first glance suggests that mDecoder is null; but the calling frame is

http://hg.mozilla.org/mozilla-central/annotate/6009974c1e1c/content/html/content/src/nsHTMLMediaElement.cpp#l2602

and we're inside of this if():

  if (mReadyState >= nsIDOMHTMLMediaElement::HAVE_CURRENT_DATA && mDecoder) {

So... mDecoder can't be null, as we just checked for it. So I'm clueless :-)

	Robert Kaiser
	Comment 1 • 13 years ago

https://crash-stats.mozilla.com/report/list?range_value=4&range_unit=weeks&signature=nsBuiltinDecoderStateMachine::GetBuffered says we have also been seeing this on Mac - and also on a 8.0a2 (aurora) build, so affects 8 as well, probably.

	Matthew Gregan [:kinetik] Assignee
	Comment 2 • 13 years ago

The real call chain is nsHTMLMediaElement->nsBuiltinDecoder->nsBuiltinDecoderStateMachine (the nsBuiltinDecoder step is not present in the Linux crash report).  nsBuiltinDecoderStateMachine assumes its mDecoder is never null, but can become null during shutdown, where mDecoder.forget() is called during the SHUTDOWN state in RunStateMachine.  This opens a window where mDecoder->mDecoderStateMachine is valid, but mDecoderStateMachine->mDecoder is not.

This code last changed in bug 592833.

	Dirkjan Ochtman (:djc)
	Comment 3 • 13 years ago

I just ran into this on http://lab.simurai.com/toy/monster/, with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:8.0a2) Gecko/20110911 Firefox/8.0a2.

	Matthew Gregan [:kinetik] Assignee
	Comment 4 • 13 years ago

Because of the assumption that an nsBuiltinDecoderStateMachine's mDecoder is never null, there are a number of similar null-deref crashes, such as attempting to set the media element's volume after a network error has occurred.

Avoid this by changing the shutdown dance slightly so that the decoder and state machine's pointers to each other are dropped at the same time.

Patch also removes unused Decoder() call on nsMediaStream discovered while auditing places where null-checks may have to be added.  Also moves mDecoder and mState decls on nsBuiltinDecoderStateMachine from public(!) to protected.

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 5 • 13 years ago

Comment on attachment 561649 [details] [diff] [review]
patch v0

Review of attachment 561649 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thanks. Lets get this landed before the next m-c -> aurora merge on September 27.

	Matthew Gregan [:kinetik] Assignee
	Comment 6 • 13 years ago

Comment on attachment 561649 [details] [diff] [review]
patch v0

http://hg.mozilla.org/integration/mozilla-inbound/rev/cf4a13b84474

	(no longer active)
	Comment 7 • 13 years ago

https://hg.mozilla.org/mozilla-central/rev/cf4a13b84474

	u279076
	Comment 8 • 13 years ago

Does this need tracking/status for Firefox 9?

What is the testcase to verify this fix? I see a test URL in comment 3 but it's not clear to me what is needed to test for this crash (apart from simply checking crashstats for the prevalence of this crash post-fix)

	Matthew Gregan [:kinetik] Assignee
	Comment 9 • 13 years ago

I tested it by loading a video from an HTTP server that returned a 400 error for every request after the first.  After loading the video, I would seek into an unbuffered area, which would fail due to the HTTP error and cause the decoder to be shutdown.  Then execute element.buffered.length or element.volume=0 and see if a crash occurs.

	u279076
	Comment 10 • 13 years ago

QA needs a minimized testcase to verify this fix -- given the requirements in comment 9.

qa- until a testcase can be produced.

	Marcia Knous [:marcia]
	Comment 11 • 13 years ago

Some Windows crashes showing up in Beta - https://crash-stats.mozilla.com/report/list?signature=nsBuiltinDecoderStateMachine::GetBuffered%28nsTimeRanges*%29. The signature is slightly different than this one so I added it to the crash signature field.

The volume in Beta is not extremely high - 125 crashes in Beta 1 and 37 in Beta 2. We hit over 1 million ADU in Beta 1 so in the scheme of things it is not high volume, but maybe a nice to have fix for 8? Adding the tracking flag so the triage team can weigh in.

	LegNeato
	Comment 12 • 13 years ago

---------------------------------[ Triage Comment ]---------------------------------

Because the volumes are not high we will not track this for Firefox 8. Please renominate if information changes or we missed something.

This landed in Firefox 9 and the above holds so we won't track it there either.