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

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

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.

Comment 1

[Chris Peterson [:cpeterson]]

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

Comment 2

[Alastor Wu [:alwu]]

Attachment: Bug 1540740 - part1 : let the return value of 'AdjustForStartTime()' to tell whether the adjustment is succeeded

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.

Comment 3

[Alastor Wu [:alwu]]

Attachment: Bug 1540740 - part2 : let AdjustForStartTime() return whether mTime is valid.

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.

Comment 4

[Alastor Wu [:alwu]]

Attachment: Bug 1540740 - part2 : using TimeUnit as input parameter for 'AdjustForStartTime()'.

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
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

Comment 6

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/ffb3ec9486e6
https://hg.mozilla.org/mozilla-central/rev/0b26cd4c1c21

Comment 7

[Chris Peterson [:cpeterson]]

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?

Comment 8

[Alastor Wu [: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.

Comment 9

[Chris Peterson [:cpeterson]]

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. :)