Crash, null pointer deref [@ nsBuiltinDecoderStateMachine::GetBuffered ]

RESOLVED FIXED in mozilla9



6 years ago
2 years ago


(Reporter: bjacob, Assigned: kinetik)


({crash, regression, topcrash})

crash, regression, topcrash

Firefox Tracking Flags

(firefox8- wontfix, firefox9-)


(Whiteboard: [qa-], crash signature)


(1 attachment)

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

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

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

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


6 years ago
Severity: normal → critical
Keywords: crash, topcrash

Comment 1

6 years ago says we have also been seeing this on Mac - and also on a 8.0a2 (aurora) build, so affects 8 as well, probably.

Comment 2

6 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.
Blocks: 592833
Keywords: regression
OS: Linux → All
Hardware: x86_64 → All
I just ran into this on, with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:8.0a2) Gecko/20110911 Firefox/8.0a2.


6 years ago
Assignee: nobody → kinetik

Comment 4

6 years ago
Created attachment 561649 [details] [diff] [review]
patch v0

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.
Attachment #561649 - Flags: review?(chris)
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.
Attachment #561649 - Flags: review?(chris) → review+

Comment 6

6 years ago
Comment on attachment 561649 [details] [diff] [review]
patch v0


6 years ago
Target Milestone: --- → mozilla9
Last Resolved: 6 years ago
Resolution: --- → FIXED
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)
tracking-firefox9: --- → ?

Comment 9

6 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.
QA needs a minimized testcase to verify this fix -- given the requirements in comment 9.

qa- until a testcase can be produced.
Keywords: testcase-wanted
Whiteboard: [qa-]
Some Windows crashes showing up in Beta -*%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.
Crash Signature: [@ nsBuiltinDecoderStateMachine::GetBuffered ] → [@ nsBuiltinDecoderStateMachine::GetBuffered ] [@ nsBuiltinDecoderStateMachine::GetBuffered(nsTimeRanges*) ]
tracking-firefox8: --- → ?

Comment 12

6 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.
status-firefox8: --- → wontfix
tracking-firefox8: ? → -
tracking-firefox9: ? → -
Keywords: testcase-wanted
You need to log in before you can comment on or make changes to this bug.