1128179 - Firefox crash in mozilla::SourceBufferDecoder::WasTrimmed()

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is 
report bp-10052775-fb3f-4e72-bc68-ede142150130.
=============================================================

Seen while looking at the Aurora explosive report, but this appears to be happening on Nightly as well as the most recent beta 5 which has yet to be pushed out (ID:20150129200438)

Crashes across all branches: http://bit.ly/1yYn0BQ

There are crashes dating back to build ID 2015012503, and crashes as recent as 2015013003 on nightly

Comments:
* Firefox crashs when i open videos on youtube using the html5 player 
* Youtube crashes 100% of the time. Normal HTML5 videos works correctly, for example http://www.w3.org/2010/05/video/mediaevents.html 
* just started firefox and visited youtube, when clicked a video to play the browser crashed. 
* when youtube is open on a tab, and open a new tab = crash 


rame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::SourceBufferDecoder::WasTrimmed() 	dom/media/mediasource/SourceBufferDecoder.h
1 	xul.dll 	mozilla::TrackBuffer::AppendData(mozilla::LargeDataBuffer*, __int64) 	dom/media/mediasource/TrackBuffer.cpp
2 	xul.dll 	mozilla::dom::SourceBuffer::AppendData(mozilla::LargeDataBuffer*, double) 	dom/media/mediasource/SourceBuffer.cpp
3 	xul.dll 	mozilla::dom::AppendDataRunnable::Run() 	dom/media/mediasource/SourceBuffer.cpp
4 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp
5 	xul.dll 	mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp
6 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc
7 	xul.dll 	nsThreadManager::GetMainThread(nsIThread**) 	xpcom/threads/nsThreadManager.cpp
8 	xul.dll 	nsBaseAppShell::Run() 	widget/nsBaseAppShell.cpp
9 	xul.dll 	nsAppStartup::Run() 	toolkit/components/startup/nsAppStartup.cpp
10 	xul.dll 	XREMain::XRE_mainRun() 	toolkit/xre/nsAppRunner.cpp
11 	xul.dll 	XREMain::XRE_main(int, char** const, nsXREAppData const*) 	toolkit/xre/nsAppRunner.cpp
12 	xul.dll 	nsAString_internal::Replace(unsigned int, unsigned int, wchar_t const*, unsigned int, mozilla::fallible_t const&) 	xpcom/string/nsTSubstring.cpp
13 	xul.dll 	nsLocalFile::AppendInternal(nsString const&, bool) 	xpcom/io/nsLocalFileWin.cpp
14 	xul.dll 	nsLocalFile::Append(nsAString_internal const&) 	xpcom/io/nsLocalFileWin.cpp
15 	kernel32.dll 	GetProcessPriorityBoost 	
16 	kernel32.dll 	GetCurrentProcess 	
17 	xul.dll 	std::basic_string<char, std::char_traits<char>, std::allocator<char> >::_Tidy(bool, unsigned int) 	c:/tools/vs2013/vc/include/xstring:2282
18 	xul.dll 	`anonymous namespace'::HistogramGet(char const*, char const*, unsigned int, unsigned int, unsigned int, unsigned int, bool, base::Histogram**) 	toolkit/components/telemetry/Telemetry.cpp
19 	xul.dll 	`anonymous namespace'::HistogramGet(char const*, char const*, unsigned int, unsigned int, unsigned int, unsigned int, bool, base::Histogram**) 	toolkit/components/telemetry/Telemetry.cpp
20 	xul.dll 	mozilla::TimeStamp::ProcessCreation(bool&) 	xpcom/ds/TimeStamp.cpp
21 		@0x2e9a7f

Comment 1

[Jean-Yves Avenard [:jya]]

This one will be fixed with bug 1125776.

But I can make a quicker fix if need be.

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Crash fix

Are you planning on uplifting the other bug to beta? If not (or not by monday), then we should take this.

Comment 3

[Jean-Yves Avenard [:jya]]

Comment on attachment 8557574 [details] [diff] [review]
Crash fix

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

I had this in my queue..

The fundamental issue is that it shows that you can get there with mCurrentDecoder being nullptr ; it should never have been the case as we must have received an init segment by now and a decoder would have been created.

As far as pushing my changes, I'm waiting for reviews...

Comment 4

[Matt Woodrow (:mattwoodrow)]

This can happen if initialization of the decoder failed, which is the case for bug 1128013.

Comment 5

[Jean-Yves Avenard [:jya]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> This can happen if initialization of the decoder failed, which is the case
> for bug 1128013.

Then this is not the proper fix... it will prevent the crash, but things will crap out after that.

     if (!decoders.NewDecoder(aTimestampOffset)) {
       return false;
     }

should be:
     if (!decoders.NewDecoder(aTimestampOffset)) {
       mParser = ContainerParser::CreateForMIMEType(mType);
       return false;
     }

so we never attempt to present a buffered range we can't play... and further call to appendBuffer will abort early (currently , as soon as we've seen an init segment, we proceed forward)

Comment 6

[Matt Woodrow (:mattwoodrow)]

(In reply to Jean-Yves Avenard [:jya] from comment #5)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> > This can happen if initialization of the decoder failed, which is the case
> > for bug 1128013.
> 
> Then this is not the proper fix... it will prevent the crash, but things
> will crap out after that.
> 
>      if (!decoders.NewDecoder(aTimestampOffset)) {
>        return false;
>      }
> 
> should be:
>      if (!decoders.NewDecoder(aTimestampOffset)) {
>        mParser = ContainerParser::CreateForMIMEType(mType);
>        return false;
>      }
> 
> so we never attempt to present a buffered range we can't play... and further
> call to appendBuffer will abort early (currently , as soon as we've seen an
> init segment, we proceed forward)

NewDecoder doesn't fail, ReadMetadata does (which calls DiscardDecoder).

Comment 8

[Marcia Knous [:marcia]]

This report was pretty explosive in Beta 5, so anything we can do to try to improve things in the next Beta would be great.

Comment 9

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6037ec1faa35

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6037ec1faa35

Comment 11

[Robert Kaiser]

[Tracking Requested - why for this release]:
Nominating for tracking 36 as this is ~3.2% of b5 crashes.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Matt, can we have an uplift request? Thanks

Comment 13

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8557574 [details] [diff] [review]
Crash fix

Approval Request Comment
[Feature/regressing bug #]: Bug 1096089
[User impact if declined]: Crashes on some videos
[Describe test coverage new/current, TreeHerder]: Few days on m-c
[Risks and why]: Very low risk, just a null check
[String/UUID change made/needed]: None

Comment 14

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8557574 [details] [diff] [review]
Crash fix

Trivial fix for crashes. Beta+ Aurora+

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0d5c5aa97c07
https://hg.mozilla.org/releases/mozilla-beta/rev/a5992bd7d600

Comment 16

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Socorro shows no more crashes after the fix landed.