Bug 951113 - Trigger a repaint request when getting a scroll offset update to cover a race condition allowed by async IPC. r=botond, a=1.3+
authorKartikaya Gupta <kgupta@mozilla.com>
Tue, 14 Jan 2014 16:43:43 -0500
changeset 175819 b3b07adaf7cd1ebea04cf76a04bdffa779632da9
parent 175818 e17047520e4ce0e7c33e75982578dc99981b7b1c
child 175820 9a006d9a6e1df1aa3c0e9cbed07346bdcd7e8d6f
push id445
push userffxbld
push dateMon, 10 Mar 2014 22:05:19 +0000
treeherdermozilla-release@dc38b741b04e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbotond, 1
bugs951113
milestone28.0a2
Bug 951113 - Trigger a repaint request when getting a scroll offset update to cover a race condition allowed by async IPC. r=botond, a=1.3+
gfx/layers/ipc/AsyncPanZoomController.cpp
widget/xpwidgets/APZCCallbackHelper.cpp
--- a/gfx/layers/ipc/AsyncPanZoomController.cpp
+++ b/gfx/layers/ipc/AsyncPanZoomController.cpp
@@ -1423,16 +1423,27 @@ void AsyncPanZoomController::NotifyLayer
     // If the layers update was not triggered by our own repaint request, then
     // we want to take the new scroll offset.
     if (aLayerMetrics.mUpdateScrollOffset) {
       APZC_LOG("Updating scroll offset from (%f, %f) to (%f, %f)\n",
         mFrameMetrics.mScrollOffset.x, mFrameMetrics.mScrollOffset.y,
         aLayerMetrics.mScrollOffset.x, aLayerMetrics.mScrollOffset.y);
 
       mFrameMetrics.mScrollOffset = aLayerMetrics.mScrollOffset;
+
+      // It is possible that when we receive this mUpdateScrollOffset flag, we have
+      // just sent a content repaint request, and it is pending inflight. That repaint
+      // request would have our old scroll offset, and will get processed on the content
+      // thread as we're processing this mUpdateScrollOffset flag. This would leave
+      // things in a state where content has the old APZC scroll offset and the APZC
+      // has the new content-specified scroll offset. In such a case we want to trigger
+      // another repaint request to bring things back in sync. In most cases this repaint
+      // request will be a no-op and get filtered out in RequestContentRepaint, so it
+      // shouldn't have bad performance implications.
+      needContentRepaint = true;
     }
   }
 
   if (needContentRepaint) {
     RequestContentRepaint();
   }
 }
 
--- a/widget/xpwidgets/APZCCallbackHelper.cpp
+++ b/widget/xpwidgets/APZCCallbackHelper.cpp
@@ -90,16 +90,22 @@ ScrollFrameTo(nsIScrollableFrame* aFrame
 {
   if (!aFrame) {
     return CSSPoint();
   }
 
   // If the scrollable frame got a scroll request from something other than us
   // since the last layers update, then we don't want to push our scroll request
   // because we'll clobber that one, which is bad.
+  // Note that content may have just finished sending a layers update with a scroll
+  // offset update to the APZ, in which case the origin will be reset to null and we
+  // might actually be clobbering the content-side scroll offset with a stale APZ
+  // scroll offset. This is unavoidable because of the async communication between
+  // APZ and content; however the code in NotifyLayersUpdated should reissue a new
+  // repaint request to bring everything back into sync.
   if (!aFrame->OriginOfLastScroll() || aFrame->OriginOfLastScroll() == nsGkAtoms::apz) {
     aFrame->ScrollToCSSPixelsApproximate(aPoint, nsGkAtoms::apz);
   }
   // Return the final scroll position after setting it so that anything that relies
   // on it can have an accurate value. Note that even if we set it above re-querying it
   // is a good idea because it may have gotten clamped or rounded.
   return CSSPoint::FromAppUnits(aFrame->GetScrollPosition());
 }