Open Bug 1605699 Opened 9 months ago Updated 2 months ago

Crash in [@ mozilla::MediaData::HasValidTime]

Categories

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

All
Unspecified
defect

Tracking

()

ASSIGNED

People

(Reporter: gsvelto, Assigned: alwu, NeedInfo)

References

(Regression)

Details

(Keywords: crash, leave-open, regression)

Crash Data

Attachments

(2 files)

This bug is for crash report bp-1b12b057-3471-4449-a789-479100191222.

Top 10 frames of crashing thread:

0 XUL mozilla::MediaData::HasValidTime const mfbt/CheckedInt.h:560
1 XUL mozilla::MediaTrackDemuxer::SamplesHolder::AppendSample dom/media/MediaDataDemuxer.h:104
2 XUL mozilla::MediaSourceTrackDemuxer::DoGetSamples dom/media/mediasource/MediaSourceDemuxer.cpp:440
3 XUL mozilla::detail::ProxyRunnable<mozilla::MozPromise<RefPtr<mozilla::MediaTrackDemuxer::SamplesHolder>, mozilla::MediaResult, true>, RefPtr<mozilla::MozPromise<RefPtr<mozilla::MediaTrackDemuxer::SamplesHolder>, mozilla::MediaResult, true> >  xpcom/threads/MozPromise.h:1343
4 XUL mozilla::TaskQueue::Runner::Run xpcom/threads/TaskQueue.cpp:207
5 XUL nsThreadPool::Run xpcom/threads/nsThreadPool.cpp:300
6 XUL non-virtual thunk to nsThreadPool::Run xpcom/threads/nsThreadPool.cpp
7 XUL nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1241
8 XUL <name omitted> xpcom/threads/nsThreadUtils.cpp:486
9 XUL mozilla::ipc::MessagePumpForNonMainThreads::Run ipc/glue/MessagePump.cpp:332

This looks like a NULL pointer dereference, low volume but happening on multiple platforms.

It seems that all crashes here are in MediaSourceTrackDemuxer.

Alastor, any guess? Does HasValidTime() need to be checked before AppendSample in MediaSourceTrackDemuxer is called, like what FlacDemuxer does ?

Flags: needinfo?(alwu)
Regressed by: 1563949

This invalid issue is an old issue, which we spent time before but couldn't find any cues. We won't need to call HasValidTime() in every sample call, because we do have a check in [1] to ensure that returned sample time is valid. But don't know why we could still see this kind of invalid time...

[1] https://searchfox.org/mozilla-central/rev/723e61e5f6fe36e69592b7742bc10338392e53e4/dom/media/mediasource/TrackBuffersManager.cpp#2619-2622


Hi, Bryce,
I remember that we have traced a similar bug before, but I couldn't find the bug number. Do you remember that?

Flags: needinfo?(alwu) → needinfo?(bvandyk)

Considering this crash doesn't have high happen rate, I would mark this one as P3.

Priority: -- → P3

Since it's happening only occasionally and it shouldn't be happening at all during regular execution, could this be a narrow race?

The sample we got from demuxer are all guaranteed to be valid because they are returned in a SamplesHolder, which has assertion to make sure all samples appended are valid [1].

ProcessFrame() might be a possible place where we incorrectly change sample from valid to invalid, because we modify sample's time and duration there. Therefore, adding an assertion to make sure all samples are still valid.

[1] https://searchfox.org/mozilla-central/source/dom/media/MediaDataDemuxer.h#103-106

Assignee: nobody → alwu
Status: NEW → ASSIGNED
QA Whiteboard: crash, regression,
Keywords: leave-open
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6057d8008988
part1 : add an assertion to ensure 'GetSample()' always return a valid sample. r=bryce
https://hg.mozilla.org/integration/autoland/rev/cc25ed1a4d21
part2 : add an assertion to ensure we only append a valid sample. r=bryce

(In reply to Alastor Wu [:alwu] from comment #2)

Hi, Bryce,
I remember that we have traced a similar bug before, but I couldn't find the bug number. Do you remember that?

Is it bug 1545108? It's similar in a "there shouldn't be invalid data at this point" sense.

Flags: needinfo?(bvandyk)

Comment on attachment 9118077 [details]
Bug 1605699 - part1 : add an assertion to ensure 'GetSample()' always return a valid sample.

Beta/Release Uplift Approval Request

  • User impact if declined: Nothing happen, the purpose of these patches is to add more assertions to help us diagnose the reason of the 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): Adding assertion to ensure the sample time is valid, which should be a correct assumption all the time.
  • String changes made/needed:
Attachment #9118077 - Flags: approval-mozilla-beta?
Attachment #9118081 - Flags: approval-mozilla-beta?

Comment on attachment 9118077 [details]
Bug 1605699 - part1 : add an assertion to ensure 'GetSample()' always return a valid sample.

Adds crash diagnostics. Approved for 73.0b6.

Attachment #9118077 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9118081 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment on attachment 9118077 [details]
Bug 1605699 - part1 : add an assertion to ensure 'GetSample()' always return a valid sample.

These actually landed on m-c when it was still tracking 73, so Beta already has these patches.

Attachment #9118077 - Flags: approval-mozilla-beta+
Attachment #9118081 - Flags: approval-mozilla-beta+

Ah, sorry, I didn't notice that. Do you know which beta version starts including these patches?
Thank you.

Flags: needinfo?(ryanvm)

Should have been there from 73.0b1 and on.

Flags: needinfo?(ryanvm)

Thank you, so apparently the patches I added didn't prevent the crash. This needs more investigation...

The leave-open keyword is there and there is no activity for 6 months.
:alwu, maybe it's time to close this bug?

Flags: needinfo?(alwu)
You need to log in before you can comment on or make changes to this bug.