Bug 1626734 - Fix edge case that can cause TimeStamps to go backwards by fractional milliseconds. r=karlt
authorKartikaya Gupta <kgupta@mozilla.com>
Thu, 07 May 2020 12:56:09 +0000
changeset 528610 1bc958f2959b724ddcf89587e75acb2f4d62fcf6
parent 528609 98c02385b0fdf3640bc0c4bd2db95ff063cadb2f
child 528611 f4dbf7b285120838398897f9ea5cc4ee4b7829a3
push id37392
push userapavel@mozilla.com
push dateThu, 07 May 2020 21:43:47 +0000
treeherdermozilla-central@f9356df325cd [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskarlt
bugs1626734
milestone78.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1626734 - Fix edge case that can cause TimeStamps to go backwards by fractional milliseconds. r=karlt Differential Revision: https://phabricator.services.mozilla.com/D72954
widget/SystemTimeConverter.h
widget/tests/gtest/TestTimeConverter.cpp
--- a/widget/SystemTimeConverter.h
+++ b/widget/SystemTimeConverter.h
@@ -68,61 +68,62 @@ class SystemTimeConverter {
     // If (ii) - (i) is negative then the source of Time values is getting
     // "ahead" of TimeStamp. We call this "forwards" skew below.
     //
     // For the reverse case, if (ii) - (i) is positive (and greater than some
     // tolerance factor), then we may have "backwards" skew. This is often
     // the case when we have a backlog of events and by the time we process
     // them, the time given by the system is comparatively "old".
     //
-    // We call the absolute difference between (i) and (ii), "deltaFromNow".
+    // The IsNewerThanTimestamp function computes the equivalent of |aTime| in
+    // the TimeStamp scale and returns that in |timeAsTimeStamp|.
     //
     // Graphically:
     //
     //                    mReferenceTime              aTime
     // Time scale:      ........+.......................*........
     //                          |--------timeDelta------|
     //
     //                  mReferenceTimeStamp             roughlyNow
     // TimeStamp scale: ........+...........................*....
     //                          |------timeStampDelta-------|
     //
     //                                                  |---|
-    //                                               deltaFromNow
+    //                                         roughlyNow-timeAsTimeStamp
     //
-    Time deltaFromNow;
-    bool newer = IsTimeNewerThanTimestamp(aTime, roughlyNow, &deltaFromNow);
+    TimeStamp timeAsTimeStamp;
+    bool newer = IsTimeNewerThanTimestamp(aTime, roughlyNow, &timeAsTimeStamp);
 
     // Tolerance when detecting clock skew.
-    static const Time kTolerance = 30;
+    static const TimeDuration kTolerance = TimeDuration::FromMilliseconds(30.0);
 
     // Check for forwards skew
     if (newer) {
       // Make aTime correspond to roughlyNow
       UpdateReferenceTime(aTime, roughlyNow);
 
       // We didn't have backwards skew so don't bother checking for
       // backwards skew again for a little while.
       mLastBackwardsSkewCheck = aTime;
 
       return roughlyNow;
     }
 
-    if (deltaFromNow <= kTolerance) {
+    if (roughlyNow - timeAsTimeStamp <= kTolerance) {
       // If the time between event times and TimeStamp values is within
       // the tolerance then assume we don't have clock skew so we can
       // avoid checking for backwards skew for a while.
       mLastBackwardsSkewCheck = aTime;
     } else if (aTime - mLastBackwardsSkewCheck > kBackwardsSkewCheckInterval) {
       aCurrentTimeGetter.GetTimeAsyncForPossibleBackwardsSkew(roughlyNow);
       mLastBackwardsSkewCheck = aTime;
     }
 
     // Finally, calculate the timestamp
-    return roughlyNow - TimeDuration::FromMilliseconds(deltaFromNow);
+    return timeAsTimeStamp;
   }
 
   void CompensateForBackwardsSkew(Time aReferenceTime,
                                   const TimeStamp& aLowerBound) {
     // Check if we actually have backwards skew. Backwards skew looks like
     // the following:
     //
     //        mReferenceTime
@@ -150,18 +151,17 @@ class SystemTimeConverter {
     //                                ^          ^
     //                          aLowerBound  TimeStamp::Now()
     //
     // If the duration (aLowerBound - mReferenceTimeStamp) is greater than
     // (aReferenceTime - mReferenceTime) then we know we have backwards skew.
     //
     // If that's not the case, then we probably just got caught behind
     // temporarily.
-    Time delta;
-    if (IsTimeNewerThanTimestamp(aReferenceTime, aLowerBound, &delta)) {
+    if (IsTimeNewerThanTimestamp(aReferenceTime, aLowerBound, nullptr)) {
       return;
     }
 
     // We have backwards skew; the equivalent TimeStamp for aReferenceTime lies
     // somewhere between aLowerBound (which was the TimeStamp when we triggered
     // the async request for the current time) and TimeStamp::Now().
     //
     // If aReferenceTime was waiting in the event queue for a long time, the
@@ -185,37 +185,42 @@ class SystemTimeConverter {
 
   void UpdateReferenceTime(Time aReferenceTime,
                            const TimeStamp& aReferenceTimeStamp) {
     mReferenceTime = aReferenceTime;
     mReferenceTimeStamp = aReferenceTimeStamp;
   }
 
   bool IsTimeNewerThanTimestamp(Time aTime, TimeStamp aTimeStamp,
-                                Time* aDelta) {
+                                TimeStamp* aTimeAsTimeStamp) {
     Time timeDelta = aTime - mReferenceTime;
 
     // Cast the result to signed 64-bit integer first since that should be
     // enough to hold the range of values returned by ToMilliseconds() and
     // the result of converting from double to an integer-type when the value
     // is outside the integer range is undefined.
     // Then we do an implicit cast to Time (typically an unsigned 32-bit
     // integer) which wraps times outside that range.
-    Time timeStampDelta = static_cast<int64_t>(
-        (aTimeStamp - mReferenceTimeStamp).ToMilliseconds());
+    TimeDuration timeStampDelta = (aTimeStamp - mReferenceTimeStamp);
+    int64_t wholeMillis = static_cast<int64_t>(timeStampDelta.ToMilliseconds());
+    Time wrappedTimeStampDelta = wholeMillis;  // truncate to unsigned
 
-    Time timeToTimeStamp = timeStampDelta - timeDelta;
+    Time timeToTimeStamp = wrappedTimeStampDelta - timeDelta;
     bool isNewer = false;
     if (timeToTimeStamp == 0) {
-      *aDelta = 0;
+      // wholeMillis needs no adjustment
     } else if (timeToTimeStamp < kTimeHalfRange) {
-      *aDelta = timeToTimeStamp;
+      wholeMillis -= timeToTimeStamp;
     } else {
       isNewer = true;
-      *aDelta = timeDelta - timeStampDelta;
+      wholeMillis += (-timeToTimeStamp);
+    }
+    if (aTimeAsTimeStamp) {
+      *aTimeAsTimeStamp =
+          mReferenceTimeStamp + TimeDuration::FromMilliseconds(wholeMillis);
     }
 
     return isNewer;
   }
 
   Time mReferenceTime;
   TimeStamp mReferenceTimeStamp;
   Time mLastBackwardsSkewCheck;
--- a/widget/tests/gtest/TestTimeConverter.cpp
+++ b/widget/tests/gtest/TestTimeConverter.cpp
@@ -229,8 +229,37 @@ TEST(TimeConverter, HalfRangeBoundary)
 
   // The above forwards skew will have reset the reference timestamp. Now
   // advance Now time by just under the half-range, to trigger about as big
   // a backwards skew as we possibly can.
   MockTimeStamp::Advance(halfRange - 1);
   ts = converter.GetTimeStampFromSystemTime(secondEvent, unused);
   EXPECT_TS(ts, 0);
 }
+
+TEST(TimeConverter, FractionalMillisBug1626734)
+{
+  MockTimeStamp::Init();
+
+  TimeConverter converter;
+
+  GTestTime eventTime = 10;
+  MockCurrentTimeGetter timeGetter(eventTime);
+  UnusedCurrentTimeGetter<GTestTime> unused;
+
+  TimeStamp ts = converter.GetTimeStampFromSystemTime(eventTime, timeGetter);
+  EXPECT_TS(ts, 0);
+
+  MockTimeStamp::Advance(0.2);
+  ts = converter.GetTimeStampFromSystemTime(eventTime, unused);
+  EXPECT_TS(ts, 0);
+
+  MockTimeStamp::Advance(0.9);
+  TimeStamp ts2 = converter.GetTimeStampFromSystemTime(eventTime, unused);
+  EXPECT_TS(ts2, 0);
+
+  // Since ts2 came from a "future" call relative to ts, we expect ts2 to not
+  // be "before" ts. (i.e. time shouldn't go backwards, even by fractional
+  // milliseconds). This assertion is technically already implied by the
+  // EXPECT_TS checks above, but fixing this assertion is the end result that
+  // we wanted in bug 1626734 so it feels appropriate to recheck it explicitly.
+  EXPECT_TRUE(ts <= ts2);
+}