1406482 - Avoid using nsINode::GetChildAt() in EditorBase::InsertTextImpl()

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement)

Comment 1

[(no longer active)]

Attachment: Avoid using nsINode::GetChildAt() in EditorBase::InsertTextImpl()

Comment 2

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

Comment on attachment 8916076 [details] [diff] [review]
Avoid using nsINode::GetChildAt() in EditorBase::InsertTextImpl()

Although, it might be better to insert:

MOZ_ASSERT(node->GetChildAt(offset) == aChildAtOffset);

in EditorBase::InsertTextImpl(). (below these lines, https://searchfox.org/mozilla-central/rev/aa721cc82a56fc307e899263d325c81b73e38a28/editor/libeditor/EditorBase.cpp#2503-2504)

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9843839bf1df
Avoid using nsINode::GetChildAt() in EditorBase::InsertTextImpl(); r=masayuki

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cce0046e302
Address review comment

Comment 5

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e9fdabc86073 and https://hg.mozilla.org/integration/mozilla-inbound/rev/56fffcd3581f for crashes like https://treeherder.mozilla.org/logviewer.html#?job_id=135804329&repo=mozilla-inbound

Comment 6

[(no longer active)]

The reason this assertion fires is that FindBetterInsertionPoint() here <https://searchfox.org/mozilla-central/rev/b53e29293c9e9a2905f4849f4e3c415e2013f0cb/editor/libeditor/TextEditRules.cpp#747> modifies selNode and selOffset, but selChild remains stale, so the assertion check fails.

Comment 7

[(no longer active)]

Attachment: Avoid using nsINode::GetChildAt() in EditorBase::InsertTextImpl()

Comment 8

[(no longer active)]

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

Comment 9

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

Comment on attachment 8917168 [details] [diff] [review]
Avoid using nsINode::GetChildAt() in EditorBase::InsertTextImpl()

>@@ -2386,23 +2386,24 @@ EditorBase::ScrollSelectionIntoView(bool aScrollToAnchor)
>   return NS_OK;
> }
> 
> void
> EditorBase::FindBetterInsertionPoint(nsCOMPtr<nsIDOMNode>& aNode,
>                                      int32_t& aOffset)
> {
>   nsCOMPtr<nsINode> node = do_QueryInterface(aNode);
>-  FindBetterInsertionPoint(node, aOffset);
>+  FindBetterInsertionPoint(node, aOffset, nullptr);
>   aNode = do_QueryInterface(node);
> }
> 
> void
> EditorBase::FindBetterInsertionPoint(nsCOMPtr<nsINode>& aNode,
>-                                     int32_t& aOffset)
>+                                     int32_t& aOffset,
>+                                     nsCOMPtr<nsIContent>* aSelChild)
> {

I really don't like this new argument because it's unclear what will be returned and when it returns non-nullptr. However, I have no better idea. Using the return (currently void) doesn't make sense since this method uses aNode and aOffset as the result too.

Anyway, could you explain what each argument means in the header file?

>@@ -2497,31 +2514,35 @@ EditorBase::InsertTextImpl(const nsAString& aStringToInsert,
> 
>   // This method doesn't support over INT32_MAX length text since aInOutOffset
>   // is int32_t*.
>   CheckedInt<int32_t> lengthToInsert(aStringToInsert.Length());
>   NS_ENSURE_TRUE(lengthToInsert.isValid(), NS_ERROR_INVALID_ARG);
> 
>   nsCOMPtr<nsINode> node = *aInOutNode;
>   int32_t offset = *aInOutOffset;
>+  nsCOMPtr<nsIContent> child = *aInOutChildAtOffset;
>+
>+  MOZ_ASSERT(node->GetChildAt(offset) == *aInOutChildAtOffset);
> 
>   // In some cases, the node may be the anonymous div elemnt or a mozBR
>   // element.  Let's try to look for better insertion point in the nearest
>   // text node if there is.
>-  FindBetterInsertionPoint(node, offset);
>+  FindBetterInsertionPoint(node, offset, address_of(child));
> 
>   // If a neighboring text node already exists, use that
>   if (!node->IsNodeOfType(nsINode::eTEXT)) {
>-    nsIContent* child = node->GetChildAt(offset);
>-    if (offset && child && child->GetPreviousSibling() &&
>+    if (offset && child &&
>+        child->GetPreviousSibling() &&

Well, I don't understand this change.

>         child->GetPreviousSibling()->IsNodeOfType(nsINode::eTEXT)) {
>       node = child->GetPreviousSibling();
>       offset = node->Length();
>     } else if (offset < static_cast<int32_t>(node->Length()) &&
>-               child && child->IsNodeOfType(nsINode::eTEXT)) {
>+               child &&
>+               child->IsNodeOfType(nsINode::eTEXT)) {

And here.

>@@ -1291,16 +1291,17 @@ HTMLEditRules::WillInsertText(EditAction aAction,
>   // for every property that is set, insert a new inline style node
>   nsresult rv = CreateStyleForInsertText(*aSelection, *doc);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   // get the (collapsed) selection location
>   NS_ENSURE_STATE(mHTMLEditor);
>   NS_ENSURE_STATE(aSelection->GetRangeAt(0));
>   nsCOMPtr<nsINode> selNode = aSelection->GetRangeAt(0)->GetStartContainer();
>+  nsCOMPtr<nsIContent> selChild = aSelection->GetRangeAt(0)->GetChildAtStartOffset();

Too lone line. Please break after |=|.

>@@ -1393,17 +1395,17 @@ HTMLEditRules::WillInsertText(EditAction aAction,
>             nsCOMPtr<Element> br =
>               mHTMLEditor->CreateBRImpl(address_of(curNode), &curOffset,
>                                         nsIEditor::eNone);
>             NS_ENSURE_STATE(br);
>             pos++;
>           } else {
>             NS_ENSURE_STATE(mHTMLEditor);
>             rv = mHTMLEditor->InsertTextImpl(subStr, address_of(curNode),
>-                                             &curOffset, doc);
>+                                             address_of(selChild), &curOffset, doc);

Too long line. Please break after |&curOffset,|.

> nsresult
> HTMLEditor::InsertTextImpl(const nsAString& aStringToInsert,
>                            nsCOMPtr<nsINode>* aInOutNode,
>+                           nsCOMPtr<nsIContent>* aInOutChildAtOffset,
>                            int32_t* aInOutOffset,
>                            nsIDocument* aDoc)
> {
>   // Do nothing if the node is read-only
>   if (!IsModifiableNode(*aInOutNode)) {
>     return NS_ERROR_FAILURE;
>   }
> 
>-  return EditorBase::InsertTextImpl(aStringToInsert, aInOutNode, aInOutOffset,
>-                                    aDoc);
>+  return EditorBase::InsertTextImpl(aStringToInsert, aInOutNode, aInOutChildAtOffset,

Too long line. Please break after |aInOutNode,|.

>@@ -721,16 +721,17 @@ TextEditRules::WillInsertText(EditAction aAction,
>     } else {
>       FillBufWithPWChars(outString, outString->Length());
>     }
>   }
> 
>   // get the (collapsed) selection location
>   NS_ENSURE_STATE(aSelection->GetRangeAt(0));
>   nsCOMPtr<nsINode> selNode = aSelection->GetRangeAt(0)->GetStartContainer();
>+  nsCOMPtr<nsIContent> selChild = aSelection->GetRangeAt(0)->GetChildAtStartOffset();

Too long line. Please break after |=|.

Comment 10

[(no longer active)]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #9)
> >   // If a neighboring text node already exists, use that
> >   if (!node->IsNodeOfType(nsINode::eTEXT)) {
> >-    nsIContent* child = node->GetChildAt(offset);
> >-    if (offset && child && child->GetPreviousSibling() &&
> >+    if (offset && child &&
> >+        child->GetPreviousSibling() &&
> 
> Well, I don't understand this change.

This is just removing the child variable from the inner scope since it's now defined in the outer scope and some whitespace change (caused by some renaming of variables which got erased during local rebasing).

> >         child->GetPreviousSibling()->IsNodeOfType(nsINode::eTEXT)) {
> >       node = child->GetPreviousSibling();
> >       offset = node->Length();
> >     } else if (offset < static_cast<int32_t>(node->Length()) &&
> >-               child && child->IsNodeOfType(nsINode::eTEXT)) {
> >+               child &&
> >+               child->IsNodeOfType(nsINode::eTEXT)) {
> 
> And here.

Just whitespace change.

Comment 11

[(no longer active)]

Attachment: Avoid using nsINode::GetChildAt() in EditorBase::InsertTextImpl()

Comment 12

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

Comment on attachment 8917417 [details] [diff] [review]
Avoid using nsINode::GetChildAt() in EditorBase::InsertTextImpl()

Thank you!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdb1abbe808a
Avoid using nsINode::GetChildAt() in EditorBase::InsertTextImpl(); r=masayuki

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fdb1abbe808a

Comment 15

[Jorg K (CEST = GMT+2)]

Can we please back this out for now, we have two regressions, see right above, and the assignee won't be available for a while.

Comment 16

[Jorg K (CEST = GMT+2)]

Looks like Masayuki is taking care of the regressions, already with success for bug 1407966.