Bug 1909614 use exact 96-bit integer arithmetic for TimeUnit comparison operators r=padenot

As well as removing 53-bit precision rounding in >= and <= operators, this
resolves a bug where operator== would sometimes return true for very
different TimeUnits.

Differential Revision: https://phabricator.services.mozilla.com/D217673

dom/media/TimeUnits.cpp

@@ -27,6 +27,46 @@ class TimeIntervals;
 
 namespace mozilla {
 
+namespace {
+struct Int96 {
+  bool operator==(const Int96& aOther) const {
+    return high == aOther.high && low == aOther.low;
+  }
+  bool operator>=(const Int96& aOther) const {
+    if (high == aOther.high) {
+      return low >= aOther.low;
+    }
+    return high > aOther.high;
+  }
+  bool operator<=(const Int96& aOther) const {
+    if (high == aOther.high) {
+      return low <= aOther.low;
+    }
+    return high < aOther.high;
+  }
+
+  const int64_t high;
+  const uint32_t low;
+};
+}  // anonymous namespace
+
+static Int96 MultS64xU32(const CheckedInt64& a, int64_t b) {
+  MOZ_ASSERT(b >= 0);
+  MOZ_ASSERT(b <= UINT32_MAX);
+  // Right shift of negative signed integers is implementation-defined until
+  // C++20, but the C++20 two's complement behavior, used by all compilers
+  // even prior to C++20, is assumed here.
+  // (Left shift of negative signed integers would be undefined until C++20).
+  int64_t high = (a.value() >> 32) * b;
+  uint64_t low = a.value() & 0xFFFFFFFF;
+  low *= b;
+  // Move overflow from low multiplication to high.
+  // This will not overflow because we have divided by 2^32 and multiplied
+  // by b, which is less than 2^32.
+  high += AssertedCast<int64_t>(low >> 32);
+  return Int96{high, AssertedCast<uint32_t>(low & 0xFFFFFFFF)};
+};
+
 namespace media {
 
 TimeUnit TimeUnit::FromSeconds(double aValue, int64_t aBase) {
@@ -163,27 +203,9 @@ bool TimeUnit::operator==(const TimeUnit& aOther) const {
       (IsNegInf() && !aOther.IsNegInf())) {
     return false;
   }
-  CheckedInt<int64_t> lhs = mTicks * aOther.mBase;
-  CheckedInt<int64_t> rhs = aOther.mTicks * mBase;
-  if (lhs.isValid() && rhs.isValid()) {
+  Int96 lhs = MultS64xU32(mTicks, aOther.mBase);
+  Int96 rhs = MultS64xU32(aOther.mTicks, mBase);
   return lhs == rhs;
-  }
-  // Reduce the fractions and try again
-  const TimeUnit a = Reduced();
-  const TimeUnit b = aOther.Reduced();
-  lhs = a.mTicks * b.mBase;
-  rhs = b.mTicks * a.mBase;
-
-  if (lhs.isValid() && rhs.isValid()) {
-    return lhs.value() == rhs.value();
-  }
-  // last ditch, convert the reduced fractions to doubles
-  double lhsFloating =
-      static_cast<double>(a.mTicks.value()) * static_cast<double>(a.mBase);
-  double rhsFloating =
-      static_cast<double>(b.mTicks.value()) * static_cast<double>(b.mBase);
-
-  return lhsFloating == rhsFloating;
 }
 bool TimeUnit::operator!=(const TimeUnit& aOther) const {
   MOZ_ASSERT(IsValid() && aOther.IsValid());
@@ -202,22 +224,9 @@ bool TimeUnit::operator>=(const TimeUnit& aOther) const {
       (!IsNegInf() && aOther.IsNegInf())) {
     return true;
   }
-  CheckedInt<int64_t> lhs = mTicks * aOther.mBase;
-  CheckedInt<int64_t> rhs = aOther.mTicks * mBase;
-  if (lhs.isValid() && rhs.isValid()) {
-    return lhs.value() >= rhs.value();
-  }
-  // Reduce the fractions and try again
-  const TimeUnit a = Reduced();
-  const TimeUnit b = aOther.Reduced();
-  lhs = a.mTicks * b.mBase;
-  rhs = b.mTicks * a.mBase;
-
-  if (lhs.isValid() && rhs.isValid()) {
-    return lhs.value() >= rhs.value();
-  }
-  // last ditch, convert the reduced fractions to doubles
-  return ToSeconds() >= aOther.ToSeconds();
+  Int96 lhs = MultS64xU32(mTicks, aOther.mBase);
+  Int96 rhs = MultS64xU32(aOther.mTicks, mBase);
+  return lhs >= rhs;
 }
 bool TimeUnit::operator>(const TimeUnit& aOther) const {
   return !(*this <= aOther);
@@ -235,21 +244,9 @@ bool TimeUnit::operator<=(const TimeUnit& aOther) const {
       (!IsNegInf() && aOther.IsNegInf())) {
     return false;
   }
-  CheckedInt<int64_t> lhs = mTicks * aOther.mBase;
-  CheckedInt<int64_t> rhs = aOther.mTicks * mBase;
-  if (lhs.isValid() && rhs.isValid()) {
-    return lhs.value() <= rhs.value();
-  }
-  // Reduce the fractions and try again
-  const TimeUnit a = Reduced();
-  const TimeUnit b = aOther.Reduced();
-  lhs = a.mTicks * b.mBase;
-  rhs = b.mTicks * a.mBase;
-  if (lhs.isValid() && rhs.isValid()) {
-    return lhs.value() <= rhs.value();
-  }
-  // last ditch, convert the reduced fractions to doubles
-  return ToSeconds() <= aOther.ToSeconds();
+  Int96 lhs = MultS64xU32(mTicks, aOther.mBase);
+  Int96 rhs = MultS64xU32(aOther.mTicks, mBase);
+  return lhs <= rhs;
 }
 bool TimeUnit::operator<(const TimeUnit& aOther) const {
   return !(*this >= aOther);

dom/media/TimeUnits.h

@@ -82,6 +82,8 @@ class TimeUnit final {
   constexpr TimeUnit(CheckedInt64 aTicks, int64_t aBase)
       : mTicks(aTicks), mBase(aBase) {
     MOZ_RELEASE_ASSERT(mBase > 0);
+    // aBase is often from a uint32_t and assumed less than 2^32.
+    MOZ_DIAGNOSTIC_ASSERT(mBase <= UINT32_MAX);
   }
 
   explicit constexpr TimeUnit(CheckedInt64 aTicks)

dom/media/gtest/TestTimeUnit.cpp

@@ -189,6 +189,23 @@ TEST(TimeUnit, Comparisons)
   EXPECT_LT(o, n);
   EXPECT_GE(n, o);
   EXPECT_GT(n, o);
+  // Comparison of very big numbers with different bases
+  TimeUnit p(12312312312312312, 100000000);
+  TimeUnit q(123123123123123119, 1000000000);
+  TimeUnit r(123123123123123120, 1000000000);
+  TimeUnit s(123123123123123121, 1000000000);
+  EXPECT_LE(q, p);
+  EXPECT_LT(q, p);
+  EXPECT_NE(q, p);
+  EXPECT_EQ(p, r);
+  EXPECT_GE(s, p);
+  EXPECT_GT(s, p);
+  EXPECT_NE(s, p);
+  // Comparison of different very big numbers.  There is intentionally only
+  // one factor of 3 in numerator or denominator to reproduce bug 1909614.
+  TimeUnit t = TimeUnit(123123123123124, 100000000 * 3);
+  TimeUnit u = TimeUnit(123123123123124 * 3, 100000000);
+  EXPECT_NE(t, u);
 
   // Values taken from a real website (this is about 53 years, Date.now() in
   // 2023).