Bug 1475769 - Bail out from nsRefreshDriver::Tick before updating mMostRecentRefresh when the refresh driver is waiting for paint. r=mattwoodrow a=lizzard
authorHiroyuki Ikezoe <hikezoe@mozilla.com>
Sun, 15 Jul 2018 20:19:29 +0900
changeset 477996 03de027328011ffb4b48e05b1fdee10dcbcefe4a
parent 477995 74193d58a1e73b22b3f7091afa593cf664c2d5c9
child 477997 ec603dd58a436d5f079da57d9bfdc1cd9037e093
push id9495
push userarchaeopteryx@coole-files.de
push dateTue, 17 Jul 2018 19:47:45 +0000
treeherdermozilla-beta@03de02732801 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmattwoodrow, lizzard
bugs1475769, 1466010
milestone62.0
Bug 1475769 - Bail out from nsRefreshDriver::Tick before updating mMostRecentRefresh when the refresh driver is waiting for paint. r=mattwoodrow a=lizzard Before this patch, there is a race condition that the refresh driver updates the most recent refresh time but animations corresponding to the refresh driver don't update their internal state, that causes the inconsistency that such animations are regarded as finished on the most recent time whereas their internal states represent the animations are still in active. This is the one of the cause of bug 1466010, i.e. the display item corresponding to the animation is going to be rebuilt without calling MarkNeedsDisplayItemRebuild. MozReview-Commit-ID: 9adzDV9E3ka
layout/base/nsRefreshDriver.cpp
layout/base/nsRefreshDriver.h
--- a/layout/base/nsRefreshDriver.cpp
+++ b/layout/base/nsRefreshDriver.cpp
@@ -1144,19 +1144,18 @@ nsRefreshDriver::nsRefreshDriver(nsPresC
     mWarningThreshold(REFRESH_WAIT_WARNING)
 {
   MOZ_ASSERT(NS_IsMainThread());
   MOZ_ASSERT(mPresContext,
              "Need a pres context to tell us to call Disconnect() later "
              "and decrement sRefreshDriverCount.");
   mMostRecentRefreshEpochTime = JS_Now();
   mMostRecentRefresh = TimeStamp::Now();
-  mMostRecentTick = mMostRecentRefresh;
-  mNextThrottledFrameRequestTick = mMostRecentTick;
-  mNextRecomputeVisibilityTick = mMostRecentTick;
+  mNextThrottledFrameRequestTick = mMostRecentRefresh;
+  mNextRecomputeVisibilityTick = mMostRecentRefresh;
 
   ++sRefreshDriverCount;
 }
 
 nsRefreshDriver::~nsRefreshDriver()
 {
   MOZ_ASSERT(NS_IsMainThread());
   MOZ_ASSERT(ObserverCount() == mEarlyRunners.Length(),
@@ -1814,28 +1813,28 @@ nsRefreshDriver::Tick(int64_t aNowEpoch,
   // The underlying assumption is that the refresh driver tick can only
   // go forward in time, not backwards. To prevent the refresh
   // driver from going back in time, just skip this tick and
   // wait until the next tick.
   if ((aNowTime <= mMostRecentRefresh) && !mTestControllingRefreshes) {
     return;
   }
 
-  TimeStamp previousRefresh = mMostRecentRefresh;
-
-  mMostRecentRefresh = aNowTime;
   mMostRecentRefreshEpochTime = aNowEpoch;
 
   if (IsWaitingForPaint(aNowTime)) {
     // We're currently suspended waiting for earlier Tick's to
     // be completed (on the Compositor). Mark that we missed the paint
     // and keep waiting.
     return;
   }
-  mMostRecentTick = aNowTime;
+
+  TimeStamp previousRefresh = mMostRecentRefresh;
+  mMostRecentRefresh = aNowTime;
+
   if (mRootRefresh) {
     mRootRefresh->RemoveRefreshObserver(this, FlushType::Style);
     mRootRefresh = nullptr;
   }
   mSkippedPaints = false;
   mWarningThreshold = 1;
 
   nsCOMPtr<nsIPresShell> presShell = mPresContext->GetPresShell();
@@ -2267,20 +2266,20 @@ nsRefreshDriver::WillRefresh(mozilla::Ti
 bool
 nsRefreshDriver::IsWaitingForPaint(mozilla::TimeStamp aTime)
 {
   if (mTestControllingRefreshes) {
     return false;
   }
 
   if (mWaitingForTransaction) {
-    if (mSkippedPaints && aTime > (mMostRecentTick + TimeDuration::FromMilliseconds(mWarningThreshold * 1000))) {
+    if (mSkippedPaints && aTime > (mMostRecentRefresh + TimeDuration::FromMilliseconds(mWarningThreshold * 1000))) {
       // XXX - Bug 1303369 - too many false positives.
       //gfxCriticalNote << "Refresh driver waiting for the compositor for "
-      //                << (aTime - mMostRecentTick).ToSeconds()
+      //                << (aTime - mMostRecentRefresh).ToSeconds()
       //                << " seconds.";
       mWarningThreshold *= 2;
     }
 
     mSkippedPaints = true;
     return true;
   }
 
--- a/layout/base/nsRefreshDriver.h
+++ b/layout/base/nsRefreshDriver.h
@@ -513,17 +513,16 @@ private:
   bool mNotifyDOMContentFlushed;
 
   int64_t mMostRecentRefreshEpochTime;
   // Number of seconds that the refresh driver is blocked waiting for a compositor
   // transaction to be completed before we append a note to the gfx critical log.
   // The number is doubled every time the threshold is hit.
   uint64_t mWarningThreshold;
   mozilla::TimeStamp mMostRecentRefresh;
-  mozilla::TimeStamp mMostRecentTick;
   mozilla::TimeStamp mTickStart;
   mozilla::TimeStamp mNextThrottledFrameRequestTick;
   mozilla::TimeStamp mNextRecomputeVisibilityTick;
 
   // separate arrays for each flush type we support
   ObserverArray mObservers[4];
   // These observers should NOT be included in HasObservers() since that method
   // is used to determine whether or not to stop the timer, or restore it when