1842496 - Crash in [@ mozilla::CheckedInt<T>::isValid | mozilla::media::TimeUnit::IsValid]

Status: RESOLVED FIXED

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

Description

[Ryan VanderMeulen [:RyanVM]]

Crash report: https://crash-stats.mozilla.org/report/index/b13ab117-4bb3-4145-9e10-7ddfc0230709

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0  xul.dll  mozilla::CheckedInt<long long>::isValid const  mfbt/CheckedInt.h:571
0  xul.dll  mozilla::media::TimeUnit::IsValid const  dom/media/TimeUnits.cpp:359
0  xul.dll  mozilla::MediaData::HasValidTime const  dom/media/MediaData.h:307
0  xul.dll  mozilla::MediaTrackDemuxer::SamplesHolder::AppendSample  dom/media/MediaDataDemuxer.h:104
1  xul.dll  mozilla::MediaSourceTrackDemuxer::DoGetSamples  dom/media/mediasource/MediaSourceDemuxer.cpp:460
2  xul.dll  mozilla::detail::RunnableMethodArguments<StoreCopyPassByRRef<int> >::apply<mozilla::MediaSourceTrackDemuxer, RefPtr<mozilla::MozPromise<RefPtr<mozilla::MediaTrackDemuxer::SamplesHolder>, mozilla::MediaResult, 1> >  const  xpcom/threads/nsThreadUtils.h:1164
2  xul.dll  std::invoke  /builds/worker/fetches/vs/VC/Tools/MSVC/14.29.30133/include/type_traits:1534
2  xul.dll  std::_Apply_impl  /builds/worker/fetches/vs/VC/Tools/MSVC/14.29.30133/include/tuple:974
2  xul.dll  std::apply  /builds/worker/fetches/vs/VC/Tools/MSVC/14.29.30133/include/tuple:979
2  xul.dll  mozilla::detail::RunnableMethodArguments<StoreCopyPassByRRef<int> >::apply  xpcom/threads/nsThreadUtils.h:1162

Comment 1

[Andrew McCreight [:mccr8]]

Here's the set of patches added in the 20230707092800 build where this first started happening.

Comment 2

[Paul Adenot (:padenot)]

This is https://bugzilla.mozilla.org/show_bug.cgi?id=1838828. I'll adjust it.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

The bug is linked to a topcrash signature, which matches the following criteria:

• Top 10 desktop browser crashes on nightly
• Top 10 AArch64 and ARM crashes on nightly

For more information, please visit BugBot documentation.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1838828

Comment 5

[Dianna Smith [:diannaS]]

:padenot any update on this crash? we only have 2 betas left this cycle

Comment 6

[Alastor Wu [:alwu]]

I checked this crash a little bit. I debugged the minidump from this crash report and it crashed on a nullptr sample which was from mNextSample.

That means after seeking, we couldn't really find a sample. By examining mAudioTracks, we can see its mNextGetSampleIndex=0 and mNextSampleTimecode/mNextSampleTime were both empty time unit, which could be from here.

The issue might be similar with this problem 1, so it seems we shouldn't assume mNextSample would always exist? Paul, WDYT?

Comment 7

[Paul Adenot (:padenot)]

We need to turn https://searchfox.org/mozilla-central/source/dom/media/mediasource/MediaSourceDemuxer.cpp#441-457 into https://searchfox.org/mozilla-release/source/dom/media/mediasource/MediaSourceDemuxer.cpp#442-447, otherwise mNextSample can be nullptr.

Comment 8

[Alastor Wu [:alwu]]

Attachment: Bug 1842496 - only set `mNextSample` when the next sample is available.

The next sample seems not always be available after seek so it
doesn't make sense to set a nullptr to mNextSample. But it's still not
clear to me why we won't be able to get a sample if the seek time is
already in the buffered range [1].

We should be able to solve the crash by not storing the nullptr in
mNexSample which would ensure we always call mManager->GetSample to
check whether thereis available sample.

[1] https://searchfox.org/mozilla-central/rev/4537bb2abc8eb49506bbad27b89ab364ac3062d3/dom/media/mediasource/MediaSourceDemuxer.cpp#380-392

Comment 9

[Alastor Wu [:alwu]]

Add NI for uplifting.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c28c9108aae1
only set `mNextSample` when the next sample is available. r=media-playback-reviewers,padenot

Comment 11

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/c28c9108aae1

Comment 12

[Alastor Wu [:alwu]]

Comment on attachment 9344740 [details]
Bug 1842496 - only set mNextSample when the next sample is available.

Beta/Release Uplift Approval Request

• User impact if declined: crash
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch is about adding a check to avoid using nullptr sample, and it doesn't add new functionality/feature or structural change.
• String changes made/needed:
• Is Android affected?: Unknown

Comment 13

[Dianna Smith [:diannaS]]

Comment on attachment 9344740 [details]
Bug 1842496 - only set mNextSample when the next sample is available.

Approved for 116.0rc1

Comment 14

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/c0991abb13d4