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.

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

(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.

(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

Verifying as fixed on 68.0a1 20190423095327 / Windows 10.