Closed
Bug 1367705
Opened 5 years ago
Closed 5 years ago
Crash in mozilla::MediaChannelStatistics::Stop
Categories
(Core :: Audio/Video: Playback, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla55
People
(Reporter: philipp, Assigned: jwwang)
References
Details
(Keywords: crash)
Crash Data
Attachments
(3 files)
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
Updated•5 years ago
|
Component: Audio/Video → Audio/Video: Playback
Assignee | ||
Comment 2•5 years 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
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•5 years ago
|
Attachment #8873728 -
Flags: review?(cpearce)
Attachment #8873729 -
Flags: review?(cpearce)
Attachment #8873730 -
Flags: review?(cpearce)
Comment 6•5 years 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•5 years 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•5 years 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•5 years ago
|
||
Thanks! I've addresses the issues.
Comment 13•5 years 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
Updated•5 years ago
|
Priority: -- → P1
![]() |
||
Comment 14•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b4002383a249 https://hg.mozilla.org/mozilla-central/rev/1ab72fcadc31 https://hg.mozilla.org/mozilla-central/rev/4037edd64d87
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 15•5 years 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•5 years ago
|
||
The crash volume is quite low. I would prefer to let it ride the train.
Updated•5 years ago
|
Comment hidden (spam) |
You need to log in
before you can comment on or make changes to this bug.
Description
•