1395936 - Avoid child index usage in HTMLEditor::DoContentInserted

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Comment 1

[Cătălin Badea (:catalinb)]

Attachment: Avoid child index usage in HTMLEditor::DoContentInserted.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8903562 [details] [diff] [review]
Avoid child index usage in HTMLEditor::DoContentInserted.

>@@ -3270,30 +3273,23 @@ HTMLEditor::DoContentInserted(nsIDocument* aDocument,
>     }
>     // Protect the edit rules object from dying
>     nsCOMPtr<nsIEditRules> rules(mRules);
>     rules->DocumentModified();
> 
>     // Update spellcheck for only the newly-inserted node (bug 743819)
>     if (mInlineSpellChecker) {
>       RefPtr<nsRange> range = new nsRange(aChild);

Although, not the scope of this bug, we could cache the range for reducing the allocation cost.

>-      int32_t endIndex = aIndexInContainer + 1;
>+      nsIContent* endRef = aChild;

nit: I like endContent since |Ref| is usually used for C++ reference (variable name for Foo&).

>       if (aInsertedOrAppended == eAppended) {
>-        // Count all the appended nodes
>-        nsIContent* sibling = aChild->GetNextSibling();
>-        while (sibling) {
>-          endIndex++;
>-          sibling = sibling->GetNextSibling();
>-        }
>-      }
>-      nsresult rv = range->SetStartAndEnd(aContainer, aIndexInContainer,
>-                                          aContainer, endIndex);
>-      if (NS_SUCCEEDED(rv)) {
>-        mInlineSpellChecker->SpellCheckRange(range);
>+        // Maybe more than 1 child was appended.
>+        endRef = aContainer->GetLastChild();
>       }
>+      range->SelectNodesInContainer(aContainer, aChild->GetPreviousSibling(), endRef);

Hmm, from the view of the point of the caller, this really looks like that the range will be from aChild->GetPreviousSibling() to aChild or aContainer->GetLastChild() rather than from aChild.

So, how about to retrieve previous sibling *in* SelectNodesInContainer()?

>+void
>+nsRange::SelectNodesInContainer(nsINode* aContainer,
>+                                nsIContent* aStartRef,
>+                                nsIContent* aEndRef)

So, I like aStartContent and aEndContent better.

>+{
>+  MOZ_ASSERT(aContainer);
>+  MOZ_ASSERT(aContainer->IndexOf(aStartRef) <= aContainer->IndexOf(aEndRef));
>+  MOZ_ASSERT_IF(aStartRef, aContainer->IndexOf(aStartRef) != -1);
>+  MOZ_ASSERT_IF(aEndRef, aContainer->IndexOf(aEndRef) != -1);
>+
>+  RawRangeBoundary start(aContainer, aStartRef);

So, this should be:

RawRangeBoundary start(aContainer, aStartContent->GetPreviousSibling());

>diff --git a/dom/base/nsRange.h b/dom/base/nsRange.h
>index 64882b7bbdd1..87f54f8ef8eb 100644
>--- a/dom/base/nsRange.h
>+++ b/dom/base/nsRange.h
>@@ -164,16 +164,27 @@ public:
>    * the start point or the end point is invalid point.
>    * If the specified start point is after the end point, the range will be
>    * collapsed at the end point.  Similarly, if they are in different root,
>    * the range will be collapsed at the end point.
>    */
>   nsresult SetStartAndEnd(nsINode* aStartContainer, uint32_t aStartOffset,
>                           nsINode* aEndContainer, uint32_t aEndOffset);
> 
>+  /**
>+   * The start offset will be set immediately after |aStartRef|, while
>+   * the end offset will be set immediately after |aEndRef|. Passing null
>+   * will correspond to a 0 offset. Caller must guarantee both refs are
>+   * children of |aContainer| and that aEndRef is after aStartRef.
>+   */

Then, you need to update this comment. aStartContent shouldn't be nullptr.

>+  void
>+  SelectNodesInContainer(nsINode* aContainer,
>+                         nsIContent* aStartRef,
>+                         nsIContent* aEndRef);

So, I like aStartContent and aEndContent.

Comment 3

[Cătălin Badea (:catalinb)]

Attachment: Avoid child index usage in HTMLEditor::DoContentInserted.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oh, sorry, I missed to catch the new review request.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8903903 [details] [diff] [review]
Avoid child index usage in HTMLEditor::DoContentInserted.

># HG changeset patch
># User Catalin Badea <[e-mail]>
>
>Bug 1395936 - Avoid child index usage in HTMLEditor::DoContentInserted. r=masayuki
>
>diff --git a/dom/base/nsRange.cpp b/dom/base/nsRange.cpp
>index d495215a2a8e..04cb97192eb7 100644
>--- a/dom/base/nsRange.cpp
>+++ b/dom/base/nsRange.cpp
>@@ -1482,16 +1482,31 @@ nsRange::SetEnd(nsINode* aContainer, uint32_t aOffset)
>   }
> 
>   RawRangeBoundary newEnd(aContainer, aOffset);
>   DoSetRange(mStart.AsRaw(), newEnd, mRoot);
> 
>   return NS_OK;
> }
> 
>+void
>+nsRange::SelectNodesInContainer(nsINode* aContainer,
>+                                nsIContent* aStartContent,
>+                                nsIContent* aEndContent)
>+{
>+  MOZ_ASSERT(aContainer);
>+  MOZ_ASSERT(aContainer->IndexOf(aStartContent) <= aContainer->IndexOf(aEndContent));

nit: too long line. Perhaps:

  MOZ_ASSERT(aContainer->IndexOf(aStartContent) <=
               aContainer->IndexOf(aEndContent));

>diff --git a/dom/base/nsRange.h b/dom/base/nsRange.h
>@@ -167,16 +167,29 @@ public:
>    * the start point or the end point is invalid point.
>    * If the specified start point is after the end point, the range will be
>    * collapsed at the end point.  Similarly, if they are in different root,
>    * the range will be collapsed at the end point.
>    */
>   nsresult SetStartAndEnd(nsINode* aStartContainer, uint32_t aStartOffset,
>                           nsINode* aEndContainer, uint32_t aEndOffset);
> 
>+  /**
>+   * Adds all nodes between |aStartContent| and |aEndContent| to the range.
>+   * The start offset will be set before |aStartContent|,
>+   * while the end offset will be set immediately after |aEndContent|.
>+   *
>+   * Caller must guarantee both nodes are non null and
>+   * children of |aContainer| and that |aEndContent| is after |aStartContent|.
>+   */
>+  void
>+  SelectNodesInContainer(nsINode* aContainer,

nit: you don't need to break after the result type at declaration. I.e.,

  void SelectNodesInContainer(nsINode* aContainer,

>+                         nsIContent* aStartContent,
>+                         nsIContent* aEndContent);

Then, adjust these indent too.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbb4fb7aa5bb
Avoid child index usage in HTMLEditor::DoContentInserted. r=masayuki

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for assertions and crashes in a11y test, mochitests, reftests, crashtest, ...:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c8b7472de17bf8d1183883a309cda6fef41a93cd

A push which ran most of the failing tests: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=d80fa4d123620fd099db9b0e790e5d5daed24590&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Comment 8

[Cătălin Badea (:catalinb)]

Attachment: Avoid child index usage in HTMLEditor::DoContentInserted.

Fix failures.

Comment 9

[Cătălin Badea (:catalinb)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfcb55cf7d4214976570b0a228f238a02f5c3f87

Comment 10

[Cătălin Badea (:catalinb)]

Attachment: Avoid child index usage in HTMLEditor::DoContentInserted.

Set the correct root. I think this fixes the rest of the test failures.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8dc4c247094c
Avoid child index usage in HTMLEditor::DoContentInserted. r=masayuki

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/8dc4c247094c