Closed Bug 1406482 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: DOM: Editor, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

Details

Attachments

(1 file, 2 obsolete files)

      No description provided.
Attachment #8916076 - Flags: review?(masayuki)
Assignee: nobody → ehsan
Blocks: 651120
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)
Attachment #8916076 - Flags: review?(masayuki) → review+
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9843839bf1df
Avoid using nsINode::GetChildAt() in EditorBase::InsertTextImpl(); r=masayuki
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.
Flags: needinfo?(ehsan)
Attachment #8917168 - Flags: review?(masayuki)
Attachment #8916076 - Attachment is obsolete: true
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 |=|.
Attachment #8917168 - Flags: review?(masayuki) → review-
(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.
Attachment #8917168 - Attachment is obsolete: true
Comment on attachment 8917417 [details] [diff] [review]
Avoid using nsINode::GetChildAt() in EditorBase::InsertTextImpl()

Thank you!
Attachment #8917417 - Flags: review?(masayuki) → review+
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdb1abbe808a
Avoid using nsINode::GetChildAt() in EditorBase::InsertTextImpl(); r=masayuki
https://hg.mozilla.org/mozilla-central/rev/fdb1abbe808a
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Depends on: 1407966
Depends on: 1408227
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.
Flags: needinfo?(masayuki)
Flags: needinfo?(m_kato)
Depends on: 1409520
Looks like Masayuki is taking care of the regressions, already with success for bug 1407966.
Flags: needinfo?(masayuki)
Flags: needinfo?(m_kato)
Depends on: 1415231
You need to log in before you can comment on or make changes to this bug.