Bug 1408544 - part 2: RangeBoundaryBase shouldn't compute mRef when mOffset is specified r=catalinb
authorMasayuki Nakano <masayuki@d-toybox.com>
Thu, 02 Nov 2017 21:25:14 +0900
changeset 443725 2a2bb9c3b9a867ae3924dc51bbd6c8c74dbe003c
parent 443724 f03a38c18b12e4fb8e715c3cf46999aa59b00ce5
child 443726 b023fdb6af095e8404f44f2cec33c9ae9a8231f3
push id1618
push userCallek@gmail.com
push dateThu, 11 Jan 2018 17:45:48 +0000
treeherdermozilla-release@882ca853e05a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerscatalinb
bugs1408544
milestone58.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1408544 - part 2: RangeBoundaryBase shouldn't compute mRef when mOffset is specified r=catalinb RangeBoundaryBase shouldn't compute mRef when it's initialized with offset. E.g., some users of the template class may initialize it with a container and offset in it but it may not refer mRef or may refer after modifying offset. On the other hand, RangeBoundaryBase::InvalidateOffset() is a special method. It assumes that mRef is always initialized when it's called but can be invalidate mOffset until retrieved actually. This is necessary for nsRange::mStart and nsRange::mEnd since the offset may be changed when some nodes are inserted before the referring node. So, now, InvalidateOffset() should be a protected method and make nsRange a friend class of RangeBoundaryBase. Then, when nsRange sets mStart and/or mEnd, nsRange itself should guarantee that their mRefs are initialized. MozReview-Commit-ID: Alr4YkDXIND
dom/base/RangeBoundary.h
dom/base/nsRange.cpp
editor/libeditor/EditorBase.cpp
editor/libeditor/HTMLEditRules.cpp
--- a/dom/base/RangeBoundary.h
+++ b/dom/base/RangeBoundary.h
@@ -6,16 +6,18 @@
 
 #ifndef mozilla_RangeBoundary_h
 #define mozilla_RangeBoundary_h
 
 #include "nsCOMPtr.h"
 #include "nsIContent.h"
 #include "mozilla/Maybe.h"
 
+class nsRange;
+
 namespace mozilla {
 
 // This class will maintain a reference to the child immediately
 // before the boundary's offset. We try to avoid computing the
 // offset as much as possible and just ensure mRef points to the
 // correct child.
 //
 // mParent
@@ -38,16 +40,20 @@ typedef RangeBoundaryBase<nsINode*, nsIC
 // pointers and one using raw pointers. This helps us avoid unnecessary
 // AddRef/Release calls.
 template<typename ParentType, typename RefType>
 class RangeBoundaryBase
 {
   template<typename T, typename U>
   friend class RangeBoundaryBase;
 
+  // nsRange needs to use InvalidOffset() which requires mRef initialized
+  // before it's called.
+  friend class ::nsRange;
+
   friend void ImplCycleCollectionTraverse(nsCycleCollectionTraversalCallback&,
                                           RangeBoundary&, const char*,
                                           uint32_t);
   friend void ImplCycleCollectionUnlink(RangeBoundary&);
 
 public:
   RangeBoundaryBase(nsINode* aContainer, nsIContent* aRef)
     : mParent(aContainer)
@@ -62,30 +68,19 @@ public:
     }
   }
 
   RangeBoundaryBase(nsINode* aContainer, int32_t aOffset)
     : mParent(aContainer)
     , mRef(nullptr)
     , mOffset(mozilla::Some(aOffset))
   {
-    if (mParent && mParent->IsContainerNode()) {
-      // Find a reference node
-      if (aOffset == static_cast<int32_t>(aContainer->GetChildCount())) {
-        mRef = aContainer->GetLastChild();
-      } else if (aOffset != 0) {
-        mRef = mParent->GetChildAt(aOffset - 1);
-      }
-
-      NS_WARNING_ASSERTION(mRef || aOffset == 0,
-                           "Constructing RangeBoundary with invalid value");
+    if (!mParent) {
+      mOffset.reset();
     }
-
-    NS_WARNING_ASSERTION(!mRef || mRef->GetParentNode() == mParent,
-                         "Constructing RangeBoundary with invalid value");
   }
 
 protected:
   RangeBoundaryBase(nsINode* aContainer, nsIContent* aRef, int32_t aOffset)
     : mParent(aContainer)
     , mRef(aRef)
     , mOffset(mozilla::Some(aOffset))
   {
@@ -113,31 +108,33 @@ public:
     , mRef(aOther.mRef)
     , mOffset(aOther.mOffset)
   {
   }
 
   nsIContent*
   Ref() const
   {
+    EnsureRef();
     return mRef;
   }
 
   nsINode*
   Container() const
   {
     return mParent;
   }
 
   nsIContent*
   GetChildAtOffset() const
   {
     if (!mParent || !mParent->IsContainerNode()) {
       return nullptr;
     }
+    EnsureRef();
     if (!mRef) {
       MOZ_ASSERT(Offset() == 0, "invalid RangeBoundary");
       return mParent->GetFirstChild();
     }
     MOZ_ASSERT(mParent->GetChildAt(Offset()) == mRef->GetNextSibling());
     return mRef->GetNextSibling();
   }
 
@@ -147,16 +144,17 @@ public:
    * this returns nullptr with warning.
    */
   nsIContent*
   GetNextSiblingOfChildAtOffset() const
   {
     if (NS_WARN_IF(!mParent) || NS_WARN_IF(!mParent->IsContainerNode())) {
       return nullptr;
     }
+    EnsureRef();
     if (NS_WARN_IF(!mRef->GetNextSibling())) {
       // Already referring the end of the container.
       return nullptr;
     }
     return mRef->GetNextSibling()->GetNextSibling();
   }
 
   /**
@@ -165,96 +163,74 @@ public:
    * children, this returns nullptr with warning.
    */
   nsIContent*
   GetPreviousSiblingOfChildAtOffset() const
   {
     if (NS_WARN_IF(!mParent) || NS_WARN_IF(!mParent->IsContainerNode())) {
       return nullptr;
     }
+    EnsureRef();
     if (NS_WARN_IF(!mRef)) {
       // Already referring the start of the container.
       return nullptr;
     }
     return mRef;
   }
 
   uint32_t
   Offset() const
   {
     if (mOffset.isSome()) {
       return mOffset.value();
     }
-
     if (!mParent) {
+      MOZ_ASSERT(!mRef);
       return 0;
     }
-
+    MOZ_ASSERT(mParent->IsContainerNode(),
+      "If the container cannot have children, mOffset.isSome() should be true");
     MOZ_ASSERT(mRef);
     MOZ_ASSERT(mRef->GetParentNode() == mParent);
+    if (!mRef->GetPreviousSibling()) {
+      mOffset = mozilla::Some(1);
+      return mOffset.value();
+    }
+    if (!mRef->GetNextSibling()) {
+      mOffset = mozilla::Some(mParent->GetChildCount());
+      return mOffset.value();
+    }
+    // Use nsINode::IndexOf() as the last resort due to being expensive.
     mOffset = mozilla::Some(mParent->IndexOf(mRef) + 1);
-
     return mOffset.value();
   }
 
   void
-  InvalidateOffset()
-  {
-    MOZ_ASSERT(mParent);
-    MOZ_ASSERT(mParent->IsContainerNode(), "Range is positioned on a text node!");
-
-    if (!mRef) {
-      MOZ_ASSERT(mOffset.isSome() && mOffset.value() == 0,
-                 "Invalidating offset of invalid RangeBoundary?");
-      return;
-    }
-    mOffset.reset();
-  }
-
-  void
   Set(nsINode* aContainer, int32_t aOffset)
   {
     mParent = aContainer;
-    if (mParent && mParent->IsContainerNode()) {
-      // Find a reference node
-      if (aOffset == static_cast<int32_t>(aContainer->GetChildCount())) {
-        mRef = aContainer->GetLastChild();
-      } else if (aOffset == 0) {
-        mRef = nullptr;
-      } else {
-        mRef = mParent->GetChildAt(aOffset - 1);
-        MOZ_ASSERT(mRef);
-      }
-
-      NS_WARNING_ASSERTION(mRef || aOffset == 0,
-                           "Setting RangeBoundary to invalid value");
-    } else {
-      mRef = nullptr;
-    }
-
+    mRef = nullptr;
     mOffset = mozilla::Some(aOffset);
-
-    NS_WARNING_ASSERTION(!mRef || mRef->GetParentNode() == mParent,
-                         "Setting RangeBoundary to invalid value");
   }
 
   /**
    * AdvanceOffset() tries to reference next sibling of mRef if its container
    * can have children or increments offset if the container is a text node or
    * something.
    * If the container can have children and there is no next sibling, this
    * outputs warning and does nothing.  So, callers need to check if there is
    * next sibling which you need to refer.
    */
   void
   AdvanceOffset()
   {
     if (NS_WARN_IF(!mParent)) {
       return;
     }
+    EnsureRef();
     if (!mRef) {
       if (!mParent->IsContainerNode()) {
         // In text node or something, just increment the offset.
         MOZ_ASSERT(mOffset.isSome());
         if (NS_WARN_IF(mOffset.value() == mParent->Length())) {
           // Already referring the end of the node.
           return;
         }
@@ -291,16 +267,17 @@ public:
    * previous sibling which you need to refer.
    */
   void
   RewindOffset()
   {
     if (NS_WARN_IF(!mParent)) {
       return;
     }
+    EnsureRef();
     if (!mRef) {
       if (NS_WARN_IF(mParent->IsContainerNode())) {
         // Already referring the start of the container
         mOffset = mozilla::Some(0);
         return;
       }
       // In text node or something, just decrement the offset.
       MOZ_ASSERT(mOffset.isSome());
@@ -338,20 +315,23 @@ public:
 
   bool
   IsSetAndValid() const
   {
     if (!IsSet()) {
       return false;
     }
 
-    if (Ref()) {
-      return Ref()->GetParentNode() == Container();
+    if (mRef && mRef->GetParentNode() != mParent) {
+      return false;
     }
-    return Offset() <= Container()->Length();
+    if (mOffset.isSome() && mOffset.value() > mParent->Length()) {
+      return false;
+    }
+    return true;
   }
 
   bool
   IsStartOfContainer() const
   {
     // We're at the first point in the container if we don't have a reference,
     // and our offset is 0. If we don't have a Ref, we should already have an
     // offset, so we can just directly fetch it.
@@ -395,19 +375,60 @@ public:
   }
 
   template<typename A, typename B>
   bool operator!=(const RangeBoundaryBase<A, B>& aOther) const
   {
     return !(*this == aOther);
   }
 
+protected:
+  /**
+   * InvalidOffset() is error prone method, unfortunately.  If somebody
+   * needs to call this method, it needs to call EnsureRef() before changing
+   * the position of the referencing point.
+   */
+  void
+  InvalidateOffset()
+  {
+    MOZ_ASSERT(mParent);
+    MOZ_ASSERT(mParent->IsContainerNode(),
+               "Range is positioned on a text node!");
+    MOZ_ASSERT(mRef || (mOffset.isSome() && mOffset.value() == 0),
+               "mRef should be computed before a call of InvalidateOffset()");
+
+    if (!mRef) {
+      return;
+    }
+    mOffset.reset();
+  }
+
+  void
+  EnsureRef() const
+  {
+    if (mRef) {
+      return;
+    }
+    if (!mParent) {
+      MOZ_ASSERT(!mOffset.isSome());
+      return;
+    }
+    MOZ_ASSERT(mOffset.isSome());
+    MOZ_ASSERT(mOffset.value() <= mParent->Length());
+    if (!mParent->IsContainerNode() ||
+        mOffset.value() == 0) {
+      return;
+    }
+    mRef = mParent->GetChildAt(mOffset.value() - 1);
+    MOZ_ASSERT(mRef);
+  }
+
 private:
   ParentType mParent;
-  RefType mRef;
+  mutable RefType mRef;
 
   mutable mozilla::Maybe<uint32_t> mOffset;
 };
 
 inline void
 ImplCycleCollectionUnlink(RangeBoundary& aField)
 {
   ImplCycleCollectionUnlink(aField.mParent);
--- a/dom/base/nsRange.cpp
+++ b/dom/base/nsRange.cpp
@@ -1015,16 +1015,24 @@ nsRange::DoSetRange(const RawRangeBounda
     (mStart.Container() != aStart.Container() || mEnd.Container() != aEnd.Container()) &&
     IsInSelection() && !aNotInsertedYet;
 
   // GetCommonAncestor is unreliable while we're unlinking (could
   // return null if our start/end have already been unlinked), so make
   // sure to not use it here to determine our "old" current ancestor.
   mStart = aStart;
   mEnd = aEnd;
+
+  // If RangeBoundary is initialized with a container node and offset in it,
+  // its mRef may not have been initialized yet.  However, nsRange will need to
+  // adjust the offset when the node position is changed.  In such case,
+  // RangeBoundary::mRef needs to be initialized for recomputing offset later.
+  mStart.EnsureRef();
+  mEnd.EnsureRef();
+
   mIsPositioned = !!mStart.Container();
   if (checkCommonAncestor) {
     nsINode* oldCommonAncestor = mRegisteredCommonAncestor;
     nsINode* newCommonAncestor = GetCommonAncestor();
     if (newCommonAncestor != oldCommonAncestor) {
       if (oldCommonAncestor) {
         UnregisterCommonAncestor(oldCommonAncestor, false);
       }
--- a/editor/libeditor/EditorBase.cpp
+++ b/editor/libeditor/EditorBase.cpp
@@ -4038,67 +4038,64 @@ EditorBase::JoinNodeDeep(nsIContent& aLe
   // While the rightmost children and their descendants of the left node match
   // the leftmost children and their descendants of the right node, join them
   // up.
 
   nsCOMPtr<nsIContent> leftNodeToJoin = &aLeftNode;
   nsCOMPtr<nsIContent> rightNodeToJoin = &aRightNode;
   nsCOMPtr<nsINode> parentNode = aRightNode.GetParentNode();
 
-  nsCOMPtr<nsINode> resultNode = nullptr;
-  int32_t resultOffset = -1;
-
+  EditorDOMPoint ret;
   while (leftNodeToJoin && rightNodeToJoin && parentNode &&
          AreNodesSameType(leftNodeToJoin, rightNodeToJoin)) {
     uint32_t length = leftNodeToJoin->Length();
 
-    resultNode = rightNodeToJoin;
-    resultOffset = length;
+    ret.Set(rightNodeToJoin, length);
 
     // Do the join
     nsresult rv = JoinNodes(*leftNodeToJoin, *rightNodeToJoin);
     if (NS_WARN_IF(NS_FAILED(rv))) {
       return EditorDOMPoint();
     }
 
     if (parentNode->GetAsText()) {
       // We've joined all the way down to text nodes, we're done!
-      return EditorDOMPoint(resultNode, resultOffset);
+      return ret;
     }
 
     // Get new left and right nodes, and begin anew
     parentNode = rightNodeToJoin;
     rightNodeToJoin = parentNode->GetChildAt(length);
     if (rightNodeToJoin) {
       leftNodeToJoin = rightNodeToJoin->GetPreviousSibling();
     } else {
       leftNodeToJoin = nullptr;
     }
 
     // Skip over non-editable nodes
     while (leftNodeToJoin && !IsEditable(leftNodeToJoin)) {
       leftNodeToJoin = leftNodeToJoin->GetPreviousSibling();
     }
     if (!leftNodeToJoin) {
-      return EditorDOMPoint(resultNode, resultOffset);
+      return ret;
     }
 
     while (rightNodeToJoin && !IsEditable(rightNodeToJoin)) {
       rightNodeToJoin = rightNodeToJoin->GetNextSibling();
     }
     if (!rightNodeToJoin) {
-      return EditorDOMPoint(resultNode, resultOffset);
+      return ret;
     }
   }
 
-  if (NS_WARN_IF(!resultNode)) {
+  if (NS_WARN_IF(!ret.IsSet())) {
     return EditorDOMPoint();
   }
 
-  return EditorDOMPoint(resultNode, resultOffset);
+  return ret;
 }
 
 void
 EditorBase::BeginUpdateViewBatch()
 {
   NS_PRECONDITION(mUpdateCount >= 0, "bad state");
 
   if (!mUpdateCount) {
--- a/editor/libeditor/HTMLEditRules.cpp
+++ b/editor/libeditor/HTMLEditRules.cpp
@@ -7359,27 +7359,26 @@ HTMLEditRules::JoinNodesSmart(nsIContent
       return EditorDOMPoint();
     }
     nsresult rv = mHTMLEditor->MoveNode(&aNodeRight, parent, parOffset);
     if (NS_WARN_IF(NS_FAILED(rv))) {
       return EditorDOMPoint();
     }
   }
 
-  nsCOMPtr<nsINode> resultNode = &aNodeRight;
-  int32_t resultOffset = aNodeLeft.Length();
+  EditorDOMPoint ret(&aNodeRight, aNodeLeft.Length());
 
   // Separate join rules for differing blocks
   if (HTMLEditUtils::IsList(&aNodeLeft) || aNodeLeft.GetAsText()) {
     // For lists, merge shallow (wouldn't want to combine list items)
     nsresult rv = mHTMLEditor->JoinNodes(aNodeLeft, aNodeRight);
     if (NS_WARN_IF(NS_FAILED(rv))) {
       return EditorDOMPoint();
     }
-    return EditorDOMPoint(resultNode, resultOffset);
+    return ret;
   }
 
   // Remember the last left child, and first right child
   if (NS_WARN_IF(!mHTMLEditor)) {
     return EditorDOMPoint();
   }
   nsCOMPtr<nsIContent> lastLeft = mHTMLEditor->GetLastEditableChild(aNodeLeft);
   if (NS_WARN_IF(!lastLeft)) {
@@ -7409,17 +7408,17 @@ HTMLEditRules::JoinNodesSmart(nsIContent
        (lastLeft->IsElement() && firstRight->IsElement() &&
         mHTMLEditor->mCSSEditUtils->ElementsSameStyle(lastLeft->AsElement(),
                                                   firstRight->AsElement())))) {
     if (NS_WARN_IF(!mHTMLEditor)) {
       return EditorDOMPoint();
     }
     return JoinNodesSmart(*lastLeft, *firstRight);
   }
-  return EditorDOMPoint(resultNode, resultOffset);
+  return ret;
 }
 
 Element*
 HTMLEditRules::GetTopEnclosingMailCite(nsINode& aNode)
 {
   nsCOMPtr<Element> ret;
 
   for (nsCOMPtr<nsINode> node = &aNode; node; node = node->GetParentNode()) {