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

--HG--
extra : rebase_source : 318c27c4473e54b30a0393fca6c2fc76083036b2

gfx/layers/apz/src/APZCTreeManager.cpp

@@ -407,7 +407,7 @@ APZCTreeManager::UpdateHitTestingTreeImpl(LayersId aRootLayerTreeId,
         {
           mApzcTreeLog << aLayerMetrics.Name() << '\t';
 
-          HitTestingTreeNode* node = PrepareNodeForLayer(aLayerMetrics,
+          HitTestingTreeNode* node = PrepareNodeForLayer(lock, aLayerMetrics,
                 aLayerMetrics.Metrics(), layersId, ancestorTransforms.top(),
                 parent, next, state);
           MOZ_ASSERT(node);
@@ -724,7 +724,8 @@ GetEventRegions(const ScrollNode& aLayer)
 
 
 already_AddRefed<HitTestingTreeNode>
-APZCTreeManager::RecycleOrCreateNode(TreeBuildingState& aState,
+APZCTreeManager::RecycleOrCreateNode(const RecursiveMutexAutoLock& aProofOfTreeLock,
+                                     TreeBuildingState& aState,
                                      AsyncPanZoomController* aApzc,
                                      LayersId aLayersId)
 {
@@ -733,9 +734,9 @@ APZCTreeManager::RecycleOrCreateNode(TreeBuildingState& aState,
   // 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)) {
       aState.mNodesToDestroy.RemoveElementAt(i);
-      node->RecycleWith(aApzc, aLayersId);
+      node->RecycleWith(aProofOfTreeLock, aApzc, aLayersId);
       return node.forget();
     }
   }
@@ -829,7 +830,8 @@ APZCTreeManager::NotifyAutoscrollRejected(const ScrollableLayerGuid& aGuid) cons
 }
 
 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,
@@ -837,8 +839,6 @@ APZCTreeManager::PrepareNodeForLayer(const ScrollNode& aLayer,
                                      HitTestingTreeNode* aNextSibling,
                                      TreeBuildingState& aState)
 {
-  mTreeLock.AssertCurrentThreadIn();
-
   bool needsApzc = true;
   if (!aMetrics.IsScrollable()) {
     needsApzc = false;
@@ -866,7 +866,7 @@ APZCTreeManager::PrepareNodeForLayer(const ScrollNode& aLayer,
     // 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);
     node->SetHitTestData(
         GetEventRegions(aLayer),
@@ -1046,7 +1046,7 @@ APZCTreeManager::PrepareNodeForLayer(const ScrollNode& aLayer,
     // 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

gfx/layers/apz/src/APZCTreeManager.h

@@ -696,11 +696,13 @@ private:
                                             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,

gfx/layers/apz/src/HitTestingTreeNode.cpp

@@ -37,10 +37,11 @@ HitTestingTreeNode::HitTestingTreeNode(AsyncPanZoomController* aApzc,
 }
 
 void
-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;
@@ -67,8 +68,12 @@ HitTestingTreeNode::Destroy()
     }
     mApzc = nullptr;
   }
+}
 
-  mLayersId = LayersId{0};
+bool
+HitTestingTreeNode::IsRecyclable(const RecursiveMutexAutoLock& aProofOfTreeLock)
+{
+  return !IsPrimaryHolder();
 }
 
 void

gfx/layers/apz/src/HitTestingTreeNode.h

@@ -57,9 +57,18 @@ private:
 public:
   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);