Bug 1437694 - Gracefully recover from hit testing bugs affecting scrollbar dragging. r=kats draft
authorBotond Ballo <botond@mozilla.com>
Mon, 12 Feb 2018 17:58:36 -0500
changeset 763354 37ef4bf990fc081e9884f604dbad0afc5cbeaf95
parent 760101 314710c9de4ed6782e1c46e28a8852446a84bfa2
push id101427
push userbballo@mozilla.com
push dateMon, 05 Mar 2018 21:17:29 +0000
reviewerskats
bugs1437694
milestone60.0a1
Bug 1437694 - Gracefully recover from hit testing bugs affecting scrollbar dragging. r=kats The recovery is only enabled for release builds, to avoid papering over such hit testing bugs. On nightly builds, a diagnostic assert is issued. MozReview-Commit-ID: Aos0j0jv6Lb
gfx/layers/apz/src/InputBlockState.cpp
gfx/layers/apz/src/InputBlockState.h
gfx/layers/apz/src/InputQueue.cpp
--- a/gfx/layers/apz/src/InputBlockState.cpp
+++ b/gfx/layers/apz/src/InputBlockState.cpp
@@ -34,28 +34,49 @@ InputBlockState::InputBlockState(const R
   // We should never be constructed with a nullptr target.
   MOZ_ASSERT(mTargetApzc);
   mOverscrollHandoffChain = mTargetApzc->BuildOverscrollHandoffChain();
 }
 
 bool
 InputBlockState::SetConfirmedTargetApzc(const RefPtr<AsyncPanZoomController>& aTargetApzc,
                                         TargetConfirmationState aState,
-                                        InputData* aFirstInput)
+                                        InputData* aFirstInput,
+                                        bool aForScrollbarDrag)
 {
   MOZ_ASSERT(aState == TargetConfirmationState::eConfirmed
           || aState == TargetConfirmationState::eTimedOut);
 
   if (mTargetConfirmed == TargetConfirmationState::eTimedOut &&
       aState == TargetConfirmationState::eConfirmed) {
     // The main thread finally responded. We had already timed out the
     // confirmation, but we want to update the state internally so that we
     // can record the time for telemetry purposes.
     mTargetConfirmed = TargetConfirmationState::eTimedOutAndMainThreadResponded;
   }
+  // Sometimes, bugs in compositor hit testing can lead to APZ confirming
+  // a different target than the main thread. If this happens for a drag
+  // block created for a scrollbar drag, the consequences can be fairly
+  // user-unfriendly, such as the scrollbar not being draggable at all,
+  // or it scrolling the contents of the wrong scrollframe. In Nightly
+  // builds, we issue a diagnostic assert in this situation, so that the
+  // underlying compositor hit testing bug can be fixed. In release builds,
+  // however, we just silently accept the main thread's confirmed target,
+  // which will produce the expected behaviour (apart from drag events
+  // received so far being dropped).
+  if (AsDragBlock() && aForScrollbarDrag &&
+      mTargetConfirmed == TargetConfirmationState::eConfirmed &&
+      aState == TargetConfirmationState::eConfirmed &&
+      mTargetApzc && aTargetApzc &&
+      mTargetApzc->GetGuid() != aTargetApzc->GetGuid()) {
+    MOZ_DIAGNOSTIC_ASSERT(false, "APZ and main thread confirmed scrollbar drag block with different targets");
+    UpdateTargetApzc(aTargetApzc);
+    return true;
+  }
+
   if (mTargetConfirmed != TargetConfirmationState::eUnconfirmed) {
     return false;
   }
   mTargetConfirmed = aState;
 
   TBS_LOG("%p got confirmed target APZC %p\n", this, mTargetApzc.get());
   if (mTargetApzc == aTargetApzc) {
     // The confirmed target is the same as the tentative one, so we're done.
@@ -358,28 +379,29 @@ WheelBlockState::SetContentResponse(bool
     EndTransaction();
   }
   return CancelableBlockState::SetContentResponse(aPreventDefault);
 }
 
 bool
 WheelBlockState::SetConfirmedTargetApzc(const RefPtr<AsyncPanZoomController>& aTargetApzc,
                                         TargetConfirmationState aState,
-                                        InputData* aFirstInput)
+                                        InputData* aFirstInput,
+                                        bool aForScrollbarDrag)
 {
   // The APZC that we find via APZCCallbackHelpers may not be the same APZC
   // ESM or OverscrollHandoff would have computed. Make sure we get the right
   // one by looking for the first apzc the next pending event can scroll.
   RefPtr<AsyncPanZoomController> apzc = aTargetApzc;
   if (apzc && aFirstInput) {
     apzc = apzc->BuildOverscrollHandoffChain()->FindFirstScrollable(
         *aFirstInput, &mAllowedScrollDirections);
   }
 
-  InputBlockState::SetConfirmedTargetApzc(apzc, aState, aFirstInput);
+  InputBlockState::SetConfirmedTargetApzc(apzc, aState, aFirstInput, aForScrollbarDrag);
   return true;
 }
 
 void
 WheelBlockState::Update(ScrollWheelInput& aEvent)
 {
   // We might not be in a transaction if the block never started in a
   // transaction - for example, if nothing was scrollable.
@@ -575,32 +597,33 @@ PanGestureBlockState::PanGestureBlockSta
       UpdateTargetApzc(apzc);
     }
   }
 }
 
 bool
 PanGestureBlockState::SetConfirmedTargetApzc(const RefPtr<AsyncPanZoomController>& aTargetApzc,
                                              TargetConfirmationState aState,
-                                             InputData* aFirstInput)
+                                             InputData* aFirstInput,
+                                             bool aForScrollbarDrag)
 {
   // The APZC that we find via APZCCallbackHelpers may not be the same APZC
   // ESM or OverscrollHandoff would have computed. Make sure we get the right
   // one by looking for the first apzc the next pending event can scroll.
   RefPtr<AsyncPanZoomController> apzc = aTargetApzc;
   if (apzc && aFirstInput) {
     RefPtr<AsyncPanZoomController> scrollableApzc =
       apzc->BuildOverscrollHandoffChain()->FindFirstScrollable(
           *aFirstInput, &mAllowedScrollDirections);
     if (scrollableApzc) {
       apzc = scrollableApzc;
     }
   }
 
-  InputBlockState::SetConfirmedTargetApzc(apzc, aState, aFirstInput);
+  InputBlockState::SetConfirmedTargetApzc(apzc, aState, aFirstInput, aForScrollbarDrag);
   return true;
 }
 
 bool
 PanGestureBlockState::MustStayActive()
 {
   return !mInterrupted;
 }
--- a/gfx/layers/apz/src/InputBlockState.h
+++ b/gfx/layers/apz/src/InputBlockState.h
@@ -70,17 +70,18 @@ public:
     return nullptr;
   }
   virtual KeyboardBlockState* AsKeyboardBlock() {
     return nullptr;
   }
 
   virtual bool SetConfirmedTargetApzc(const RefPtr<AsyncPanZoomController>& aTargetApzc,
                                       TargetConfirmationState aState,
-                                      InputData* aFirstInput);
+                                      InputData* aFirstInput,
+                                      bool aForScrollbarDrag);
   const RefPtr<AsyncPanZoomController>& GetTargetApzc() const;
   const RefPtr<const OverscrollHandoffChain>& GetOverscrollHandoffChain() const;
   uint64_t GetBlockId() const;
 
   bool IsTargetConfirmed() const;
   bool HasReceivedRealConfirmedTarget() const;
 
   virtual bool ShouldDropEvents() const;
@@ -225,17 +226,18 @@ public:
                   TargetConfirmationFlags aFlags,
                   const ScrollWheelInput& aEvent);
 
   bool SetContentResponse(bool aPreventDefault) override;
   bool MustStayActive() override;
   const char* Type() override;
   bool SetConfirmedTargetApzc(const RefPtr<AsyncPanZoomController>& aTargetApzc,
                               TargetConfirmationState aState,
-                              InputData* aFirstInput) override;
+                              InputData* aFirstInput,
+                              bool aForScrollbarDrag) override;
 
   WheelBlockState *AsWheelBlock() override {
     return this;
   }
 
   /**
    * Determine whether this wheel block is accepting new events.
    */
@@ -341,17 +343,18 @@ public:
 
   bool SetContentResponse(bool aPreventDefault) override;
   bool HasReceivedAllContentNotifications() const override;
   bool IsReadyForHandling() const override;
   bool MustStayActive() override;
   const char* Type() override;
   bool SetConfirmedTargetApzc(const RefPtr<AsyncPanZoomController>& aTargetApzc,
                               TargetConfirmationState aState,
-                              InputData* aFirstInput) override;
+                              InputData* aFirstInput,
+                              bool aForScrollbarDrag) override;
 
   PanGestureBlockState *AsPanGestureBlock() override {
     return this;
   }
 
   /**
    * @return Whether or not overscrolling is prevented for this block.
    */
--- a/gfx/layers/apz/src/InputQueue.cpp
+++ b/gfx/layers/apz/src/InputQueue.cpp
@@ -109,17 +109,18 @@ InputQueue::ReceiveTouchInput(const RefP
       // If we're already in a fast fling, and a single finger goes down, then
       // we want special handling for the touch event, because it shouldn't get
       // delivered to content. Note that we don't set this flag when going
       // from a fast fling to a pinch state (i.e. second finger goes down while
       // the first finger is moving).
       block->SetDuringFastFling();
       block->SetConfirmedTargetApzc(aTarget,
           InputBlockState::TargetConfirmationState::eConfirmed,
-          nullptr /* the block was just created so it has no events */);
+          nullptr /* the block was just created so it has no events */,
+          false /* not a scrollbar drag */);
       if (gfxPrefs::TouchActionEnabled()) {
         block->SetAllowedTouchBehaviors(currentBehaviors);
       }
       INPQ_LOG("block %p tagged as fast-motion\n", block);
     }
 
     CancelAnimationsForNewBlock(block);
 
@@ -637,17 +638,22 @@ InputQueue::MainThreadTimeout(uint64_t a
     CancelableBlockState* block = inputBlock->AsCancelableBlock();
     // time out the touch-listener response and also confirm the existing
     // target apzc in the case where the main thread doesn't get back to us
     // fast enough.
     success = block->TimeoutContentResponse();
     success |= block->SetConfirmedTargetApzc(
         block->GetTargetApzc(),
         InputBlockState::TargetConfirmationState::eTimedOut,
-        firstInput);
+        firstInput,
+        // This actually could be a scrollbar drag, but we pass
+        // aForScrollbarDrag=false because for scrollbar drags,
+        // SetConfirmedTargetApzc() will also be called by ConfirmDragBlock(),
+        // and we pass aForScrollbarDrag=true there.
+        false);
   } else if (inputBlock) {
     NS_WARNING("input block is not a cancelable block");
   }
   if (success) {
     ProcessQueue();
   }
 }
 
@@ -678,17 +684,22 @@ InputQueue::SetConfirmedTargetApzc(uint6
     aInputBlockId, aTargetApzc ? Stringify(aTargetApzc->GetGuid()).c_str() : "");
   bool success = false;
   InputData* firstInput = nullptr;
   InputBlockState* inputBlock = FindBlockForId(aInputBlockId, &firstInput);
   if (inputBlock && inputBlock->AsCancelableBlock()) {
     CancelableBlockState* block = inputBlock->AsCancelableBlock();
     success = block->SetConfirmedTargetApzc(aTargetApzc,
         InputBlockState::TargetConfirmationState::eConfirmed,
-        firstInput);
+        firstInput,
+        // This actually could be a scrollbar drag, but we pass
+        // aForScrollbarDrag=false because for scrollbar drags,
+        // SetConfirmedTargetApzc() will also be called by ConfirmDragBlock(),
+        // and we pass aForScrollbarDrag=true there.
+        false);
     block->RecordContentResponseTime();
   } else if (inputBlock) {
     NS_WARNING("input block is not a cancelable block");
   }
   if (success) {
     ProcessQueue();
   }
 }
@@ -704,17 +715,18 @@ InputQueue::ConfirmDragBlock(uint64_t aI
   bool success = false;
   InputData* firstInput = nullptr;
   InputBlockState* inputBlock = FindBlockForId(aInputBlockId, &firstInput);
   if (inputBlock && inputBlock->AsDragBlock()) {
     DragBlockState* block = inputBlock->AsDragBlock();
     block->SetDragMetrics(aDragMetrics);
     success = block->SetConfirmedTargetApzc(aTargetApzc,
         InputBlockState::TargetConfirmationState::eConfirmed,
-        firstInput);
+        firstInput,
+        /* aForScrollbarDrag = */ true);
     block->RecordContentResponseTime();
   }
   if (success) {
     ProcessQueue();
   }
 }
 
 void