Bug 1479972 - nsRange should ignore mutations of the DOM tree while it's cached by Selection r=smaug

Selection caches an nsRange instance for saving re-allocation cost and AddMutationObserver() and RemoveMutationObserver()'s cost when its RemoveAllRangesTemporarily() is called.

Then, the instance is detached from the Selection but still referring editing point. E.g., the only text node in TextEditor when its value is set. Therefore, it'll receive character data change notification and need to check whether the point is still valid with new text.  However, the range will be always set new position later, i.e., immediately before going back to a part of Selection. Therefore, even if the point becomes invalid, nobody must not have any problems.

This patch makes Selection make the cached range not positioned, and makes nsRange ignore any mutations when it's not positioned.

Differential Revision: https://phabricator.services.mozilla.com/D2587

--HG--
extra : moz-landing-system : lando

dom/base/Selection.cpp

@@ -2104,6 +2104,10 @@ Selection::RemoveAllRangesTemporarily()
   RemoveAllRanges(result);
   if (result.Failed()) {
     mCachedRange = nullptr;
+  } else if (mCachedRange) {
+    // To save the computing cost to keep valid DOM point against DOM tree
+    // changes, we should clear the range temporarily.
+    mCachedRange->ResetTemporarily();
   }
   return result.StealNSResult();
 }

dom/base/Selection.h

@@ -662,6 +662,12 @@ private:
   // released by Clear(), RemoveAllRangesTemporarily() stores it with this.
   // If Collapse() is called without existing ranges, it'll reuse this range
   // for saving the creation cost.
+  // Note that while the range is cached by this, we keep the range being
+  // a mutation observer because it is not so cheap to register the range
+  // as a mutation observer again.  On the other hand, we make it not
+  // positioned because it is not so cheap to keep valid DOM point against
+  // mutations.  This does not cause any problems because we will set new
+  // DOM point when we treat it as a range of Selection again.
   RefPtr<nsRange> mCachedRange;
   RefPtr<nsFrameSelection> mFrameSelection;
   RefPtr<nsAutoScrollTimer> mAutoScrollTimer;

dom/base/nsRange.cpp

@@ -491,9 +491,15 @@ void
 nsRange::CharacterDataChanged(nsIContent* aContent,
                               const CharacterDataChangeInfo& aInfo)
 {
+  // If this is called when this is not positioned, it means that this range
+  // will be initialized again or destroyed soon.  See Selection::mCachedRange.
+  if (!mIsPositioned) {
+    MOZ_ASSERT(mRoot);
+    return;
+  }
+
   MOZ_ASSERT(!mNextEndRef);
   MOZ_ASSERT(!mNextStartRef);
-  MOZ_ASSERT(mIsPositioned, "shouldn't be notified if not positioned");
 
   nsINode* newRoot = nullptr;
   RawRangeBoundary newStart;
@@ -645,7 +651,12 @@ nsRange::CharacterDataChanged(nsIContent* aContent,
 void
 nsRange::ContentAppended(nsIContent*  aFirstNewContent)
 {
-  NS_ASSERTION(mIsPositioned, "shouldn't be notified if not positioned");
+  // If this is called when this is not positioned, it means that this range
+  // will be initialized again or destroyed soon.  See Selection::mCachedRange.
+  if (!mIsPositioned) {
+    MOZ_ASSERT(mRoot);
+    return;
+  }
 
   nsINode* container = aFirstNewContent->GetParentNode();
   MOZ_ASSERT(container);
@@ -680,7 +691,12 @@ nsRange::ContentAppended(nsIContent*  aFirstNewContent)
 void
 nsRange::ContentInserted(nsIContent* aChild)
 {
-  MOZ_ASSERT(mIsPositioned, "shouldn't be notified if not positioned");
+  // If this is called when this is not positioned, it means that this range
+  // will be initialized again or destroyed soon.  See Selection::mCachedRange.
+  if (!mIsPositioned) {
+    MOZ_ASSERT(mRoot);
+    return;
+  }
 
   bool updateBoundaries = false;
   nsINode* container = aChild->GetParentNode();
@@ -730,7 +746,13 @@ nsRange::ContentInserted(nsIContent* aChild)
 void
 nsRange::ContentRemoved(nsIContent* aChild, nsIContent* aPreviousSibling)
 {
-  MOZ_ASSERT(mIsPositioned, "shouldn't be notified if not positioned");
+  // If this is called when this is not positioned, it means that this range
+  // will be initialized again or destroyed soon.  See Selection::mCachedRange.
+  if (!mIsPositioned) {
+    MOZ_ASSERT(mRoot);
+    return;
+  }
+
   nsINode* container = aChild->GetParentNode();
   MOZ_ASSERT(container);
 
@@ -937,17 +959,17 @@ nsRange::DoSetRange(const RawRangeBoundary& aStart,
                     nsINode* aRoot, bool aNotInsertedYet)
 {
   MOZ_ASSERT((aStart.IsSet() && aEnd.IsSet() && aRoot) ||
-             (!aStart.IsSet() && !aEnd.IsSet() && !aRoot),
+             (!aStart.IsSet() && !aEnd.IsSet()),
              "Set all or none");
 
-  MOZ_ASSERT(!aRoot || aNotInsertedYet ||
+  MOZ_ASSERT(!aRoot || (!aStart.IsSet() && !aEnd.IsSet()) || aNotInsertedYet ||
              (nsContentUtils::ContentIsDescendantOf(aStart.Container(), aRoot) &&
               nsContentUtils::ContentIsDescendantOf(aEnd.Container(), aRoot) &&
               aRoot == IsValidBoundary(aStart.Container()) &&
               aRoot == IsValidBoundary(aEnd.Container())),
              "Wrong root");
 
-  MOZ_ASSERT(!aRoot ||
+  MOZ_ASSERT(!aRoot || (!aStart.IsSet() && !aEnd.IsSet()) ||
              (aStart.Container()->IsContent() &&
               aEnd.Container()->IsContent() &&
               aRoot ==

dom/base/nsRange.h

@@ -158,6 +158,17 @@ public:
   nsINode* GetCommonAncestor() const;
   void Reset();
 
+  /**
+   * ResetTemporarily() is called when Selection starts to cache the instance
+   * to reuse later.  This method clears mStart, mEnd and mIsPositioned but
+   * does not clear mRoot for reducing the cost to register this as a mutation
+   * observer again.
+   */
+  void ResetTemporarily()
+  {
+    DoSetRange(RawRangeBoundary(), RawRangeBoundary(), mRoot);
+  }
+
   /**
    * SetStart() and SetEnd() sets start point or end point separately.
    * However, this is expensive especially when it's a range of Selection.