Bug 1484559 - Ensure that the scroll frame deregister its refresh driver observers (mAsyncScroll & mAsyncSmoothMSDScroll) before it's destroyed. r=dholbert
authorMats Palmgren <mats@mozilla.com>
Tue, 21 Aug 2018 14:55:22 +0200
changeset 432573 6d8d234a983ae65eee63239a8536dd24428f6db1
parent 432572 dcbb7762dc2fa3afbbf969016c29b125d97b52f2
child 432574 95b2521e281c5b37ad2acd1023cc798355c553b1
child 432608 130bb81b5f4f9c98ba138ab5593148537dd77b57
push id106785
push usermpalmgren@mozilla.com
push dateTue, 21 Aug 2018 12:55:41 +0000
treeherdermozilla-inbound@6d8d234a983a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdholbert
bugs1484559
milestone63.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 1484559 - Ensure that the scroll frame deregister its refresh driver observers (mAsyncScroll & mAsyncSmoothMSDScroll) before it's destroyed. r=dholbert
layout/generic/nsGfxScrollFrame.cpp
--- a/layout/generic/nsGfxScrollFrame.cpp
+++ b/layout/generic/nsGfxScrollFrame.cpp
@@ -1848,39 +1848,40 @@ public:
     if (!RefreshDriver(aCallee)->AddRefreshObserver(this, FlushType::Style)) {
       return false;
     }
 
     mCallee = aCallee;
     return true;
   }
 
+  /**
+   * The mCallee holds a strong ref to us since the refresh driver doesn't.
+   * Our dtor and mCallee's Destroy() method both call RemoveObserver() -
+   * whichever comes first removes us from the refresh driver.
+   */
+  void RemoveObserver() {
+    if (mCallee) {
+      RefreshDriver(mCallee)->RemoveRefreshObserver(this, FlushType::Style);
+      mCallee = nullptr;
+    }
+  }
+
 private:
   // Private destructor, to discourage deletion outside of Release():
   ~AsyncSmoothMSDScroll() {
     RemoveObserver();
     Telemetry::SetHistogramRecordingEnabled(
       Telemetry::FX_REFRESH_DRIVER_SYNC_SCROLL_FRAME_DELAY_MS, false);
   }
 
   nsRefreshDriver* RefreshDriver(ScrollFrameHelper* aCallee) {
     return aCallee->mOuter->PresContext()->RefreshDriver();
   }
 
-  /*
-   * The refresh driver doesn't hold a reference to its observers,
-   *   so releasing this object can (and is) used to remove the observer on DTOR.
-   * Currently, this object is released once the scrolling ends.
-   */
-  void RemoveObserver() {
-    if (mCallee) {
-      RefreshDriver(mCallee)->RemoveRefreshObserver(this, FlushType::Style);
-    }
-  }
-
   mozilla::layers::AxisPhysicsMSDModel mXAxisModel, mYAxisModel;
   nsRect mRange;
   mozilla::TimeStamp mLastRefreshTime;
   ScrollFrameHelper *mCallee;
   nscoord mOneDevicePixelInAppUnits;
 };
 
 // AsyncScroll has ref counting.
@@ -1969,36 +1970,36 @@ public:
   }
 
   virtual void WillRefresh(mozilla::TimeStamp aTime) override {
     // The callback may release "this".
     // We don't access members after returning, so no need for KungFuDeathGrip.
     ScrollFrameHelper::AsyncScrollCallback(mCallee, aTime);
   }
 
-private:
-  ScrollFrameHelper *mCallee;
-
-  nsRefreshDriver* RefreshDriver(ScrollFrameHelper* aCallee) {
-    return aCallee->mOuter->PresContext()->RefreshDriver();
-  }
-
-  /*
-   * The refresh driver doesn't hold a reference to its observers,
-   *   so releasing this object can (and is) used to remove the observer on DTOR.
-   * Currently, this object is released once the scrolling ends.
+  /**
+   * The mCallee holds a strong ref to us since the refresh driver doesn't.
+   * Our dtor and mCallee's Destroy() method both call RemoveObserver() -
+   * whichever comes first removes us from the refresh driver.
    */
   void RemoveObserver() {
     if (mCallee) {
       RefreshDriver(mCallee)->RemoveRefreshObserver(this, FlushType::Style);
       if (nsIPresShell* shell = mCallee->mOuter->PresShell()) {
         shell->SuppressDisplayport(false);
       }
+      mCallee = nullptr;
     }
   }
+private:
+  ScrollFrameHelper *mCallee;
+
+  nsRefreshDriver* RefreshDriver(ScrollFrameHelper* aCallee) {
+    return aCallee->mOuter->PresContext()->RefreshDriver();
+  }
 };
 
 /*
  * Calculate duration, possibly dynamically according to events rate and event origin.
  * (also maintain previous timestamps - which are only used here).
  */
 static ScrollAnimationBezierPhysicsSettings
 ComputeBezierAnimationSettingsForOrigin(nsAtom *aOrigin)
@@ -4886,16 +4887,22 @@ ScrollFrameHelper::Destroy(PostDestroyDa
     delete gScrollFrameActivityTracker;
     gScrollFrameActivityTracker = nullptr;
   }
 
   if (mScrollActivityTimer) {
     mScrollActivityTimer->Cancel();
     mScrollActivityTimer = nullptr;
   }
+  if (mAsyncScroll) {
+    mAsyncScroll->RemoveObserver();
+  }
+  if (mAsyncSmoothMSDScroll) {
+    mAsyncSmoothMSDScroll->RemoveObserver();
+  }
 }
 
 /**
  * Called when we want to update the scrollbar position, either because scrolling happened
  * or the user moved the scrollbar position and we need to undo that (e.g., when the user
  * clicks to scroll and we're using smooth scrolling, so we need to put the thumb back
  * to its initial position for the start of the smooth sequence).
  */