Bug 1433671 - Make AccessibleCaretManager flushes a bit more sound. r=TYLin, a=RyanVM
authorEmilio Cobos Álvarez <emilio@crisal.io>
Mon, 05 Feb 2018 15:27:35 -0500
changeset 454664 3d51ba1a7c3fdffd5f44540038f11def9832fc96
parent 454663 e83f82bb996219e9e64e05d17d489ba4d9d63f64
child 454665 711b8a407ffd287573fa14d5ca27631d47b8c413
push id1648
push usermtabara@mozilla.com
push dateThu, 01 Mar 2018 12:45:47 +0000
treeherdermozilla-release@cbb9688c2eeb [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersTYLin, RyanVM
bugs1433671
milestone59.0
Bug 1433671 - Make AccessibleCaretManager flushes a bit more sound. r=TYLin, a=RyanVM The accessible caret manager is owned by the event hub, that is owned by the shell. All the callers of methods that call FlushLayout on the AccessibleCaretManager should hold an external reference to the event hub. Flushing pending notifications can run arbitrary script, that can call Destroy() on the pres shell (and thus tear down the accessible caret event hub, and the manager with him). I don't know why before my change this wasn't crashing badly, but the code as it was just doesn't look sound to me at all either (maybe I'm misunderstanding something and I should just revert that patch and give up on having nice invariants during our flushes..., but I don't think it's the case). This also adds some sanity-checking that we don't die under our flush. MozReview-Commit-ID: 4s0UT0fD3TI
layout/base/AccessibleCaretEventHub.cpp
layout/base/AccessibleCaretManager.cpp
layout/base/AccessibleCaretManager.h
layout/base/gtest/TestAccessibleCaretManager.cpp
--- a/layout/base/AccessibleCaretEventHub.cpp
+++ b/layout/base/AccessibleCaretEventHub.cpp
@@ -666,17 +666,18 @@ AccessibleCaretEventHub::CancelLongTapIn
 
   mLongTapInjectorTimer->Cancel();
 }
 
 /* static */ void
 AccessibleCaretEventHub::FireLongTap(nsITimer* aTimer,
                                      void* aAccessibleCaretEventHub)
 {
-  auto* self = static_cast<AccessibleCaretEventHub*>(aAccessibleCaretEventHub);
+  RefPtr<AccessibleCaretEventHub> self =
+    static_cast<AccessibleCaretEventHub*>(aAccessibleCaretEventHub);
   self->mState->OnLongTap(self, self->mPressPoint);
 }
 
 NS_IMETHODIMP
 AccessibleCaretEventHub::Reflow(DOMHighResTimeStamp aStart,
                                 DOMHighResTimeStamp aEnd)
 {
   if (!mInitialized) {
@@ -768,17 +769,18 @@ AccessibleCaretEventHub::CancelScrollEnd
 
   mScrollEndInjectorTimer->Cancel();
 }
 
 /* static */ void
 AccessibleCaretEventHub::FireScrollEnd(nsITimer* aTimer,
                                        void* aAccessibleCaretEventHub)
 {
-  auto* self = static_cast<AccessibleCaretEventHub*>(aAccessibleCaretEventHub);
+  RefPtr<AccessibleCaretEventHub> self =
+    static_cast<AccessibleCaretEventHub*>(aAccessibleCaretEventHub);
   self->mState->OnScrollEnd(self);
 }
 
 nsresult
 AccessibleCaretEventHub::NotifySelectionChanged(nsIDOMDocument* aDoc,
                                                 nsISelection* aSel,
                                                 int16_t aReason)
 {
--- a/layout/base/AccessibleCaretManager.cpp
+++ b/layout/base/AccessibleCaretManager.cpp
@@ -117,16 +117,21 @@ AccessibleCaretManager::AccessibleCaretM
     Preferences::AddBoolVarCache(&sExtendSelectionForPhoneNumber,
       "layout.accessiblecaret.extend_selection_for_phone_number");
     Preferences::AddBoolVarCache(&sHideCaretsForMouseInput,
       "layout.accessiblecaret.hide_carets_for_mouse_input");
     addedPrefs = true;
   }
 }
 
+AccessibleCaretManager::~AccessibleCaretManager()
+{
+  MOZ_RELEASE_ASSERT(!mFlushingLayout, "Going away in FlushLayout? Bad!");
+}
+
 void
 AccessibleCaretManager::Terminate()
 {
   mFirstCaret = nullptr;
   mSecondCaret = nullptr;
   mActiveCaret = nullptr;
   mPresShell = nullptr;
 }
@@ -212,18 +217,17 @@ AccessibleCaretManager::HideCarets()
     mSecondCaret->SetAppearance(Appearance::None);
     DispatchCaretStateChangedEvent(CaretChangedReason::Visibilitychange);
   }
 }
 
 void
 AccessibleCaretManager::UpdateCarets(const UpdateCaretsHintSet& aHint)
 {
-  FlushLayout();
-  if (IsTerminated()) {
+  if (!FlushLayout()) {
     return;
   }
 
   mLastUpdateCaretMode = GetCaretMode();
 
   switch (mLastUpdateCaretMode) {
   case CaretMode::None:
     HideCarets();
@@ -380,18 +384,17 @@ AccessibleCaretManager::UpdateCaretsForS
   PositionChangedResult firstCaretResult =
     updateSingleCaret(mFirstCaret.get(), startFrame, startOffset);
   PositionChangedResult secondCaretResult =
     updateSingleCaret(mSecondCaret.get(), endFrame, endOffset);
 
   if (firstCaretResult == PositionChangedResult::Changed ||
       secondCaretResult == PositionChangedResult::Changed) {
     // Flush layout to make the carets intersection correct.
-    FlushLayout();
-    if (IsTerminated()) {
+    if (!FlushLayout()) {
       return;
     }
   }
 
   if (aHints == UpdateCaretsHint::Default) {
     // Only check for tilt carets with default update hint. Otherwise we might
     // override the appearance set by the caller.
     if (sCaretsAlwaysTilt) {
@@ -1020,22 +1023,27 @@ AccessibleCaretManager::ClearMaintainedS
   // Selection made by double-clicking for example will maintain the original
   // word selection. We should clear it so that we can drag caret freely.
   RefPtr<nsFrameSelection> fs = GetFrameSelection();
   if (fs) {
     fs->MaintainSelection(eSelectNoAmount);
   }
 }
 
-void
-AccessibleCaretManager::FlushLayout() const
+bool
+AccessibleCaretManager::FlushLayout()
 {
   if (mPresShell) {
+    AutoRestore<bool> flushing(mFlushingLayout);
+    mFlushingLayout = true;
+
     mPresShell->FlushPendingNotifications(FlushType::Layout);
   }
+
+  return !IsTerminated();
 }
 
 nsIFrame*
 AccessibleCaretManager::GetFrameForFirstRangeStartOrLastRangeEnd(
   nsDirection aDirection,
   int32_t* aOutOffset,
   nsIContent** aOutContent,
   int32_t* aOutContentOffset) const
@@ -1397,24 +1405,19 @@ void
 AccessibleCaretManager::StopSelectionAutoScrollTimer() const
 {
   RefPtr<nsFrameSelection> fs = GetFrameSelection();
   MOZ_ASSERT(fs);
   fs->StopAutoScrollTimer();
 }
 
 void
-AccessibleCaretManager::DispatchCaretStateChangedEvent(CaretChangedReason aReason) const
+AccessibleCaretManager::DispatchCaretStateChangedEvent(CaretChangedReason aReason)
 {
-  if (!mPresShell) {
-    return;
-  }
-
-  FlushLayout();
-  if (IsTerminated()) {
+  if (!FlushLayout()) {
     return;
   }
 
   Selection* sel = GetSelection();
   if (!sel) {
     return;
   }
 
--- a/layout/base/AccessibleCaretManager.h
+++ b/layout/base/AccessibleCaretManager.h
@@ -44,17 +44,17 @@ class Selection;
 //
 // Please see the wiki page for more information.
 // https://wiki.mozilla.org/AccessibleCaret
 //
 class AccessibleCaretManager
 {
 public:
   explicit AccessibleCaretManager(nsIPresShell* aPresShell);
-  virtual ~AccessibleCaretManager() = default;
+  virtual ~AccessibleCaretManager();
 
   // Called by AccessibleCaretEventHub to inform us that PresShell is destroyed.
   void Terminate();
 
   // The aPoint in the following public methods should be relative to root
   // frame.
 
   // Press caret on the given point. Return NS_OK if the point is actually on
@@ -194,19 +194,23 @@ protected:
 
   // Start the selection scroll timer if the caret is being dragged out of
   // the scroll port.
   void StartSelectionAutoScrollTimer(const nsPoint& aPoint) const;
   void StopSelectionAutoScrollTimer() const;
 
   void ClearMaintainedSelection() const;
 
-  // Caller is responsible to use IsTerminated() to check whether PresShell is
-  // still valid.
-  void FlushLayout() const;
+  // This method could kill the shell, so callers to methods that call
+  // FlushLayout should ensure the event hub that owns us is still alive.
+  //
+  // See the mRefCnt assertions in AccessibleCaretEventHub.
+  //
+  // Returns whether mPresShell we're holding is still valid.
+  MOZ_MUST_USE bool FlushLayout();
 
   dom::Element* GetEditingHostForFrame(nsIFrame* aFrame) const;
   dom::Selection* GetSelection() const;
   already_AddRefed<nsFrameSelection> GetFrameSelection() const;
   nsAutoString StringifiedSelection() const;
 
   // Get the union of all the child frame scrollable overflow rects for aFrame,
   // which is used as a helper function to restrict the area where the caret can
@@ -252,17 +256,17 @@ protected:
   // @param aOutOffset returns frame offset as well.
   virtual bool IsCaretDisplayableInCursorMode(nsIFrame** aOutFrame = nullptr,
                                               int32_t* aOutOffset = nullptr) const;
 
   virtual bool HasNonEmptyTextContent(nsINode* aNode) const;
 
   // This function will flush layout, so caller must ensure the PresShell is
   // still valid after calling this method.
-  virtual void DispatchCaretStateChangedEvent(dom::CaretChangedReason aReason) const;
+  virtual void DispatchCaretStateChangedEvent(dom::CaretChangedReason aReason);
 
   // ---------------------------------------------------------------------------
   // Member variables
   //
   nscoord mOffsetYToCaretLogicalPosition = NS_UNCONSTRAINEDSIZE;
 
   // AccessibleCaretEventHub owns us by a UniquePtr. When it's destroyed, we'll
   // also be destroyed. No need to worry if we outlive mPresShell.
@@ -296,16 +300,19 @@ protected:
   // or not show the carets when the selection is updated, as we want to hide
   // the carets for mouse-triggered selection changes but show them for other
   // input types such as touch.
   uint16_t mLastInputSource = nsIDOMMouseEvent::MOZ_SOURCE_UNKNOWN;
 
   // Set to true in OnScrollStart() and set to false in OnScrollEnd().
   bool mIsScrollStarted = false;
 
+  // Whether we're flushing layout, used for sanity-checking.
+  bool mFlushingLayout = false;
+
   static const int32_t kAutoScrollTimerDelay = 30;
 
   // Clicking on the boundary of input or textarea will move the caret to the
   // front or end of the content. To avoid this, we need to deflate the content
   // boundary by 61 app units, which is 1 pixel + 1 app unit as defined in
   // AppUnit.h.
   static const int32_t kBoundaryAppUnits = 61;
 
--- a/layout/base/gtest/TestAccessibleCaretManager.cpp
+++ b/layout/base/gtest/TestAccessibleCaretManager.cpp
@@ -103,18 +103,18 @@ public:
       if (mSecondCaret->IsVisuallyVisible()) {
         mSecondCaret->SetAppearance(Appearance::Right);
       }
     }
 
     bool IsTerminated() const override { return false; }
 
     MOCK_CONST_METHOD0(GetCaretMode, CaretMode());
-    MOCK_CONST_METHOD1(DispatchCaretStateChangedEvent,
-                       void(CaretChangedReason aReason));
+    MOCK_METHOD1(DispatchCaretStateChangedEvent,
+                 void(CaretChangedReason aReason));
     MOCK_CONST_METHOD1(HasNonEmptyTextContent, bool(nsINode* aNode));
 
   }; // class MockAccessibleCaretManager
 
   using Appearance = AccessibleCaret::Appearance;
   using PositionChangedResult = AccessibleCaret::PositionChangedResult;
   using CaretMode = MockAccessibleCaretManager::CaretMode;