Closed Bug 1540740 Opened 7 months ago Closed 6 months ago

Crash in [@ mozilla::CheckedInt<T>::value] via MediaData::AdjustForStartTime

Categories

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

Unspecified
Android
defect

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox-esr60 --- unaffected
firefox66 --- wontfix
firefox67 --- wontfix
firefox68 --- fixed

People

(Reporter: mccr8, Assigned: alwu)

References

Details

(Keywords: crash, regression, Whiteboard: [geckoview:fenix:m4][gvtv:p1][media-q2])

Crash Data

Attachments

(2 files, 1 obsolete file)

This bug is for crash report bp-cb656a71-c939-417d-9e2e-1c2ab0190401.

Top 10 frames of crashing thread:

0 libxul.so mozilla::CheckedInt<long long>::value const mfbt/CheckedInt.h:535
1 libxul.so mozilla::media::TimeUnit::IsNegative const dom/media/TimeUnits.h:107
2 libxul.so mozilla::MediaData::AdjustForStartTime dom/media/MediaData.h:301
3 libxul.so mozilla::AudioData::AdjustForStartTime dom/media/MediaData.cpp:66
4 libxul.so mozilla::ReaderProxy::OnAudioDataRequestCompleted dom/media/ReaderProxy.cpp:46
5 libxul.so mozilla::MozPromise<RefPtr<mozilla::AudioData>, mozilla::MediaResult, true>::ThenValue<mozilla::ReaderProxy*, RefPtr<mozilla::MozPromise<RefPtr<mozilla::AudioData>, mozilla::MediaResult, true> >  xpcom/threads/MozPromise.h:502
6 libxul.so mozilla::MozPromise<RefPtr<mozilla::AudioData>, mozilla::MediaResult, true>::ThenValueBase::ResolveOrRejectRunnable::Run 
7 libxul.so mozilla::AutoTaskDispatcher::TaskGroupRunnable::Run xpcom/threads/TaskDispatcher.h:197
8 libxul.so mozilla::TaskQueue::Runner::Run xpcom/threads/TaskQueue.cpp:199
9 libxul.so nsThreadPool::Run xpcom/threads/nsThreadPool.cpp:244

Alex made integer overflow checks into release asserts, and this is one of the crashes. It is Android-only.

Summary: Crash in [@ mozilla::CheckedInt<T>::value] called in TimeUnit::IsNegative → Crash in [@ mozilla::CheckedInt<T>::value] via MediaData::AdjustForStartTime
Rank: 9
Priority: -- → P1
Whiteboard: [geckoview:fenix:m4][gvtv:p1]

This CheckedInt assertion failure is a top crash for Fennec 68 Nightly.

Whiteboard: [geckoview:fenix:m4][gvtv:p1] → [geckoview:fenix:m4][gvtv:p1][media-q2]
Depends on: 1540748

In order to prevent the integer overflow due to the super large start time value, we should check whether the result is valid after the calculation.

No one is using current return value after calling AdjustForStartTime(), but we would check whether mTime is valid instead.

Therefore, we should let AdjustForStartTime() return whether mTime is valid in order to reduce the additional check.

There is no need to cast to TimeUnit to int64 and then cast to TimeUnit again.

Assignee: nobody → alwu
Attachment #9057001 - Attachment is obsolete: true
Attachment #9057002 - Attachment description: Bug 1540740 - part3 : using TimeUnit as input parameter for 'AdjustForStartTime()'. → Bug 1540740 - part2 : using TimeUnit as input parameter for 'AdjustForStartTime()'.
Attachment #9056999 - Attachment description: Bug 1540740 - part1 : ensure adjusted time won't be overflow. → Bug 1540740 - part1 : let the return value of 'AdjustForStartTime()' to tell whether the adjustment is succeeded
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ffb3ec9486e6
part1 : let the return value of 'AdjustForStartTime()' to tell whether the adjustment is succeeded r=jya
https://hg.mozilla.org/integration/autoland/rev/0b26cd4c1c21
part2 : using TimeUnit as input parameter for 'AdjustForStartTime()'. r=jya
Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

Alastor, should we uplift your AdjustForStartTime fix to 67 Beta? I know CheckedInt's MOZ_RELEASE_ASSERT check is not in 67 Beta, but is the AdjustForStartTime problem a real bug that is relevant to 67 Beta?

Flags: needinfo?(alwu)

As there is no strong evidence showing that this issue (improper start time) would occur in 67 as well (the issue might be caused by other patches in 68), so I think we don't need to uplift those patches.

Flags: needinfo?(alwu)

there is no strong evidence showing that this issue (improper start time) would occur in 67

That's good news. In that case, I will set 67=wontfix. :)

You need to log in before you can comment on or make changes to this bug.