Crash in mozilla::MediaChannelStatistics::Stop

RESOLVED FIXED in Firefox 55

Status

()

Product:
Core
Component:
Audio/Video: Playback
Importance:
P1
critical
Status:
RESOLVED FIXED
Reported:
a year ago
Modified:
a year ago

People

(Reporter: philipp, Assigned: jwwang)

Tracking

({crash})

Version:
52 Branch
Target:
mozilla55
Keywords:
crash
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
(Reporter)

Description

•
a year ago 
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

Comment 1

•
a year ago 
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
 (Assignee)

Comment 2

•
a year ago 
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
 (Assignee)

Updated

•
a year ago
See Also: → bug 820588
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

•
a year ago
Attachment #8873728 - Flags: review?(cpearce)
Attachment #8873729 - Flags: review?(cpearce)
Attachment #8873730 - Flags: review?(cpearce)

Comment 6

•
a year ago
mozreview-review 
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 7

•
a year ago
mozreview-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 8

•
a year ago
mozreview-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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 12

•
a year ago 
Thanks! I've addresses the issues.

Comment 13

•
a year ago 
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

Comment 15

•
a year ago 
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.
 (Assignee)

Comment 16

•
a year ago 
The crash volume is quite low. I would prefer to let it ride the train.
status-firefox54: affected → wontfix
status-firefox-esr52: affected → wontfix
You need to log in before you can comment on or make changes to this bug.