Closed Bug 1542685 Opened 8 months ago Closed 8 months ago

Crash in [@ mozilla::DriftCompensator::GetVideoTime]

Categories

(Core :: Audio/Video: Recording, defect, P2, critical)

68 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla68
Tracking Status
geckoview66 --- unaffected
firefox-esr60 --- unaffected
firefox66 --- unaffected
firefox67 --- unaffected
firefox68 --- verified

People

(Reporter: pehrsons, Assigned: pehrsons)

References

(Regression)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

This bug is for crash report bp-95ba0a07-c2d7-460a-93a5-9064a0190408.

Top 10 frames of crashing thread:

0 xul.dll class mozilla::TimeStamp mozilla::DriftCompensator::GetVideoTime dom/media/DriftCompensation.h
1 xul.dll void mozilla::VideoTrackEncoder::AdvanceCurrentTime dom/media/encoder/TrackEncoder.cpp:695
2 xul.dll nsresult mozilla::detail::RunnableMethodImpl<RefPtr<mozilla::AudioTrackEncoder>, void  xpcom/threads/nsThreadUtils.h:1174
3 xul.dll nsresult mozilla::TaskQueue::Runner::Run xpcom/threads/TaskQueue.cpp:199
4 xul.dll nsresult nsThreadPool::Run xpcom/threads/nsThreadPool.cpp:244
5 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1180
6 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:486
7 xul.dll mozilla::ipc::MessagePumpForNonMainThreads::Run ipc/glue/MessagePump.cpp:333
8 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:308
9 xul.dll static void nsThread::ThreadFunc xpcom/threads/nsThread.cpp:454

QA got this crash trying to verify bug 1414277.

It's a MOZ_RELEASE_ASSERT(mIsValid) (Invalid checked integer (division by zero or integer overflow)) after only 50 minutes of recording (or, well, process uptime is ~4h but the amount of recorded data in the temp folder is 50min).

CheckedInt64 SaferMultDiv(int64_t aValue, uint64_t aMul, uint64_t aDiv) {
  if (aMul > INT64_MAX || aDiv > INT64_MAX) {
    return CheckedInt64(INT64_MAX) + 1;  // Return an invalid checked int.
  }
  int64_t mul = aMul;
  int64_t div = aDiv;
  int64_t major = aValue / div;
  int64_t remainder = aValue % div;
  return CheckedInt64(remainder) * mul / div + CheckedInt64(major) * mul;
}

If aDiv here is larger than aValue (which happens if the frame we're calculating the time for is in the past, which is likely), we end up multiplying aValue with aMul. These two are roughly the same, one is the time for the frame (slightly in the past), the other is the current audio time. Both in microseconds.

When this happens we crash after sqrt(2^63-1) microseconds, or 50.6 minutes.

One way to solve this would be to operate on milliseconds instead of microseconds. That let's us record for 35 days instead of 50 minutes. We'd also lose some accuracy, but it's probably not major.

There's already some jitter in when we TimeStamp::Now(). It could happen at any time during an audio callback, and we assume that those wall-clock and audio-clock times equal each other. The time between audio callbacks varies a bit, but is generally a couple of milliseconds long, so this jitter is already in the size of milliseconds.

We took some measurements on accuracy of using doubles to do the math instead, and it's pretty good. No inaccuracy detected for a worst-case of a 100-day recording, even. We'll take that.

SaferMultDiv(time, audioScale, videoScale) could easily result in overflow
because all three args are roughly equal, and SaferMultDiv would always do the
multiplication first. The worst-case is then multiplying an int64_t to another
int64_t that have very similar values. Since we represent time here in
microseconds, this would overflow after only 50 minutes.

Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b8855a45d798
Avoid integer overflows by multiplying doubles. r=padenot
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

(In reply to Andreas Pehrson [:pehrsons] from comment #4)

Created attachment 9056517 [details]
Bug 1542685 - Avoid integer overflows by multiplying doubles. r?padenot

SaferMultDiv(time, audioScale, videoScale) could easily result in overflow
because all three args are roughly equal, and SaferMultDiv would always do the
multiplication first. The worst-case is then multiplying an int64_t to another
int64_t that have very similar values. Since we represent time here in
microseconds, this would overflow after only 50 minutes.

Not it cannot. And it doesnt' do the multiplication first, that's precisely the problem it's designed to avoid

SaferMultDiv can only overflow if the end result itself doesn't fit on 64 bits.

Using double only means you lose accuracy as the mantissa is only 52 bits.

Flags: needinfo?(apehrson)

(In reply to Jean-Yves Avenard [:jya] from comment #7)

(In reply to Andreas Pehrson [:pehrsons] from comment #4)

Created attachment 9056517 [details]
Bug 1542685 - Avoid integer overflows by multiplying doubles. r?padenot

SaferMultDiv(time, audioScale, videoScale) could easily result in overflow
because all three args are roughly equal, and SaferMultDiv would always do the
multiplication first. The worst-case is then multiplying an int64_t to another
int64_t that have very similar values. Since we represent time here in
microseconds, this would overflow after only 50 minutes.

Not it cannot. And it doesnt' do the multiplication first, that's precisely the problem it's designed to avoid

SaferMultDiv can only overflow if the end result itself doesn't fit on 64 bits.

Using double only means you lose accuracy as the mantissa is only 52 bits.

See [1], aka return CheckedInt64(remainder) * mul / div + CheckedInt64(major) * mul;

CheckedInt64(remainder) * mul happens before the division with div. If div is larger than either remainder or mul, swapping doesn't help, and remainder ends up equal to aValue. The multiplication above then overflows and the division that later happens cannot make the CheckedInt64 valid again, even if it would have brought the result into valid int64-range.

An obvious example proving you wrong is SaferMultDiv(2^63-2, 2^63-2, 2^63-1). Expanded this becomes return CheckedInt64(2^63-2) * (2^63-2) / (2^63-1) + CheckedInt64(0) * (2^63-2);

[1] https://searchfox.org/mozilla-central/rev/dd7e27f4a805e4115d0dbee70e1220b23b23c567/dom/media/VideoUtils.cpp#48

Flags: needinfo?(apehrson)

Verifying as fixed on 68.0a1 20190423095327 / Windows 10.

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.