Crash in mozilla::MediaChannelStatistics::Stop

RESOLVED FIXED in Firefox 55

Status

()

Product:
Core
Component:
Audio/Video: Playback
Type:
defect
Importance:
P1
critical
Status:
RESOLVED FIXED
Opened:
2 years ago
Updated:
2 years ago

People

(Reporter: philipp, Assigned: jwwang)

Tracking

({crash})

Version:
52 Branch
Target:
mozilla55
Points:
---

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox53 wontfix, firefox54 wontfix, firefox55 fixed)

Details

(crash signature)

Attachments

(3 attachments)

Bug 1367705. P1 - remove unused RecordStatisticsTo().
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1367705. P2 - since RecordStatisticsTo() is removed in P1, we can now init mChannelStatistics in the constructor.
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1367705. P3 - fix MediaChannelStatistics which no longer needs to be sharable.
59 bytes, text/x-review-board-request
cpearce
: review+
Details 
This bug was filed from the Socorro interface and is 
report bp-1af9bf4c-1657-42e9-8184-fd4640170525.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::MediaChannelStatistics::Stop() 	dom/media/MediaResource.h:97
1 	xul.dll 	mozilla::ChannelMediaResource::CloseChannel() 	dom/media/MediaResource.cpp:640
2 	xul.dll 	mozilla::ChannelMediaResource::Close() 	dom/media/MediaResource.cpp:592
3 	xul.dll 	mozilla::MediaDecoder::Shutdown() 	dom/media/MediaDecoder.cpp:514
4 	xul.dll 	mozilla::dom::HTMLMediaElement::ShutdownDecoder() 	dom/html/HTMLMediaElement.cpp:1588
5 	xul.dll 	mozilla::dom::HTMLMediaElement::FinishDecoderSetup(mozilla::MediaDecoder*, mozilla::MediaResource*, nsIStreamListener**) 	dom/html/HTMLMediaElement.cpp:4634
6 	xul.dll 	mozilla::dom::HTMLMediaElement::InitializeDecoderForChannel(nsIChannel*, nsIStreamListener**) 	dom/html/HTMLMediaElement.cpp:4601
7 	xul.dll 	mozilla::dom::HTMLMediaElement::MediaLoadListener::OnStartRequest(nsIRequest*, nsISupports*) 	dom/html/HTMLMediaElement.cpp:531
8 	xul.dll 	nsJARChannel::OnStartRequest(nsIRequest*, nsISupports*) 	modules/libjar/nsJARChannel.cpp:1022
9 	xul.dll 	nsInputStreamPump::OnStateStart() 	netwerk/base/nsInputStreamPump.cpp:524
10 	xul.dll 	nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) 	netwerk/base/nsInputStreamPump.cpp:426
11 	xul.dll 	nsInputStreamReadyEvent::Run() 	xpcom/io/nsStreamUtils.cpp:95
12 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp:1240
13 	xul.dll 	XPTC__InvokebyIndex 	xpcom/reflect/xptcall/md/win32/xptcinvoke_asm_x86_64.asm:97
14 	xul.dll 	js::jit::ICTypeMonitor_Fallback::addMonitorStubForValue(JSContext*, js::jit::SharedStubInfo*, JS::Handle<JS::Value>) 	js/src/jit/SharedIC.cpp:2504
15 		@0xfff8ffffffffffff

crash reports with this signature started showing up since firefox 53 - they are rather low volume though. over three quarters of those crashes are coming form users with chinese builds of firefox.

the reports also started on 52.1.0esr which makes it likely that something that got uplifted in this pushlog is at the root of this:
https://hg.mozilla.org/releases/mozilla-esr52/pushloghtml?fromchange=FIREFOX_52_0_2esr_RELEASE&tochange=FIREFOX_52_1_0esr_RELEASE
Win32 Nightly is out: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0 ID:20140625030206 CSet: a19e0434ea52

I visited the site to watch the video but an error occurred
http://thanhlycuongphat.com
http://banmaytinhgiare.com
http://toursapagiatot.com
http://tourhalong1ngay.com
http://vnco.net
http://cantho60s.com
Component: Audio/Video → Audio/Video: Playback
https://hg.mozilla.org/releases/mozilla-release/annotate/1fe643b3421a/dom/media/MediaResource.cpp#l640

Looks like a null-deref of mChannelStatistics which could happen when a ChannelMediaResource is Close()ed without being Open()ed.

I don't think this is a regression since the problematic code is introduced by bug 820588.

https://hg.mozilla.org/mozilla-central/rev/c926d32f8ace#l4.69
Assignee: nobody → jwwang
Keywords: regression
See Also: → 820588
Attachment #8873728 - Flags: review?(cpearce)
Attachment #8873729 - Flags: review?(cpearce)
Attachment #8873730 - Flags: review?(cpearce)
Comment on attachment 8873730 [details]
Bug 1367705. P3 - fix MediaChannelStatistics which no longer needs to be sharable.

https://reviewboard.mozilla.org/r/145132/#review149864

r+ with comments addressed.

::: dom/media/MediaDecoder.h:663
(Diff revision 1)
>    RefPtr<VideoFrameContainer> mVideoFrameContainer;
>  
>    // Data needed to estimate playback data rate. The timeline used for
>    // this estimate is "decode time" (where the "current time" is the
>    // time of the last decoded video frame).
> -  RefPtr<MediaChannelStatistics> mPlaybackStatistics;
> +  UniquePtr<MediaChannelStatistics> mPlaybackStatistics;

Since this isn't being shared anymore, this can just be:

MediaChannelStatistics mPlaybackStatistics

::: dom/media/MediaResource.h:692
(Diff revision 1)
>    // Any thread access
>    MediaCacheStream mCacheStream;
>  
>    // This lock protects mChannelStatistics
>    Mutex               mLock;
> -  RefPtr<MediaChannelStatistics> mChannelStatistics;
> +  UniquePtr<MediaChannelStatistics> mChannelStatistics;

Since this isn't being shared anymore, this can just be:

MediaChannelStatistics mChannelStatistics

::: dom/media/MediaResource.cpp:650
(Diff revision 1)
>  {
>    NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
>    NS_ASSERTION(mCacheStream.IsAvailableForSharing(), "Stream can't be cloned");
>  
>    RefPtr<ChannelMediaResource> resource = new ChannelMediaResource(
> -    aCallback, nullptr, mURI, GetContentType(), mChannelStatistics);
> +    aCallback, nullptr, mURI, GetContentType(), *mChannelStatistics);

I don't like this because references are supposed to \*never\* be null, but you're passing a deref'd pointer in, so it \*could\* be null. It's not currently likely to be null because of how you're doing things, but someone could change the code in future to make it null, and it feels like you're circumventing the type system to achieve what you want here.

Can you make the MediaChannelStatistics members of ChannelMediaResource and MediaDecoder allocated inside their owners directly, i.e.:

Instead of declaring the MediaChannelStatistics instancecs in their owning class definitions as:

UniquePtr<MediaChannelStatistics> mChannelStatistics;

Just do:
MediaChannelStatistics mChannelStatistics;

Then it makes sense to use a const reference, and it can never be null. This means the type system will help us ensure that what we're passing into here is safe.

If you're really set on storing the MediaChannelStatistics in a UniquePtr, then you should pass it in as:
const UniquePtr<const MediaChannelStatistics>&, and null check it. Which is a bit of a mouthful. So I reckon you just want to allocate the MediaChannelStatistics inside their owning objects directly.
Attachment #8873730 - Flags: review?(cpearce) → review+
Comment on attachment 8873729 [details]
Bug 1367705. P2 - since RecordStatisticsTo() is removed in P1, we can now init mChannelStatistics in the constructor.

https://reviewboard.mozilla.org/r/145130/#review149868
Attachment #8873729 - Flags: review?(cpearce) → review+
Comment on attachment 8873728 [details]
Bug 1367705. P1 - remove unused RecordStatisticsTo().

https://reviewboard.mozilla.org/r/145128/#review149866

::: commit-message-10562:1
(Diff revision 1)
> +Bug 1367705. P1 - remove unused RecordStatisticsTo().

Your commit messages should also describe why you're making the change you're making, to make it easier for people looking back on the commit to figure out why you did what you're doing.
Attachment #8873728 - Flags: review?(cpearce) → review+
Thanks! I've addresses the issues.
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b4002383a249
P1 - remove unused RecordStatisticsTo(). r=cpearce
https://hg.mozilla.org/integration/autoland/rev/1ab72fcadc31
P2 - since RecordStatisticsTo() is removed in P1, we can now init mChannelStatistics in the constructor. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/4037edd64d87
P3 - fix MediaChannelStatistics which no longer needs to be sharable. r=cpearce
Priority: -- → P1
This has probably missed 54 already (though we could probably consider it for a dot-release if the risk and crash volume warranted it). Likewise the same for ESR52.
The crash volume is quite low. I would prefer to let it ride the train.
You need to log in before you can comment on or make changes to this bug.