Bug 1457590 - Strengthen the contract around recycling HitTestingTreeNodes. r=botond
authorKartikaya Gupta <kgupta@mozilla.com>
Wed, 30 May 2018 15:54:50 -0400
changeset 420571 8a1bdb8d8bf5a1c8869bf3f82fd86c56f9f4b72c
parent 420570 494c6daf83dfe7b37cac80a21bda894375f5dbec
child 420572 437b901404dbea22d9fc7fdf8c3d363adbdf20f3
push id34074
push userapavel@mozilla.com
push dateThu, 31 May 2018 10:03:38 +0000
treeherdermozilla-central@d5eed637f5dd [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
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 1457590 - Strengthen the contract around recycling HitTestingTreeNodes. r=botond This patch makes three related changes: - A non-functional change that factors a IsRecyclable function on HitTestingTreeNode. - A non-functional change that sprinkles proof-of-tree-lock arguments to a few HitTestingTreeNode functions, to ensure at compile-time that they can only be called while holding the tree lock. - A functional change that stops clearing mLayersId in HitTestingTreeNode::Destroy, so that if a node is non-recyclable, and it gets Destroy()'d while other code still has a RefPtr to it, that other code can still read the layers id off in a safe manner. These changes provide a stronger set of checks around node recycling, and allows for a safe mechanism to use a HitTestingTreeNode on the controller thread without having to lock the entire APZ tree. The mechanism is effectively a per-node lock, which will be added in the next patch. MozReview-Commit-ID: DBIjFDZJwhE
--- a/gfx/layers/apz/src/APZCTreeManager.cpp
+++ b/gfx/layers/apz/src/APZCTreeManager.cpp
@@ -402,17 +402,17 @@ APZCTreeManager::UpdateHitTestingTreeImp
     mApzcTreeLog << "[start]\n";
         [&](ScrollNode aLayerMetrics)
           mApzcTreeLog << aLayerMetrics.Name() << '\t';
-          HitTestingTreeNode* node = PrepareNodeForLayer(aLayerMetrics,
+          HitTestingTreeNode* node = PrepareNodeForLayer(lock, aLayerMetrics,
                 aLayerMetrics.Metrics(), layersId, ancestorTransforms.top(),
                 parent, next, state);
           AsyncPanZoomController* apzc = node->GetApzc();
           // GetScrollbarAnimationId is only non-zero when webrender is enabled,
           // which limits the extra thumb mapping work to the webrender-enabled
@@ -719,28 +719,29 @@ GetEventRegions(const ScrollNode& aLayer
     return eventRegions;
   return aLayer.GetEventRegions();
-APZCTreeManager::RecycleOrCreateNode(TreeBuildingState& aState,
+APZCTreeManager::RecycleOrCreateNode(const RecursiveMutexAutoLock& aProofOfTreeLock,
+                                     TreeBuildingState& aState,
                                      AsyncPanZoomController* aApzc,
                                      LayersId aLayersId)
   // Find a node without an APZC and return it. Note that unless the layer tree
   // actually changes, this loop should generally do an early-return on the
   // first iteration, so it should be cheap in the common case.
   for (int32_t i = aState.mNodesToDestroy.Length() - 1; i >= 0; i--) {
     RefPtr<HitTestingTreeNode> node = aState.mNodesToDestroy[i];
-    if (!node->IsPrimaryHolder()) {
+    if (node->IsRecyclable(aProofOfTreeLock)) {
-      node->RecycleWith(aApzc, aLayersId);
+      node->RecycleWith(aProofOfTreeLock, aApzc, aLayersId);
       return node.forget();
   RefPtr<HitTestingTreeNode> node = new HitTestingTreeNode(aApzc, false, aLayersId);
   return node.forget();
 template<class ScrollNode> static EventRegionsOverride
@@ -824,26 +825,25 @@ APZCTreeManager::NotifyAutoscrollRejecte
   RefPtr<GeckoContentController> controller =
 template<class ScrollNode> HitTestingTreeNode*
-APZCTreeManager::PrepareNodeForLayer(const ScrollNode& aLayer,
+APZCTreeManager::PrepareNodeForLayer(const RecursiveMutexAutoLock& aProofOfTreeLock,
+                                     const ScrollNode& aLayer,
                                      const FrameMetrics& aMetrics,
                                      LayersId aLayersId,
                                      const AncestorTransform& aAncestorTransform,
                                      HitTestingTreeNode* aParent,
                                      HitTestingTreeNode* aNextSibling,
                                      TreeBuildingState& aState)
-  mTreeLock.AssertCurrentThreadIn();
   bool needsApzc = true;
   if (!aMetrics.IsScrollable()) {
     needsApzc = false;
   // XXX: As a future optimization we can probably stick these things on the
   // TreeBuildingState, and update them as we change layers id during the
   // traversal
@@ -861,17 +861,17 @@ APZCTreeManager::PrepareNodeForLayer(con
   bool parentHasPerspective = aState.mParentHasPerspective.top();
   RefPtr<HitTestingTreeNode> node = nullptr;
   if (!needsApzc) {
     // Note: if layer properties must be propagated to nodes, RecvUpdate in
     // LayerTransactionParent.cpp must ensure that APZ will be notified
     // when those properties change.
-    node = RecycleOrCreateNode(aState, nullptr, aLayersId);
+    node = RecycleOrCreateNode(aProofOfTreeLock, aState, nullptr, aLayersId);
     AttachNodeToTree(node, aParent, aNextSibling);
         (!parentHasPerspective && aLayer.GetClipRect())
           ? Some(ParentLayerIntRegion(*aLayer.GetClipRect()))
           : Nothing(),
@@ -1041,17 +1041,17 @@ APZCTreeManager::PrepareNodeForLayer(con
     // Add a guid -> APZC mapping for the newly created APZC.
     insertResult.first->second = apzc;
   } else {
     // We already built an APZC earlier in this tree walk, but we have another layer
     // now that will also be using that APZC. The hit-test region on the APZC needs
     // to be updated to deal with the new layer's hit region.
-    node = RecycleOrCreateNode(aState, apzc, aLayersId);
+    node = RecycleOrCreateNode(aProofOfTreeLock, aState, apzc, aLayersId);
     AttachNodeToTree(node, aParent, aNextSibling);
     // Even though different layers associated with a given APZC may be at
     // different levels in the layer tree (e.g. one being an uncle of another),
     // we require from Layout that the CSS transforms up to their common
     // ancestor be roughly the same. There are cases in which the transforms
     // are not exactly the same, for example if the parent is container layer
     // for an opacity, and this container layer has a resolution-induced scale
--- a/gfx/layers/apz/src/APZCTreeManager.h
+++ b/gfx/layers/apz/src/APZCTreeManager.h
@@ -691,21 +691,23 @@ private:
                                                   ScrollableLayerGuid* aOutTargetGuid,
                                                   uint64_t* aOutInputBlockId);
   void FlushRepaintsToClearScreenToGeckoTransform();
   void SynthesizePinchGestureFromMouseWheel(const ScrollWheelInput& aWheelInput,
                                             const RefPtr<AsyncPanZoomController>& aTarget);
-  already_AddRefed<HitTestingTreeNode> RecycleOrCreateNode(TreeBuildingState& aState,
+  already_AddRefed<HitTestingTreeNode> RecycleOrCreateNode(const RecursiveMutexAutoLock& aProofOfTreeLock,
+                                                           TreeBuildingState& aState,
                                                            AsyncPanZoomController* aApzc,
                                                            LayersId aLayersId);
   template<class ScrollNode>
-  HitTestingTreeNode* PrepareNodeForLayer(const ScrollNode& aLayer,
+  HitTestingTreeNode* PrepareNodeForLayer(const RecursiveMutexAutoLock& aProofOfTreeLock,
+                                          const ScrollNode& aLayer,
                                           const FrameMetrics& aMetrics,
                                           LayersId aLayersId,
                                           const AncestorTransform& aAncestorTransform,
                                           HitTestingTreeNode* aParent,
                                           HitTestingTreeNode* aNextSibling,
                                           TreeBuildingState& aState);
   template<class ScrollNode>
--- a/gfx/layers/apz/src/HitTestingTreeNode.cpp
+++ b/gfx/layers/apz/src/HitTestingTreeNode.cpp
@@ -32,20 +32,21 @@ HitTestingTreeNode::HitTestingTreeNode(A
   if (mIsPrimaryApzcHolder) {
   MOZ_ASSERT(!mApzc || mApzc->GetLayersId() == mLayersId);
-HitTestingTreeNode::RecycleWith(AsyncPanZoomController* aApzc,
+HitTestingTreeNode::RecycleWith(const RecursiveMutexAutoLock& aProofOfTreeLock,
+                                AsyncPanZoomController* aApzc,
                                 LayersId aLayersId)
-  MOZ_ASSERT(!mIsPrimaryApzcHolder);
+  MOZ_ASSERT(IsRecyclable(aProofOfTreeLock));
   Destroy(); // clear out tree pointers
   mApzc = aApzc;
   mLayersId = aLayersId;
   MOZ_ASSERT(!mApzc || mApzc->GetLayersId() == mLayersId);
   // The caller is expected to call SetHitTestData to repopulate the hit-test
   // fields.
@@ -62,18 +63,22 @@ HitTestingTreeNode::Destroy()
   mParent = nullptr;
   if (mApzc) {
     if (mIsPrimaryApzcHolder) {
     mApzc = nullptr;
-  mLayersId = LayersId{0};
+HitTestingTreeNode::IsRecyclable(const RecursiveMutexAutoLock& aProofOfTreeLock)
+  return !IsPrimaryHolder();
 HitTestingTreeNode::SetLastChild(HitTestingTreeNode* aChild)
   mLastChild = aChild;
   if (aChild) {
     aChild->mParent = this;
--- a/gfx/layers/apz/src/HitTestingTreeNode.h
+++ b/gfx/layers/apz/src/HitTestingTreeNode.h
@@ -52,19 +52,28 @@ class AsyncPanZoomController;
 class HitTestingTreeNode {
   HitTestingTreeNode(AsyncPanZoomController* aApzc, bool aIsPrimaryHolder,
                      LayersId aLayersId);
-  void RecycleWith(AsyncPanZoomController* aApzc, LayersId aLayersId);
+  void RecycleWith(const RecursiveMutexAutoLock& aProofOfTreeLock,
+                   AsyncPanZoomController* aApzc,
+                   LayersId aLayersId);
+  // Clears the tree pointers on the node, thereby breaking RefPtr cycles. This
+  // can trigger free'ing of this and other HitTestingTreeNode instances.
   void Destroy();
+  // Returns true if and only if the node is available for recycling as part
+  // of a hit-testing tree update. Note that this node can have Destroy() called
+  // on it whether or not it is recyclable.
+  bool IsRecyclable(const RecursiveMutexAutoLock& aProofOfTreeLock);
   /* Tree construction methods */
   void SetLastChild(HitTestingTreeNode* aChild);
   void SetPrevSibling(HitTestingTreeNode* aSibling);
   void MakeRoot();
   /* Tree walking methods. GetFirstChild is O(n) in the number of children. The
    * other tree walking methods are all O(1). */