Bug 1418387 - Add missing tree lock. r?botond draft
authorKartikaya Gupta <kgupta@mozilla.com>
Fri, 24 Nov 2017 16:23:05 -0500
changeset 703339 e4248e57686c8077e49a23b88e65a5084ef74da8
parent 703338 f554a355862b8b587126b4490a28384097d78cb8
child 703340 133b72a8ff6351182746ebf6d42a57d171ebad25
push id90784
push userkgupta@mozilla.com
push dateFri, 24 Nov 2017 21:25:10 +0000
reviewersbotond
bugs1418387, 1417519
milestone59.0a1
Bug 1418387 - Add missing tree lock. r?botond This is follow-up to bug 1417519, to fix an incorrect change in that bug. GetAPZCAtPointWR calls things like FindRootApzcForLayersId which require the tree lock to be held, so we should hold it. It makes more sense to hold the lock across the whole GetTargetAPZC function since we don't want tree mutations to happen while we're in this function. It also means we can't call GetTargetAPZC inside GetAPZCAtPointWR because that will recursively try to pick up the tree lock; instead we can use GetTargetNode and get the APZC from that. MozReview-Commit-ID: 7ZXQMMes8hV
gfx/layers/apz/src/APZCTreeManager.cpp
--- a/gfx/layers/apz/src/APZCTreeManager.cpp
+++ b/gfx/layers/apz/src/APZCTreeManager.cpp
@@ -2183,24 +2183,22 @@ APZCTreeManager::GetTargetNode(const Scr
   return target.forget();
 }
 
 already_AddRefed<AsyncPanZoomController>
 APZCTreeManager::GetTargetAPZC(const ScreenPoint& aPoint,
                                HitTestResult* aOutHitResult,
                                RefPtr<HitTestingTreeNode>* aOutScrollbarNode)
 {
+  MutexAutoLock lock(mTreeLock);
+
   HitTestResult hitResult = HitNothing;
   HitTestingTreeNode* scrollbarNode = nullptr;
   RefPtr<AsyncPanZoomController> target;
-
-  { // scope mTreeLock
-    MutexAutoLock lock(mTreeLock);
-    target = GetAPZCAtPoint(mRootNode, aPoint, &hitResult, &scrollbarNode);
-  }
+  target = GetAPZCAtPoint(mRootNode, aPoint, &hitResult, &scrollbarNode);
 
   if (gfxPrefs::WebRenderHitTest()) {
     HitTestResult wrHitResult = HitNothing;
     RefPtr<AsyncPanZoomController> wrTarget = GetAPZCAtPointWR(aPoint, &wrHitResult);
     // For now just compare the WR and non-WR results.
     if (wrHitResult != hitResult) {
       printf_stderr("WR hit result mismatch at %s: got %d, expected %d\n",
           Stringify(aPoint).c_str(), (int)wrHitResult, (int)hitResult);
@@ -2241,17 +2239,21 @@ APZCTreeManager::GetAPZCAtPointWR(const 
   gfx::CompositorHitTestInfo hitInfo;
   bool hitSomething = wr->HitTest(wr::ToWorldPoint(aHitTestPoint),
       pipelineId, scrollId, hitInfo);
   if (!hitSomething) {
     return result.forget();
   }
 
   uint64_t layersId = wr::AsUint64(pipelineId);
-  result = GetTargetAPZC(layersId, scrollId);
+  RefPtr<HitTestingTreeNode> node = GetTargetNode(
+      ScrollableLayerGuid(layersId, 0, scrollId),
+      &GuidComparatorIgnoringPresShell);
+  MOZ_ASSERT(!node || node->GetApzc()); // any node returned must have an APZC
+  result = node ? node->GetApzc() : nullptr;
   if (!result) {
     // It falls back to the root
     MOZ_ASSERT(scrollId == FrameMetrics::NULL_SCROLL_ID);
     result = FindRootApzcForLayersId(layersId);
     MOZ_ASSERT(result);
   }
 
   *aOutHitResult = HitLayer;