Bug 1477832 - Acquire AsyncPanZoomController::mRecursiveMutex in more places to make sure it's held whenever Metrics() is accessed. r=botond
authorSrujana Peddinti <srujana.htt121@gmail.com>
Fri, 26 Apr 2019 18:57:48 -0400
changeset 531269 b758384a42e27280c1b11f2a60ff2a378949583e
parent 531268 a9d7796acb8e2444e1452e3ab2dbb9c037994ea4
child 531270 e330d90e540084087b6a9c155eabe069ec690c49
push id11265
push userffxbld-merge
push dateMon, 13 May 2019 10:53:39 +0000
treeherdermozilla-beta@77e0fe8dbdd3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbotond
bugs1477832
milestone68.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 1477832 - Acquire AsyncPanZoomController::mRecursiveMutex in more places to make sure it's held whenever Metrics() is accessed. r=botond Differential Revision: https://phabricator.services.mozilla.com/D29084
gfx/layers/apz/src/APZCTreeManager.cpp
gfx/layers/apz/src/AsyncPanZoomController.cpp
gfx/layers/apz/src/AsyncPanZoomController.h
--- a/gfx/layers/apz/src/APZCTreeManager.cpp
+++ b/gfx/layers/apz/src/APZCTreeManager.cpp
@@ -641,17 +641,17 @@ void APZCTreeManager::SampleForWebRender
     }
 
     ParentLayerPoint layerTranslation =
         apzc->GetCurrentAsyncTransform(AsyncPanZoomController::eForCompositing)
             .mTranslation;
     LayoutDeviceToParentLayerScale zoom;
     if (Maybe<uint64_t> zoomAnimationId = apzc->GetZoomAnimationId()) {
       // for now we only support zooming on root content APZCs
-      MOZ_ASSERT(apzc->Metrics().IsRootContent());
+      MOZ_ASSERT(apzc->IsRootContent());
       zoom = apzc->GetCurrentPinchZoomScale(
           AsyncPanZoomController::eForCompositing);
       transforms.AppendElement(wr::ToWrTransformProperty(
           *zoomAnimationId, Matrix4x4::Scaling(zoom.scale, zoom.scale, 1.0f)));
     }
 
     // The positive translation means the painted content is supposed to
     // move down (or to the right), and that corresponds to a reduction in
--- a/gfx/layers/apz/src/AsyncPanZoomController.cpp
+++ b/gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ -841,20 +841,20 @@ AsyncPanZoomController::AsyncPanZoomCont
       mLastContentPaintMetrics(mLastContentPaintMetadata.GetMetrics()),
       mX(this),
       mY(this),
       mPanDirRestricted(false),
       mPinchLocked(false),
       mPinchEventBuffer(
           TimeDuration::FromMilliseconds(gfxPrefs::APZPinchLockBufferMaxAge())),
       mZoomConstraints(false, false,
-                       Metrics().GetDevPixelsPerCSSPixel() * kViewportMinScale /
-                           ParentLayerToScreenScale(1),
-                       Metrics().GetDevPixelsPerCSSPixel() * kViewportMaxScale /
-                           ParentLayerToScreenScale(1)),
+                       mScrollMetadata.GetMetrics().GetDevPixelsPerCSSPixel() *
+                           kViewportMinScale / ParentLayerToScreenScale(1),
+                       mScrollMetadata.GetMetrics().GetDevPixelsPerCSSPixel() *
+                           kViewportMaxScale / ParentLayerToScreenScale(1)),
       mLastSampleTime(GetFrameTime()),
       mLastCheckerboardReport(GetFrameTime()),
       mOverscrollEffect(MakeUnique<OverscrollEffect>(*this)),
       mState(NOTHING),
       mNotificationBlockers(0),
       mInputQueue(aInputQueue),
       mPinchPaintTimerSet(false),
       mAPZCId(sAsyncPanZoomControllerCount++),
@@ -905,18 +905,18 @@ void AsyncPanZoomController::Destroy() {
     mGeckoContentController = nullptr;
     mGestureEventListener = nullptr;
   }
   mParent = nullptr;
   mTreeManager = nullptr;
 
   // Only send the release message if the SharedFrameMetrics has been created.
   if (mMetricsSharingController && mSharedFrameMetricsBuffer) {
-    Unused << mMetricsSharingController->StopSharingMetrics(
-        Metrics().GetScrollId(), mAPZCId);
+    Unused << mMetricsSharingController->StopSharingMetrics(GetScrollId(),
+                                                            mAPZCId);
   }
 
   {  // scope the lock
     RecursiveMutexAutoLock lock(mRecursiveMutex);
     mSharedFrameMetricsBuffer = nullptr;
     delete mSharedLock;
     mSharedLock = nullptr;
   }
@@ -1571,16 +1571,17 @@ nsEventStatus AsyncPanZoomController::On
       controller->NotifyPinchGesture(aEvent.mType, GetGuid(), 0,
                                      aEvent.modifiers);
     }
   }
 
   SetState(PINCHING);
   mX.SetVelocity(0);
   mY.SetVelocity(0);
+  RecursiveMutexAutoLock lock(mRecursiveMutex);
   mLastZoomFocus =
       aEvent.mLocalFocusPoint - Metrics().GetCompositionBounds().TopLeft();
 
   mPinchEventBuffer.push(aEvent);
 
   return nsEventStatus_eConsumeNoDefault;
 }
 
@@ -1618,26 +1619,25 @@ nsEventStatus AsyncPanZoomController::On
           aEvent.mType, GetGuid(),
           ViewAs<LayoutDevicePixel>(
               aEvent.mCurrentSpan - aEvent.mPreviousSpan,
               PixelCastJustification::LayoutDeviceIsParentLayerForRCDRSF),
           aEvent.modifiers);
     }
   }
 
-  // Only the root APZC is zoomable, and the root APZC is not allowed to have
-  // different x and y scales. If it did, the calculations in this function
-  // would have to be adjusted (as e.g. it would no longer be valid to take
-  // the minimum or maximum of the ratios of the widths and heights of the
-  // page rect and the composition bounds).
-  MOZ_ASSERT(Metrics().IsRootContent());
-  MOZ_ASSERT(Metrics().GetZoom().AreScalesSame());
-
   {
     RecursiveMutexAutoLock lock(mRecursiveMutex);
+    // Only the root APZC is zoomable, and the root APZC is not allowed to have
+    // different x and y scales. If it did, the calculations in this function
+    // would have to be adjusted (as e.g. it would no longer be valid to take
+    // the minimum or maximum of the ratios of the widths and heights of the
+    // page rect and the composition bounds).
+    MOZ_ASSERT(Metrics().IsRootContent());
+    MOZ_ASSERT(Metrics().GetZoom().AreScalesSame());
 
     CSSToParentLayerScale userZoom = Metrics().GetZoom().ToScaleFactor();
     ParentLayerPoint focusPoint =
         aEvent.mLocalFocusPoint - Metrics().GetCompositionBounds().TopLeft();
     CSSPoint cssFocusPoint = focusPoint / Metrics().GetZoom();
 
     ParentLayerPoint focusChange = mLastZoomFocus - focusPoint;
     mLastZoomFocus = focusPoint;
@@ -2337,17 +2337,17 @@ nsEventStatus AsyncPanZoomController::On
              this);
   } else if ((delta.x || delta.y) && !CanScrollWithWheel(delta)) {
     // We can't scroll this apz anymore, so we simply drop the event.
     if (mInputQueue->GetActiveWheelTransaction() &&
         gfxPrefs::MouseScrollTestingEnabled()) {
       if (RefPtr<GeckoContentController> controller =
               GetGeckoContentController()) {
         controller->NotifyMozMouseScrollEvent(
-            Metrics().GetScrollId(), NS_LITERAL_STRING("MozMouseScrollFailed"));
+            GetScrollId(), NS_LITERAL_STRING("MozMouseScrollFailed"));
       }
     }
     return nsEventStatus_eConsumeNoDefault;
   }
 
   MOZ_ASSERT(mInputQueue->GetCurrentWheelBlock());
   AdjustDeltaForAllowedScrollDirections(
       delta, mInputQueue->GetCurrentWheelBlock()->GetAllowedScrollDirections());
@@ -2357,17 +2357,21 @@ nsEventStatus AsyncPanZoomController::On
     return nsEventStatus_eIgnore;
   }
 
   switch (aEvent.mScrollMode) {
     case ScrollWheelInput::SCROLLMODE_INSTANT: {
       // Wheel events from "clicky" mouse wheels trigger scroll snapping to the
       // next snap point. Check for this, and adjust the delta to take into
       // account the snap point.
-      CSSPoint startPosition = Metrics().GetScrollOffset();
+      CSSPoint startPosition;
+      {
+        RecursiveMutexAutoLock lock(mRecursiveMutex);
+        startPosition = Metrics().GetScrollOffset();
+      }
       MaybeAdjustDeltaForScrollSnappingOnWheelInput(aEvent, delta,
                                                     startPosition);
 
       ScreenPoint distance = ToScreenCoordinates(
           ParentLayerPoint(fabs(delta.x), fabs(delta.y)), aEvent.mLocalOrigin);
 
       CancelAnimation();
 
@@ -2450,18 +2454,17 @@ nsEventStatus AsyncPanZoomController::On
 }
 
 void AsyncPanZoomController::NotifyMozMouseScrollEvent(
     const nsString& aString) const {
   RefPtr<GeckoContentController> controller = GetGeckoContentController();
   if (!controller) {
     return;
   }
-
-  controller->NotifyMozMouseScrollEvent(Metrics().GetScrollId(), aString);
+  controller->NotifyMozMouseScrollEvent(GetScrollId(), aString);
 }
 
 nsEventStatus AsyncPanZoomController::OnPanMayBegin(
     const PanGestureInput& aEvent) {
   APZC_LOG("%p got a pan-maybegin in state %d\n", this, mState);
 
   mX.StartTouch(aEvent.mLocalPanStartPoint.x, aEvent.mTime);
   mY.StartTouch(aEvent.mLocalPanStartPoint.y, aEvent.mTime);
@@ -3971,34 +3974,32 @@ bool AsyncPanZoomController::AdvanceAnim
   // smooth. If an animation frame is requested, it is the compositor's
   // responsibility to schedule a composite.
   mAsyncTransformAppliedToContent = false;
   bool requestAnimationFrame = false;
   nsTArray<RefPtr<Runnable>> deferredTasks;
 
   {
     RecursiveMutexAutoLock lock(mRecursiveMutex);
-
     {  // scope lock
       MutexAutoLock lock(mCheckerboardEventLock);
       // Update RendertraceProperty before UpdateAnimation() call, since
       // the UpdateAnimation() updates effective ScrollOffset for next frame
       // if APZFrameDelay is enabled.
       if (mCheckerboardEvent) {
         mCheckerboardEvent->UpdateRendertraceProperty(
             CheckerboardEvent::UserVisible,
             CSSRect(GetEffectiveScrollOffset(
                         AsyncPanZoomController::eForCompositing),
                     Metrics().CalculateCompositedSizeInCssPixels()));
       }
     }
 
     requestAnimationFrame = UpdateAnimation(aSampleTime, &deferredTasks);
   }
-
   // Execute any deferred tasks queued up by mAnimation's Sample() (called by
   // UpdateAnimation()). This needs to be done after the monitor is released
   // since the tasks are allowed to call APZCTreeManager methods which can grab
   // the tree lock.
   for (uint32_t i = 0; i < deferredTasks.Length(); ++i) {
     APZThreadUtils::RunOnControllerThread(deferredTasks[i].forget());
   }
 
@@ -4154,16 +4155,17 @@ AsyncPanZoomController::GetCurrentAsyncT
     AsyncTransformConsumer aMode, AsyncTransformComponents aComponents) const {
   return AsyncTransformComponentMatrix(
              GetCurrentAsyncTransform(aMode, aComponents)) *
          GetOverscrollTransform(aMode);
 }
 
 LayoutDeviceToParentLayerScale AsyncPanZoomController::GetCurrentPinchZoomScale(
     AsyncTransformConsumer aMode) const {
+  RecursiveMutexAutoLock lock(mRecursiveMutex);
   AutoApplyAsyncTestAttributes testAttributeApplier(this);
   CSSToParentLayerScale2D scale = GetEffectiveZoom(aMode);
   // Note that in general the zoom might have different x- and y-scales.
   // However, this function in particular is only used on the WebRender codepath
   // for which the scales should always be the same.
   return scale.ToScaleFactor() / Metrics().GetDevPixelsPerCSSPixel();
 }
 
@@ -4750,17 +4752,17 @@ FrameMetrics& AsyncPanZoomController::Me
 
 const FrameMetrics& AsyncPanZoomController::Metrics() const {
   return mScrollMetadata.GetMetrics();
   ;
 }
 
 const FrameMetrics& AsyncPanZoomController::GetFrameMetrics() const {
   mRecursiveMutex.AssertCurrentThreadIn();
-  return mScrollMetadata.GetMetrics();
+  return Metrics();
   ;
 }
 
 const ScrollMetadata& AsyncPanZoomController::GetScrollMetadata() const {
   mRecursiveMutex.AssertCurrentThreadIn();
   return mScrollMetadata;
 }
 
@@ -4790,29 +4792,29 @@ void AsyncPanZoomController::ZoomToRect(
     // If zooming out is disabled, an empty rect is nonsensical
     // and will produce undesirable scrolling.
     NS_WARNING(
         "ZoomToRect got called with an empty rect and zoom out disabled; "
         "ignoring...");
     return;
   }
 
-  // Only the root APZC is zoomable, and the root APZC is not allowed to have
-  // different x and y scales. If it did, the calculations in this function
-  // would have to be adjusted (as e.g. it would no longer be valid to take
-  // the minimum or maximum of the ratios of the widths and heights of the
-  // page rect and the composition bounds).
-  MOZ_ASSERT(Metrics().IsRootContent());
-  MOZ_ASSERT(Metrics().GetZoom().AreScalesSame());
-
   SetState(ANIMATING_ZOOM);
 
   {
     RecursiveMutexAutoLock lock(mRecursiveMutex);
 
+    // Only the root APZC is zoomable, and the root APZC is not allowed to have
+    // different x and y scales. If it did, the calculations in this function
+    // would have to be adjusted (as e.g. it would no longer be valid to take
+    // the minimum or maximum of the ratios of the widths and heights of the
+    // page rect and the composition bounds).
+    MOZ_ASSERT(Metrics().IsRootContent());
+    MOZ_ASSERT(Metrics().GetZoom().AreScalesSame());
+
     ParentLayerRect compositionBounds = Metrics().GetCompositionBounds();
     CSSRect cssPageRect = Metrics().GetScrollableRect();
     CSSPoint scrollOffset = Metrics().GetScrollOffset();
     CSSToParentLayerScale currentZoom = Metrics().GetZoom().ToScaleFactor();
     CSSToParentLayerScale targetZoom;
 
     // The minimum zoom to prevent over-zoom-out.
     // If the zoom factor is lower than this (i.e. we are zoomed more into the
@@ -5018,16 +5020,17 @@ void AsyncPanZoomController::DispatchSta
                !IsTransformingState(aNewState)) {
 #if defined(MOZ_WIDGET_ANDROID)
       // The Android UI thread only shows overlay UI elements when the content
       // is not being panned or zoomed and it is in a steady state. So the
       // FrameMetrics only need to be updated when the transform ends.
       if (APZCTreeManager* manager = GetApzcTreeManager()) {
         if (AndroidDynamicToolbarAnimator* animator =
                 manager->GetAndroidDynamicToolbarAnimator()) {
+          RecursiveMutexAutoLock lock(mRecursiveMutex);
           animator->UpdateRootFrameMetrics(Metrics());
         }
       }
 #endif
 
       controller->NotifyAPZStateChange(GetGuid(),
                                        APZStateChange::eTransformEnd);
 #if defined(XP_WIN) || defined(MOZ_WIDGET_GTK)
@@ -5054,16 +5057,17 @@ void AsyncPanZoomController::UpdateZoomC
            aConstraints.mAllowZoom, aConstraints.mAllowDoubleTapZoom,
            aConstraints.mMinZoom.scale, aConstraints.mMaxZoom.scale);
   if (IsNaN(aConstraints.mMinZoom.scale) ||
       IsNaN(aConstraints.mMaxZoom.scale)) {
     NS_WARNING("APZC received zoom constraints with NaN values; dropping...");
     return;
   }
 
+  RecursiveMutexAutoLock lock(mRecursiveMutex);
   CSSToParentLayerScale min = Metrics().GetDevPixelsPerCSSPixel() *
                               kViewportMinScale / ParentLayerToScreenScale(1);
   CSSToParentLayerScale max = Metrics().GetDevPixelsPerCSSPixel() *
                               kViewportMaxScale / ParentLayerToScreenScale(1);
 
   // inf float values and other bad cases should be sanitized by the code below.
   mZoomConstraints.mAllowZoom = aConstraints.mAllowZoom;
   mZoomConstraints.mAllowDoubleTapZoom = aConstraints.mAllowDoubleTapZoom;
@@ -5104,16 +5108,17 @@ bool AsyncPanZoomController::HasTreeMana
 
 void AsyncPanZoomController::GetGuid(ScrollableLayerGuid* aGuidOut) const {
   if (aGuidOut) {
     *aGuidOut = GetGuid();
   }
 }
 
 ScrollableLayerGuid AsyncPanZoomController::GetGuid() const {
+  RecursiveMutexAutoLock lock(mRecursiveMutex);
   return ScrollableLayerGuid(mLayersId, Metrics().GetPresShellId(),
                              Metrics().GetScrollId());
 }
 
 void AsyncPanZoomController::UpdateSharedCompositorFrameMetrics() {
   mRecursiveMutex.AssertCurrentThreadIn();
 
   FrameMetrics* frame =
--- a/gfx/layers/apz/src/AsyncPanZoomController.h
+++ b/gfx/layers/apz/src/AsyncPanZoomController.h
@@ -1404,16 +1404,22 @@ class AsyncPanZoomController {
   // further above due to initialization-order constraints.
 
   RefPtr<AsyncPanZoomController> mParent;
 
   /* ===================================================================
    * The functions and members in this section are used for scrolling,
    * including handing off scroll to another APZC, and overscrolling.
    */
+
+  ScrollableLayerGuid::ViewID GetScrollId() const {
+    RecursiveMutexAutoLock lock(mRecursiveMutex);
+    return Metrics().GetScrollId();
+  }
+
  public:
   ScrollableLayerGuid::ViewID GetScrollHandoffParentId() const {
     return mScrollMetadata.GetScrollParentId();
   }
 
   /**
    * Attempt to scroll in response to a touch-move from |aStartPoint| to
    * |aEndPoint|, which are in our (transformed) screen coordinates.