Bug 1246918 - Handle PresShell gone after FlushLayout(). r=roc draft
authorTing-Yu Lin <tlin@mozilla.com>
Mon, 15 Feb 2016 15:12:35 +0800
changeset 331781 280e2512fb9f28d933f5b8d65a9f303ac81c58e5
parent 331780 0ef8d28ce55c3ddd29ea32ee6888ee7fe14c34ad
child 331782 94917bda7e83bf7f9c87b1e8479824cf519c8525
push id11083
push usertlin@mozilla.com
push dateThu, 18 Feb 2016 10:44:54 +0000
reviewersroc
bugs1246918
milestone47.0a1
Bug 1246918 - Handle PresShell gone after FlushLayout(). r=roc After calling FlushLayout(), PresShell::Destroy() might be called and we should consider PresShell and other resources will be no longer valid. Before this patch, AccessibleCaretManager and AccessibleCaret(s) are deallocated in PresShell::Destroy(). However FlushLayout() are all invoked in AccessibleCaretManager, we need to keep manager alive to clean up after PresShell::Destroy(). This patch makes AccessibleCaretManager live after PresShell::Destroy(), and use IsTerminated() to check whether PreShell is vaild after each FlushLayout() calls. Note that event though AccessibleCaretEventHub will be unref in PresShell::Destroy(), all the callers to AccessibleCaretEventHub's public methods already add a ref to AccessibleCaretEventHub. So we don't need to worry about AccessibleCaretEventHub and AccessibleCaretManager die immediately after PresShell::Destroy(). MozReview-Commit-ID: DDpXZ7v3zyo
layout/base/AccessibleCaret.h
layout/base/AccessibleCaretEventHub.cpp
layout/base/AccessibleCaretManager.cpp
layout/base/AccessibleCaretManager.h
layout/base/gtest/TestAccessibleCaretManager.cpp
--- a/layout/base/AccessibleCaret.h
+++ b/layout/base/AccessibleCaret.h
@@ -194,19 +194,19 @@ protected:
     virtual ~DummyTouchListener() {};
   };
 
   // Member variables
   Appearance mAppearance = Appearance::None;
 
   bool mSelectionBarEnabled = false;
 
-  // AccessibleCaretManager owns us. When it's destroyed by
+  // AccessibleCaretManager owns us by a UniquePtr. When it's terminated by
   // AccessibleCaretEventHub::Terminate() which is called in
-  // PresShell::Destroy(), it frees us automatically. No need to worry we
+  // PresShell::Destroy(), it frees us automatically. No need to worry if we
   // outlive mPresShell.
   nsIPresShell* MOZ_NON_OWNING_REF const mPresShell = nullptr;
 
   RefPtr<dom::AnonymousContent> mCaretElementHolder;
 
   // mImaginaryCaretRect is relative to root frame.
   nsRect mImaginaryCaretRect;
 
--- a/layout/base/AccessibleCaretEventHub.cpp
+++ b/layout/base/AccessibleCaretEventHub.cpp
@@ -454,17 +454,17 @@ AccessibleCaretEventHub::Terminate()
   if (mLongTapInjectorTimer) {
     mLongTapInjectorTimer->Cancel();
   }
 
   if (mScrollEndInjectorTimer) {
     mScrollEndInjectorTimer->Cancel();
   }
 
-  mManager = nullptr;
+  mManager->Terminate();
   mPresShell = nullptr;
   mInitialized = false;
 }
 
 nsEventStatus
 AccessibleCaretEventHub::HandleEvent(WidgetEvent* aEvent)
 {
   nsEventStatus status = nsEventStatus_eIgnore;
--- a/layout/base/AccessibleCaretManager.cpp
+++ b/layout/base/AccessibleCaretManager.cpp
@@ -100,17 +100,27 @@ AccessibleCaretManager::AccessibleCaretM
     Preferences::AddBoolVarCache(&sHapticFeedback,
                                  "layout.accessiblecaret.hapticfeedback");
     addedPrefs = true;
   }
 }
 
 AccessibleCaretManager::~AccessibleCaretManager()
 {
+}
+
+void
+AccessibleCaretManager::Terminate()
+{
   CancelCaretTimeoutTimer();
+  mCaretTimeoutTimer = nullptr;
+  mFirstCaret = nullptr;
+  mSecondCaret = nullptr;
+  mActiveCaret = nullptr;
+  mPresShell = nullptr;
 }
 
 nsresult
 AccessibleCaretManager::OnSelectionChanged(nsIDOMDocument* aDoc,
                                            nsISelection* aSel, int16_t aReason)
 {
   Selection* selection = GetSelection();
   AC_LOG("%s: aSel: %p, GetSelection(): %p, aReason: %d", __FUNCTION__,
@@ -127,17 +137,16 @@ AccessibleCaretManager::OnSelectionChang
     return NS_OK;
   }
 
   // Move the cursor by Javascript / or unknown internal.
   if (aReason == nsISelectionListener::NO_REASON) {
     // Update visible carets, if javascript changes are allowed.
     if (sCaretsScriptUpdates &&
         (mFirstCaret->IsLogicallyVisible() || mSecondCaret->IsLogicallyVisible())) {
-        FlushLayout();
         UpdateCarets();
         return NS_OK;
     }
     // Default for NO_REASON is to make hidden.
     HideCarets();
     return NS_OK;
   }
 
@@ -187,16 +196,21 @@ AccessibleCaretManager::DoNotShowCarets(
     DispatchCaretStateChangedEvent(CaretChangedReason::Visibilitychange);
     CancelCaretTimeoutTimer();
   }
 }
 
 void
 AccessibleCaretManager::UpdateCarets(UpdateCaretsHint aHint)
 {
+  FlushLayout();
+  if (IsTerminated()) {
+    return;
+  }
+
   mLastUpdateCaretMode = GetCaretMode();
 
   switch (mLastUpdateCaretMode) {
   case CaretMode::None:
     HideCarets();
     break;
   case CaretMode::Cursor:
     UpdateCaretsForCursorMode(aHint);
@@ -367,16 +381,19 @@ AccessibleCaretManager::UpdateCaretsForS
     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()) {
+      return;
+    }
   }
 
   if (aHint == UpdateCaretsHint::Default) {
     // Only check for tilt carets with default update hint. Otherwise we might
     // override the appearance set by the caller.
     UpdateCaretsForTilt();
   }
 
@@ -589,21 +606,16 @@ AccessibleCaretManager::OnScrollEnd()
 {
   if (mLastUpdateCaretMode != GetCaretMode()) {
     return;
   }
 
   mFirstCaret->SetAppearance(mFirstCaretAppearanceOnScrollStart);
   mSecondCaret->SetAppearance(mSecondCaretAppearanceOnScrollStart);
 
-  // Flush layout to make the carets intersection correct since we turn the
-  // appearance of the carets from None or NormalNotShown into something
-  // visible.
-  FlushLayout();
-
   if (GetCaretMode() == CaretMode::Cursor) {
     if (!mFirstCaret->IsLogicallyVisible()) {
       // If the caret is hidden (Appearance::None) due to timeout or blur, no
       // need to update it.
       return;
     }
   }
 
@@ -1127,17 +1139,17 @@ AccessibleCaretManager::CaretTimeoutMs()
   }
 
   return caretTimeoutMs;
 }
 
 void
 AccessibleCaretManager::LaunchCaretTimeoutTimer()
 {
-  if (!mCaretTimeoutTimer || CaretTimeoutMs() == 0 ||
+  if (!mPresShell || !mCaretTimeoutTimer || CaretTimeoutMs() == 0 ||
       GetCaretMode() != CaretMode::Cursor || mActiveCaret) {
     return;
   }
 
   nsTimerCallbackFunc callback = [](nsITimer* aTimer, void* aClosure) {
     auto self = static_cast<AccessibleCaretManager*>(aClosure);
     if (self->GetCaretMode() == CaretMode::Cursor) {
       self->HideCarets();
@@ -1154,21 +1166,22 @@ AccessibleCaretManager::CancelCaretTimeo
   if (mCaretTimeoutTimer) {
     mCaretTimeoutTimer->Cancel();
   }
 }
 
 void
 AccessibleCaretManager::DispatchCaretStateChangedEvent(CaretChangedReason aReason) const
 {
-  // Holding PresShell to prevent AccessibleCaretManager to be destroyed.
-  nsCOMPtr<nsIPresShell> presShell = mPresShell;
+  if (!mPresShell) {
+    return;
+  }
 
   FlushLayout();
-  if (presShell->IsDestroying()) {
+  if (IsTerminated()) {
     return;
   }
 
   Selection* sel = GetSelection();
   if (!sel) {
     return;
   }
 
--- a/layout/base/AccessibleCaretManager.h
+++ b/layout/base/AccessibleCaretManager.h
@@ -45,16 +45,19 @@ class Selection;
 // https://wiki.mozilla.org/Copy_n_Paste
 //
 class AccessibleCaretManager
 {
 public:
   explicit AccessibleCaretManager(nsIPresShell* aPresShell);
   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
   // one of the carets.
   virtual nsresult PressCaret(const nsPoint& aPoint);
 
   // Drag caret to the given point. It's required to call PressCaret()
@@ -119,17 +122,19 @@ protected:
     // the caret in cursor mode is hidden due to timeout, do not change its
     // appearance to Normal.
     RespectOldAppearance
   };
 
   friend std::ostream& operator<<(std::ostream& aStream,
                                   const UpdateCaretsHint& aResult);
 
-  // Update carets based on current selection status.
+  // Update carets based on current selection status. This function will flush
+  // layout, so caller must ensure the PresShell is still valid after calling
+  // this method.
   void UpdateCarets(UpdateCaretsHint aHint = UpdateCaretsHint::Default);
 
   // Force hiding all carets regardless of the current selection status.
   void HideCarets();
 
   // Force carets to be "present" logically, but not visible. Allows ActionBar
   // to stay open when carets visibility is supressed during scroll.
   void DoNotShowCarets();
@@ -155,17 +160,21 @@ protected:
   // If aBackward is false, find the first node from the first range in current
   // selection, and return the frame and the offset into that frame. If aBackward
   // is true, find the last node from the last range instead.
   nsIFrame* FindFirstNodeWithFrame(bool aBackward, int32_t* aOutOffset) const;
 
   nsresult DragCaretInternal(const nsPoint& aPoint);
   nsPoint AdjustDragBoundary(const nsPoint& aPoint) const;
   void ClearMaintainedSelection() const;
+
+  // Caller is responsible to use IsTerminated() to check whether PresShell is
+  // still valid.
   void FlushLayout() const;
+
   dom::Element* GetEditingHostForFrame(nsIFrame* aFrame) const;
   dom::Selection* GetSelection() const;
   already_AddRefed<nsFrameSelection> GetFrameSelection() const;
 
   // Get the bounding rectangle for aFrame where the caret under cursor mode can
   // be positioned. The rectangle is relative to the root frame.
   nsRect GetContentBoundaryForFrame(nsIFrame* aFrame) const;
 
@@ -179,16 +188,19 @@ protected:
   // no one touches it.
   uint32_t CaretTimeoutMs() const;
   void LaunchCaretTimeoutTimer();
   void CancelCaretTimeoutTimer();
 
   // ---------------------------------------------------------------------------
   // The following functions are made virtual for stubbing or mocking in gtest.
   //
+  // @return true if Terminate() had been called.
+  virtual bool IsTerminated() const { return !mPresShell; }
+
   // Get caret mode based on current selection.
   virtual CaretMode GetCaretMode() const;
 
   // @return true if aStartFrame comes before aEndFrame.
   virtual bool CompareTreePosition(nsIFrame* aStartFrame,
                                    nsIFrame* aEndFrame) const;
 
   // Check if the two carets is overlapping to become tilt.
@@ -197,29 +209,31 @@ protected:
   // Check whether AccessibleCaret is displayable in cursor mode or not.
   // @param aOutFrame returns frame of the cursor if it's displayable.
   // @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 call FlushPendingNotifications. So caller must ensure
-  // everything exists after calling this method.
+  // 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;
 
   // ---------------------------------------------------------------------------
   // Member variables
   //
   nscoord mOffsetYToCaretLogicalPosition = NS_UNCONSTRAINEDSIZE;
 
-  // AccessibleCaretEventHub owns us. When it's Terminate() called by
-  // PresShell::Destroy(), we will be destroyed. No need to worry we outlive
-  // mPresShell.
-  nsIPresShell* MOZ_NON_OWNING_REF const mPresShell = nullptr;
+  // AccessibleCaretEventHub owns us by a UniquePtr. When it's destroyed, we'll
+  // also be destroyed. No need to worry if we outlive mPresShell.
+  //
+  // mPresShell will be set to nullptr in Terminate(). Therefore mPresShell is
+  // nullptr either we are in gtest or PresShell::IsDestroying() is true.
+  nsIPresShell* MOZ_NON_OWNING_REF mPresShell = nullptr;
 
   // First caret is attached to nsCaret in cursor mode, and is attached to
   // selection highlight as the left caret in selection mode.
   UniquePtr<AccessibleCaret> mFirstCaret;
 
   // Second caret is used solely in selection mode, and is attached to selection
   // highlight as the right caret.
   UniquePtr<AccessibleCaret> mSecondCaret;
--- a/layout/base/gtest/TestAccessibleCaretManager.cpp
+++ b/layout/base/gtest/TestAccessibleCaretManager.cpp
@@ -88,16 +88,18 @@ public:
     virtual bool IsCaretDisplayableInCursorMode(
       nsIFrame** aOutFrame = nullptr, int32_t* aOutOffset = nullptr) const override
     {
       return true;
     }
 
     virtual void UpdateCaretsForTilt() override {}
 
+    virtual bool IsTerminated() const override { return false; }
+
     MOCK_CONST_METHOD0(GetCaretMode, CaretMode());
     MOCK_CONST_METHOD1(DispatchCaretStateChangedEvent,
                        void(CaretChangedReason aReason));
     MOCK_CONST_METHOD1(HasNonEmptyTextContent, bool(nsINode* aNode));
 
   }; // class MockAccessibleCaretManager
 
   using Appearance = AccessibleCaret::Appearance;