Bug 57743 – REGRESSION(r81887): Crash in SplitElement

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

We're getting a lot of null pointer crash reports with the following stack trace and it's a release blocker for us:
0x6caeb8da	 [chrome.dll	 - splitelementcommand.cpp:47]	WebCore::SplitElementCommand::executeApply()
0x6caeba30	 [chrome.dll	 - splitelementcommand.cpp:76]	WebCore::SplitElementCommand::doApply()
0x6ca94f52	 [chrome.dll	 - editcommand.cpp:92]	WebCore::EditCommand::apply()
0x6ca81533	 [chrome.dll	 - compositeeditcommand.cpp:102]	WebCore::CompositeEditCommand::applyCommandToComposite(WTF::PassRefPtr<WebCore::EditCommand>)
0x6ca81d53	 [chrome.dll	 - compositeeditcommand.cpp:270]	WebCore::CompositeEditCommand::splitElement(WTF::PassRefPtr<WebCore::Element>,WTF::PassRefPtr<WebCore::Node>)
0x6ca994a5	 [chrome.dll	 - replaceselectioncommand.cpp:965]	WebCore::ReplaceSelectionCommand::doApply()
0x6ca94f52	 [chrome.dll	 - editcommand.cpp:92]	WebCore::EditCommand::apply()
0x6ca950db	 [chrome.dll	 - editcommand.cpp:224]	WebCore::applyCommand(WTF::PassRefPtr<WebCore::EditCommand>)
0x6c9f4964	 [chrome.dll	 - editor.cpp:438]	WebCore::Editor::replaceSelectionWithFragment(WTF::PassRefPtr<WebCore::DocumentFragment>,bool,bool,bool)
0x6c9f40c8	 [chrome.dll	 - editor.cpp:195]	WebCore::Editor::handleTextEvent(WebCore::TextEvent *)
0x6c9cf16d	 [chrome.dll	 - node.cpp:3074]	WebCore::Node::defaultEventHandler(WebCore::Event *)
0x6c9ce60c	 [chrome.dll	 - node.cpp:2768]	WebCore::Node::dispatchEvent(WTF::PassRefPtr<WebCore::Event>)
0x6ca642a3	 [chrome.dll	 - eventtarget.cpp:297]	WebCore::EventTarget::dispatchEvent(WTF::PassRefPtr<WebCore::Event>,int &)
0x6c9f4605	 [chrome.dll	 - editor.cpp:381]	WebCore::Editor::pasteAsFragment(WTF::PassRefPtr<WebCore::DocumentFragment>,bool,bool)
0x6c9f479a	 [chrome.dll	 - editor.cpp:403]	WebCore::Editor::pasteWithPasteboard(WebCore::Pasteboard *,bool)
0x6c9f698a	 [chrome.dll	 - editor.cpp:1300]	WebCore::Editor::paste()

As far as I can tell, node after positon is null in:
splitElement(static_cast<Element*>(parentNode), insertionPos.computeNodeAfterPosition());

See: http://trac.webkit.org/changeset/81887/trunk/Source/WebCore/editing/ReplaceSelectionCommand.cpp

http://crbug.com/77685

[Berend-Jan Wever]

Created attachment 88051 [details]
Repro

I think I hit this during fuzzing, repro:
<pre id="x"><x style="white-space: pre-wrap;"><br></x></pre>
<script>
  var x = document.getElementById("x");
  document.execCommand("selectall",false);
  document.designMode="on";
  console.log(x.innerHTML);             // <x style="white-space: pre-wrap;"><br></x>
  document.execCommand("InsertImage");  // replace <br> with <img> and set some state: simply changing the original
                                        // html does not reproduce the issue.
  console.log(x.innerHTML);             // <x style="white-space: pre-wrap;"><img></x>
  document.execCommand("InsertImage");
  console.log(x.innerHTML);             // <span class="Apple-style-span" style="font-family: 'Times New Roman'; white-space: normal; "><img></span><x style="white-space: pre-wrap;"><img></x>
  document.execCommand("InsertImage");  // crash
  console.log(x.innerHTML);
</script>

id:             chrome.dll!WebCore::SplitElementCommand::executeApply ReadAV@NULL (3a063a704d2ab72ff3cb23ae09a074fa)
description:    Attempt to read from unallocated NULL pointer+0x24 in chrome.dll!WebCore::SplitElementCommand::executeApply
stack:          chrome.dll!WebCore::SplitElementCommand::executeApply
                chrome.dll!WebCore::CompositeEditCommand::applyCommandToComposite
                chrome.dll!WebCore::CompositeEditCommand::splitElement
                chrome.dll!WebCore::ReplaceSelectionCommand::doApply
                chrome.dll!WebCore::EditCommand::apply
                chrome.dll!WebCore::CompositeEditCommand::applyCommandToComposite
                chrome.dll!WebCore::CompositeEditCommand::moveParagraphs
                chrome.dll!WebCore::ReplaceSelectionCommand::doApply
                chrome.dll!WebCore::EditCommand::apply
                chrome.dll!WebCore::applyCommand
                chrome.dll!WebCore::executeInsertFragment
                chrome.dll!WebCore::executeInsertNode
                chrome.dll!WebCore::executeInsertImage
                chrome.dll!WebCore::Editor::Command::execute
                chrome.dll!WebCore::Document::execCommand
                chrome.dll!WebCore::DocumentInternal::execCommandCallback
                chrome.dll!v8::internal::Invoke
                chrome.dll!v8::internal::Execution::Call
                ...

[Ryosuke Niwa]

Sorry, wrong revision number.

[Ryosuke Niwa]

Created attachment 88833 [details]
fixes the bug

[Ryosuke Niwa]

This is currently one of top crashers and blocks Chromium release.

[Tony Chang]

Comment on attachment 88833 [details]
fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=88833&action=review

> Source/WebCore/editing/CompositeEditCommand.cpp:1220
>  PassRefPtr<Node> CompositeEditCommand::splitTreeToNode(Node* start, Node* end, bool splitAncestor)

Nit: Can we rename splitAncestor to shouldSplitAncestor?

> LayoutTests/editing/inserting/insert-images-in-pre-x-crash.html:1
> +<pre id="x"><x style="white-space: pre-wrap;"><br></x></pre>

Did you mean to include <html> and <body> here?  Maybe a doctype too.

[Darin Adler]

Comment on attachment 88833 [details]
fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=88833&action=review

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:957
> +        if (insertionPos.containerNode()->isTextNode() && insertionPos.offsetInContainerNode() && !insertionPos.atLastEditingPositionForNode()) {
> +            splitTextNodeContainingElement(static_cast<Text*>(insertionPos.anchorNode()), insertionPos.offsetInContainerNode());

It would be clearer if we called insertionPos.containerNode() both times. To someone reading the code the cast seems dangerous since the type of insertionPos.containerNode() is checked and based on that the code does a typecast of static_cast<Text*>(insertionPos.anchorNode()). Someone reading should be able to see the type safety without knowing that one condition guarantees the other.

[Darin Adler]

Comment on attachment 88833 [details]
fixes the bug

Oops. Review tool clobbered Tony’s review+. I’ll say review+ myself -- bugs.webkit.org doesn’t give me a way to restore his review.

[Enrica Casucci]

Comment on attachment 88833 [details]
fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=88833&action=review

> Source/WebCore/editing/CompositeEditCommand.cpp:-1225
> -    for (node = start; node && node->parentNode() != end; node = node->parentNode()) {

I think you should add a comment here explaining the logic, something similar to what you have in the ChangeLog.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:966
>          }

Does this move the insertion point also when there is nothing to split? For example, if you have <b><i>hello</i></b> and the original insertion point is at offset 0 in the text node, does it move the insertion point before the b tag?

[Ryosuke Niwa]

Thanks for the review, Tony & Darin!

(In reply to comment #5)
> (From update of attachment 88833 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88833&action=review
> 
> > Source/WebCore/editing/CompositeEditCommand.cpp:1220
> >  PassRefPtr<Node> CompositeEditCommand::splitTreeToNode(Node* start, Node* end, bool splitAncestor)
> 
> Nit: Can we rename splitAncestor to shouldSplitAncestor?

Will do!

> > LayoutTests/editing/inserting/insert-images-in-pre-x-crash.html:1
> > +<pre id="x"><x style="white-space: pre-wrap;"><br></x></pre>
> 
> Did you mean to include <html> and <body> here?  Maybe a doctype too.

It tried but the crash doesn't reproduce if we have DOCTYPE, html, or body.  It's a really fragile test.  I'll add a comment in the test so that people don't accidentally add them in the future.

(In reply to comment #6)
> (From update of attachment 88833 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88833&action=review
> 
> > Source/WebCore/editing/ReplaceSelectionCommand.cpp:957
> > +        if (insertionPos.containerNode()->isTextNode() && insertionPos.offsetInContainerNode() && !insertionPos.atLastEditingPositionForNode()) {
> > +            splitTextNodeContainingElement(static_cast<Text*>(insertionPos.anchorNode()), insertionPos.offsetInContainerNode());
> 
> It would be clearer if we called insertionPos.containerNode() both times. To someone reading the code the cast seems dangerous since the type of insertionPos.containerNode() is checked and based on that the code does a typecast of static_cast<Text*>(insertionPos.anchorNode()). Someone reading should be able to see the type safety without knowing that one condition guarantees the other.

Oops, yeah I totally agree. I must have copied & pasted from the old code and forgot to change it.  Will fix!

[Ryosuke Niwa]

Comment on attachment 88833 [details]
fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=88833&action=review

Thanks for the feedback, Enrica!

>> Source/WebCore/editing/CompositeEditCommand.cpp:-1225

> 
> I think you should add a comment here explaining the logic, something similar to what you have in the ChangeLog.

Which part of change log are you referring to?

>> Source/WebCore/editing/ReplaceSelectionCommand.cpp:966
>>          }
> 
> Does this move the insertion point also when there is nothing to split? For example, if you have <b><i>hello</i></b> and the original insertion point is at offset 0 in the text node, does it move the insertion point before the b tag?

Yes.

[Enrica Casucci]

(In reply to comment #10)
> (From update of attachment 88833 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88833&action=review
> 
> Thanks for the feedback, Enrica!
> 
> >> Source/WebCore/editing/CompositeEditCommand.cpp:-1225
> 
> > 
> > I think you should add a comment here explaining the logic, something similar to what you have in the ChangeLog.
> 
> Which part of change log are you referring to?
where you explain the changes to CompositeEditCommand.cpp.
> 
> >> Source/WebCore/editing/ReplaceSelectionCommand.cpp:966
> >>          }
> > 
> > Does this move the insertion point also when there is nothing to split? For example, if you have <b><i>hello</i></b> and the original insertion point is at offset 0 in the text node, does it move the insertion point before the b tag?
> 
> Yes.

[Ryosuke Niwa]

(In reply to comment #11)
> (In reply to comment #10)
> > Which part of change log are you referring to?
> where you explain the changes to CompositeEditCommand.cpp.

Oh you mean not to duplicate nodes?  How about this?

        if (!node->parentNode()->isElementNode())
            break;
        // Do not split a node when doing so introduces an empty node
        VisiblePosition positionInParent = firstPositionInNode(node->parentNode());
        VisiblePosition positionInNode = firstPositionInOrBeforeNode(node.get());
        if (positionInParent != positionInNode)
            splitElement(static_cast<Element*>(node->parentNode()), node);

[Enrica Casucci]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > Which part of change log are you referring to?
> > where you explain the changes to CompositeEditCommand.cpp.
> 
> Oh you mean not to duplicate nodes?  How about this?
> 
>         if (!node->parentNode()->isElementNode())
>             break;
>         // Do not split a node when doing so introduces an empty node
>         VisiblePosition positionInParent = firstPositionInNode(node->parentNode());
>         VisiblePosition positionInNode = firstPositionInOrBeforeNode(node.get());
>         if (positionInParent != positionInNode)
>             splitElement(static_cast<Element*>(node->parentNode()), node);

Yes, thank you!

[Ryosuke Niwa]

Committed r83322: <http://trac.webkit.org/changeset/83322>