Bug 1431130 - Ensure we retain the lock when accessing sLayerIndirectTree off the compositor thread. r=botond
authorAndrew Osmond <aosmond@mozilla.com>
Thu, 18 Jan 2018 07:28:56 -0500
changeset 399870 3919edc6531353d70f57214ec2f7ae7376010763
parent 399869 4dc616cbbb203e6b36ae21507359e70481a25919
child 399871 398fb8533bcb1ca48b5977ef04e3efa97b089214
push id33279
push useraciure@mozilla.com
push dateThu, 18 Jan 2018 21:53:37 +0000
treeherdermozilla-central@cffb3cd9dbb1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbotond
bugs1431130
milestone59.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 1431130 - Ensure we retain the lock when accessing sLayerIndirectTree off the compositor thread. r=botond CompositorBridgeParent::GetIndirectShadowTree is meant to be a compositor thread only method. This however was not enforced with an assert and over time we began using it on the main thread for simple accesses. These accesses should generally be safe on the main thread, depending on the individual access, but not outside the lock GetIndirectShadowTree holds. As such, a safer variant is provided to execute a lambda inside the lock context, and the unsafe variant will assert it is indeed in the compositor thread context.
gfx/layers/apz/src/APZCTreeManager.cpp
gfx/layers/apz/src/APZCTreeManager.h
gfx/layers/ipc/CompositorBridgeParent.cpp
gfx/layers/ipc/CompositorBridgeParent.h
gfx/layers/ipc/UiCompositorControllerParent.cpp
--- a/gfx/layers/apz/src/APZCTreeManager.cpp
+++ b/gfx/layers/apz/src/APZCTreeManager.cpp
@@ -703,27 +703,29 @@ APZCTreeManager::StopAutoscroll(const Sc
   if (RefPtr<AsyncPanZoomController> apzc = GetTargetAPZC(aGuid)) {
     apzc->StopAutoscroll();
   }
 }
 
 void
 APZCTreeManager::NotifyScrollbarDragRejected(const ScrollableLayerGuid& aGuid) const
 {
-  const LayerTreeState* state = CompositorBridgeParent::GetIndirectShadowTree(aGuid.mLayersId);
-  MOZ_ASSERT(state && state->mController);
-  state->mController->NotifyAsyncScrollbarDragRejected(aGuid.mScrollId);
+  RefPtr<GeckoContentController> controller =
+    GetContentController(aGuid.mLayersId);
+  MOZ_ASSERT(controller);
+  controller->NotifyAsyncScrollbarDragRejected(aGuid.mScrollId);
 }
 
 void
 APZCTreeManager::NotifyAutoscrollRejected(const ScrollableLayerGuid& aGuid) const
 {
-  const LayerTreeState* state = CompositorBridgeParent::GetIndirectShadowTree(aGuid.mLayersId);
-  MOZ_ASSERT(state && state->mController);
-  state->mController->NotifyAsyncAutoscrollRejected(aGuid.mScrollId);
+  RefPtr<GeckoContentController> controller =
+    GetContentController(aGuid.mLayersId);
+  MOZ_ASSERT(controller);
+  controller->NotifyAsyncAutoscrollRejected(aGuid.mScrollId);
 }
 
 template<class ScrollNode> HitTestingTreeNode*
 APZCTreeManager::PrepareNodeForLayer(const ScrollNode& aLayer,
                                      const FrameMetrics& aMetrics,
                                      uint64_t aLayersId,
                                      const AncestorTransform& aAncestorTransform,
                                      HitTestingTreeNode* aParent,
@@ -984,22 +986,21 @@ WillHandleInput(const PanGestureOrScroll
 
 void
 APZCTreeManager::FlushApzRepaints(uint64_t aLayersId)
 {
   // Previously, paints were throttled and therefore this method was used to
   // ensure any pending paints were flushed. Now, paints are flushed
   // immediately, so it is safe to simply send a notification now.
   APZCTM_LOG("Flushing repaints for layers id 0x%" PRIx64 "\n", aLayersId);
-  const LayerTreeState* state =
-    CompositorBridgeParent::GetIndirectShadowTree(aLayersId);
-  MOZ_ASSERT(state && state->mController);
-  state->mController->DispatchToRepaintThread(
+  RefPtr<GeckoContentController> controller = GetContentController(aLayersId);
+  MOZ_ASSERT(controller);
+  controller->DispatchToRepaintThread(
     NewRunnableMethod("layers::GeckoContentController::NotifyFlushComplete",
-                      state->mController,
+                      controller,
                       &GeckoContentController::NotifyFlushComplete));
 }
 
 nsEventStatus
 APZCTreeManager::ReceiveInputEvent(InputData& aEvent,
                                    ScrollableLayerGuid* aOutTargetGuid,
                                    uint64_t* aOutInputBlockId)
 {
@@ -1066,19 +1067,20 @@ APZCTreeManager::ReceiveInputEvent(Input
         if (!apzc && mRootNode) {
           apzc = mRootNode->GetApzc();
         }
       }
 
       if (apzc) {
         if (gfxPrefs::APZTestLoggingEnabled() && mouseInput.mType == MouseInput::MOUSE_HITTEST) {
           ScrollableLayerGuid guid = apzc->GetGuid();
-          if (LayerTreeState* state = CompositorBridgeParent::GetIndirectShadowTree(guid.mLayersId)) {
-            state->mApzTestData.RecordHitResult(mouseInput.mOrigin, hitResult, guid.mScrollId);
-          }
+          CompositorBridgeParent::CallWithIndirectShadowTree(guid.mLayersId,
+            [&](LayerTreeState& aState) -> void {
+              aState.mApzTestData.RecordHitResult(mouseInput.mOrigin, hitResult, guid.mScrollId);
+            });
         }
 
         bool targetConfirmed = (hitResult != CompositorHitTestInfo::eInvisibleToHitTest)
                             && !(hitResult & CompositorHitTestInfo::eDispatchToContent);
         bool apzDragEnabled = gfxPrefs::APZDragEnabled();
         if (apzDragEnabled && hitScrollbar) {
           // If scrollbar dragging is enabled and we hit a scrollbar, wait
           // for the main-thread confirmation because it contains drag metrics
@@ -2852,24 +2854,36 @@ APZCTreeManager::ComputeTransformForNode
   // Otherwise, the node does not have an async transform.
   return aNode->GetTransform() * AsyncTransformMatrix();
 }
 
 already_AddRefed<wr::WebRenderAPI>
 APZCTreeManager::GetWebRenderAPI() const
 {
   RefPtr<wr::WebRenderAPI> api;
-  if (LayerTreeState* state = CompositorBridgeParent::GetIndirectShadowTree(mRootLayersId)) {
-    if (state->mWrBridge) {
-      api = state->mWrBridge->GetWebRenderAPI();
-    }
-  }
+  CompositorBridgeParent::CallWithIndirectShadowTree(mRootLayersId,
+    [&](LayerTreeState& aState) -> void {
+      if (aState.mWrBridge) {
+        api = aState.mWrBridge->GetWebRenderAPI();
+      }
+    });
   return api.forget();
 }
 
+already_AddRefed<GeckoContentController>
+APZCTreeManager::GetContentController(uint64_t aLayersId) const
+{
+  RefPtr<GeckoContentController> controller;
+  CompositorBridgeParent::CallWithIndirectShadowTree(aLayersId,
+    [&](LayerTreeState& aState) -> void {
+      controller = aState.mController;
+    });
+  return controller.forget();
+}
+
 #if defined(MOZ_WIDGET_ANDROID)
 void
 APZCTreeManager::InitializeDynamicToolbarAnimator(const int64_t& aRootLayerTreeId)
 {
   MOZ_ASSERT(mToolbarAnimator);
   mToolbarAnimator->Initialize(aRootLayerTreeId);
 }
 
--- a/gfx/layers/apz/src/APZCTreeManager.h
+++ b/gfx/layers/apz/src/APZCTreeManager.h
@@ -617,16 +617,19 @@ private:
 
   // Requires the caller to hold mTreeLock.
   LayerToParentLayerMatrix4x4 ComputeTransformForNode(const HitTestingTreeNode* aNode) const;
 
   // Returns a pointer to the WebRenderAPI for the root layers id this APZCTreeManager
   // is for. This might be null (for example, if WebRender is not enabled).
   already_AddRefed<wr::WebRenderAPI> GetWebRenderAPI() const;
 
+  // Returns a pointer to the GeckoContentController for the given layers id.
+  already_AddRefed<GeckoContentController> GetContentController(uint64_t aLayersId) const;
+
 protected:
   /* The input queue where input events are held until we know enough to
    * figure out where they're going. Protected so gtests can access it.
    */
   RefPtr<InputQueue> mInputQueue;
 
 private:
   /* Layers id for the root CompositorBridgeParent that owns this APZCTreeManager. */
--- a/gfx/layers/ipc/CompositorBridgeParent.cpp
+++ b/gfx/layers/ipc/CompositorBridgeParent.cpp
@@ -2061,24 +2061,46 @@ UpdateIndirectTree(uint64_t aId, Layer* 
   MonitorAutoLock lock(*sIndirectLayerTreesLock);
   sIndirectLayerTrees[aId].mRoot = aRoot;
   sIndirectLayerTrees[aId].mTargetConfig = aTargetConfig;
 }
 
 /* static */ CompositorBridgeParent::LayerTreeState*
 CompositorBridgeParent::GetIndirectShadowTree(uint64_t aId)
 {
+  // Only the compositor thread should use this method variant, however it is
+  // safe to be called on the main thread during APZ gtests.
+  APZThreadUtils::AssertOnCompositorThread();
+
   MonitorAutoLock lock(*sIndirectLayerTreesLock);
   LayerTreeMap::iterator cit = sIndirectLayerTrees.find(aId);
   if (sIndirectLayerTrees.end() == cit) {
     return nullptr;
   }
   return &cit->second;
 }
 
+/* static */ bool
+CompositorBridgeParent::CallWithIndirectShadowTree(uint64_t aId,
+                                                   const std::function<void(CompositorBridgeParent::LayerTreeState&)>& aFunc)
+{
+  // Note that this does not make things universally threadsafe just because the
+  // sIndirectLayerTreesLock mutex is held. This is because the compositor
+  // thread can mutate the LayerTreeState outside the lock. It does however
+  // ensure that the *storage* for the LayerTreeState remains stable, since we
+  // should always hold the lock when adding/removing entries to the map.
+  MonitorAutoLock lock(*sIndirectLayerTreesLock);
+  LayerTreeMap::iterator cit = sIndirectLayerTrees.find(aId);
+  if (sIndirectLayerTrees.end() == cit) {
+    return false;
+  }
+  aFunc(cit->second);
+  return true;
+}
+
 static CompositorBridgeParent::LayerTreeState*
 GetStateForRoot(uint64_t aContentLayersId, const MonitorAutoLock& aProofOfLock)
 {
   CompositorBridgeParent::LayerTreeState* state = nullptr;
   LayerTreeMap::iterator itr = sIndirectLayerTrees.find(aContentLayersId);
   if (sIndirectLayerTrees.end() != itr) {
     state = &itr->second;
   }
--- a/gfx/layers/ipc/CompositorBridgeParent.h
+++ b/gfx/layers/ipc/CompositorBridgeParent.h
@@ -370,16 +370,24 @@ public:
   /**
    * Lookup the indirect shadow tree for |aId| and return it if it
    * exists.  Otherwise null is returned.  This must only be called on
    * the compositor thread.
    */
   static LayerTreeState* GetIndirectShadowTree(uint64_t aId);
 
   /**
+   * Lookup the indirect shadow tree for |aId|, call the function object and
+   * return true if found. If not found, return false.
+   */
+  static bool CallWithIndirectShadowTree(
+        uint64_t aId,
+        const std::function<void(LayerTreeState&)>& aFunc);
+
+  /**
    * Given the layers id for a content process, get the APZCTreeManagerParent
    * for the corresponding *root* layers id. That is, the APZCTreeManagerParent,
    * if one is found, will always be connected to the parent process rather
    * than a content process. Note that unless the compositor process is
    * separated this is expected to return null, because if the compositor is
    * living in the gecko parent process then there is no APZCTreeManagerParent
    * for the parent process.
    */
--- a/gfx/layers/ipc/UiCompositorControllerParent.cpp
+++ b/gfx/layers/ipc/UiCompositorControllerParent.cpp
@@ -16,22 +16,22 @@
 namespace mozilla {
 namespace layers {
 
 typedef CompositorBridgeParent::LayerTreeState LayerTreeState;
 
 /* static */ RefPtr<UiCompositorControllerParent>
 UiCompositorControllerParent::GetFromRootLayerTreeId(const uint64_t& aRootLayerTreeId)
 {
-  LayerTreeState* state = CompositorBridgeParent::GetIndirectShadowTree(aRootLayerTreeId);
-  if (state) {
-    return state->mUiControllerParent;
-  }
-
-  return nullptr;
+  RefPtr<UiCompositorControllerParent> controller;
+  CompositorBridgeParent::CallWithIndirectShadowTree(aRootLayerTreeId,
+    [&](LayerTreeState& aState) -> void {
+      controller = aState.mUiControllerParent;
+    });
+  return Move(controller);
 }
 
 /* static */ RefPtr<UiCompositorControllerParent>
 UiCompositorControllerParent::Start(const uint64_t& aRootLayerTreeId, Endpoint<PUiCompositorControllerParent>&& aEndpoint)
 {
   RefPtr<UiCompositorControllerParent> parent = new UiCompositorControllerParent(aRootLayerTreeId);
 
   RefPtr<Runnable> task =