Closed Bug 1466606 Opened 6 years ago Closed 6 years ago

Crash in mozilla::Maybe<T>::ref (from TrackBuffersManager::GetNextRandomAccessPoint)

Categories

(Core :: Audio/Video: Playback, defect, P2)

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- fixed
firefox60 --- wontfix
firefox61 --- fixed
firefox62 --- fixed

People

(Reporter: MatsPalmgren_bugz, Assigned: jya)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(2 files, 2 obsolete files)

Bug 1466606 - P1. Calculate the current GetSample index when needed.
59 bytes, text/x-review-board-request
bryce
: review+
RyanVM
: approval-mozilla-beta+
RyanVM
: approval-mozilla-esr60+
Details
Bug 1466606 - P2. Re-use code to determine NextGetSample index.
59 bytes, text/x-review-board-request
bryce
: review+
Details 
This bug was filed from the Socorro interface and is
report bp-709aaefc-f24f-499b-9765-cbcfd0180603.
=============================================================

MOZ_CRASH Reason:  MOZ_RELEASE_ASSERT(mIsSome)


Top 10 frames of crashing thread:

0 xul.dll mozilla::Maybe<JSExnType>::ref mfbt/Maybe.h:565
1 xul.dll mozilla::TrackBuffersManager::GetNextRandomAccessPoint dom/media/mediasource/TrackBuffersManager.cpp:2646
2 xul.dll mozilla::MediaSourceTrackDemuxer::DoGetSamples dom/media/mediasource/MediaSourceDemuxer.cpp:526
3 xul.dll mozilla::detail::ProxyRunnable<mozilla::MozPromise<RefPtr<mozilla::MediaTrackDemuxer::SamplesHolder>, mozilla::MediaResult, 1>, RefPtr<mozilla::MozPromise<RefPtr<mozilla::MediaTrackDemuxer::SamplesHolder>, mozilla::MediaResult, 1> >  xpcom/threads/MozPromise.h:1392
4 xul.dll mozilla::TaskQueue::Runner::Run xpcom/threads/TaskQueue.cpp:243
5 xul.dll nsThreadPool::Run xpcom/threads/nsThreadPool.cpp:229
6 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1088
7 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:519
8 xul.dll mozilla::ipc::MessagePumpForNonMainThreads::Run ipc/glue/MessagePump.cpp:334
9 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:319

=============================================================
Assignee: nobody → jyavenard
See Also: → 1410186
likely happening following bug 1410186.

weird crash...
While this diagnostic assert crash only affects Firefox Nightly 62, the underlying bug (unwrapping an empty Maybe mNextGetSampleIndex) probably affects earlier versions. The code was added in Firefox 41 by MediaSourceDemuxer bug 1174981.
Blocks: 1174981
status-firefox60: --- → ?
status-firefox61: --- → ?
Keywords: leave-open
Comment on attachment 8983368 [details]
Bug 1466606 - P1. Calculate the current GetSample index when needed.

https://reviewboard.mozilla.org/r/249278/#review255436
Attachment #8983368 - Flags: review?(bvandyk) → review+
Depends on: 1467013
Component: Audio/Video → Audio/Video: Playback
I believe the following is happening (from clues in bug 1467013).

MediaSourceTrackDemuxer::Seek is called, this seek to where we want to seek, demux the next frame to get the seek time. That sample is stored in MediaSourceTrackDemuxer::mNextSample and seek completes.

Now new data is added to the source buffer, data that overrides existing content
here: https://searchfox.org/mozilla-central/source/dom/media/mediasource/TrackBuffersManager.cpp#1931

or sourcebuffer.remove() was called:
https://searchfox.org/mozilla-central/source/dom/media/mediasource/TrackBuffersManager.cpp#2049

when we attempt to get the next frame, and the last keyframe was prior the seek, then
https://searchfox.org/mozilla-central/source/dom/media/mediasource/MediaSourceDemuxer.cpp#507

we then attempt to calculate where the next keyframe is, but mNextGetSampleIndex has been reset in between. causing the assert.

When we call GetNextRandomAccessPoint, we must recalculate the next sample index if it's not set, just like we do with TrackBuffersManager::SkipToNextKeyframe
Bug 1467105 proves the above is what's happening.
Depends on: 1467105
Blocks: MSE
Priority: -- → P2
status-firefox-esr60: --- → ?
I wonder if this is the real reason behind bug 1247189
See Also: → 1247189
There are 4 crashes with signature "mozilla::TrackBuffersManager::AssertHasNextSampleIndex".
The moz_crash_reason is "MOZ_RELEASE_ASSERT(trackData.mNextGetSampleIndex.isSome()) (next sample index must be set)".
Crash Signature: [@ mozilla::Maybe<T>::ref] → [@ mozilla::Maybe<T>::ref] [@ mozilla::TrackBuffersManager::AssertHasNextSampleIndex]
Crash Signature: [@ mozilla::Maybe<T>::ref] [@ mozilla::TrackBuffersManager::AssertHasNextSampleIndex] → [@ mozilla::Maybe<T>::ref] [@ mozilla::TrackBuffersManager::AssertHasNextSampleIndex] [@ mozilla::MediaSourceTrackDemuxer::DoGetSamples]
Attachment #8983920 - Attachment is obsolete: true
Attachment #8983920 - Flags: review?(bvandyk)
Attachment #8983921 - Attachment is obsolete: true
Attachment #8983921 - Flags: review?(bvandyk)
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/033f5f2b2582
Revert "Add diagnostic code.". r=bryce
Comment on attachment 8983368 [details]
Bug 1466606 - P1. Calculate the current GetSample index when needed.

Mozreview is getting confused thinking it's always the same 1st patch...
Attachment #8983368 - Flags: review+ → review?(bvandyk)
See Also: → 1467303
Crash Signature: [@ mozilla::Maybe<T>::ref] [@ mozilla::TrackBuffersManager::AssertHasNextSampleIndex] [@ mozilla::MediaSourceTrackDemuxer::DoGetSamples] → [@ mozilla::Maybe<T>::ref] [@ mozilla::TrackBuffersManager::AssertHasNextSampleIndex] [@ mozilla::MediaSourceTrackDemuxer::DoGetSamples] [@ mozilla::TrackBuffersManager::AssertHasNextSampleIndex const]
Comment on attachment 8983368 [details]
Bug 1466606 - P1. Calculate the current GetSample index when needed.

https://reviewboard.mozilla.org/r/249278/#review256172

::: dom/media/mediasource/TrackBuffersManager.h:167
(Diff revision 5)
>                                const media::TimeUnit& aFuzz) const;
> +
> +  // Will set the next GetSample index if needed. This information is determined
> +  // through the value of mNextSampleTimecode. Return false if the index
> +  // couldn't be determined or if there's nothing more that could be demuxed.
> +  // This occurs of either the track buffer doesn't contain the required

of -> if?
Attachment #8983368 - Flags: review?(bvandyk) → review+
Comment on attachment 8983971 [details]
Bug 1466606 - P2. Re-use code to determine NextGetSample index.

https://reviewboard.mozilla.org/r/249824/#review256176

::: dom/media/mediasource/TrackBuffersManager.cpp:2529
(Diff revision 1)
>      aResult = MediaResult(NS_ERROR_OUT_OF_MEMORY, __func__);
>      return nullptr;
>    }
>  
>    // Find the previous keyframe to calculate the evictable amount.
> -  int32_t i = pos;
> +  int32_t i = trackData.mNextGetSampleIndex.ref();

Can we use a uint32_t here?
Attachment #8983971 - Flags: review?(bvandyk) → review+
Comment on attachment 8983971 [details]
Bug 1466606 - P2. Re-use code to determine NextGetSample index.

https://reviewboard.mozilla.org/r/249824/#review256176

> Can we use a uint32_t here?

we could... I think the loop earlier relied on never going past the first element of the array. But we can strongly assert that the first frame in the array must be a keyframe.
See Also: → 1467316
Keywords: leave-open
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1bdee3c3ca7c
P1. Calculate the current GetSample index when needed. r=bryce
https://hg.mozilla.org/integration/autoland/rev/af8ba448f7d3
P2. Re-use code to determine NextGetSample index. r=bryce
https://hg.mozilla.org/mozilla-central/rev/1bdee3c3ca7c
https://hg.mozilla.org/mozilla-central/rev/af8ba448f7d3
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Is this something that can ride the trains or should it be nominated for backport?
Flags: needinfo?(jyavenard)
I will ask to uplift the first patch..

The 2nd is an optimization using a routine added in the first, but I'll prefer to let that one ride the train
Comment on attachment 8983368 [details]
Bug 1466606 - P1. Calculate the current GetSample index when needed.

.

Approval Request Comment
[Feature/Bug causing the regression]: 1174981
[User impact if declined]:crashes,memory corruption. It's very likely this bug is the core reasons for what would appear as unrelated crashes. Those are linked to this bug. 
[Is this code covered by automated tests?]: yes and no. Yes, in that the code is exercised by mochitest, however it was racy. 
[Has the fix been verified in Nightly?]: no more crashes since fix went in
[Needs manual test from QE? If yes, steps to reproduce]: hard to manually reproduce it consistently. 
[List of other uplifts needed for the feature/fix]: request is only for p1, p2 is to ride the train. 
[Is the change risky?]: no. 
[Why is the change risky/not risky?]:We use the same code as used elsewhere to calculate the position in our index. 
[String changes made/needed]:none
Flags: needinfo?(jyavenard)
Attachment #8983368 - Flags: approval-mozilla-esr60?
Attachment #8983368 - Flags: approval-mozilla-beta?
Comment on attachment 8983368 [details]
Bug 1466606 - P1. Calculate the current GetSample index when needed.

Fixes a memory corruption issue causing various crashes. Approved for 61.0b14 and ESR 60.1.
Attachment #8983368 - Flags: approval-mozilla-esr60?
Attachment #8983368 - Flags: approval-mozilla-esr60+
Attachment #8983368 - Flags: approval-mozilla-beta?
Attachment #8983368 - Flags: approval-mozilla-beta+
could this issue have been the cause of firefox parent process leaking memory when facebook/messenger played the new message tone?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: