Bug 57743 - REGRESSION(r81887): Crash in SplitElement
Summary: REGRESSION(r81887): Crash in SplitElement
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P1 Critical
Assignee: Ryosuke Niwa
URL:
Keywords: HasReduction
Depends on: 58074 58078 58081
Blocks: 58158
  Show dependency treegraph
 
Reported: 2011-04-04 00:13 PDT by Ryosuke Niwa
Modified: 2011-04-08 13:19 PDT (History)
8 users (show)

See Also:


Attachments
Repro (865 bytes, text/html)
2011-04-04 04:54 PDT, Berend-Jan Wever
no flags Details
fixes the bug (8.28 KB, patch)
2011-04-08 09:47 PDT, Ryosuke Niwa
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2011-04-04 00:13:39 PDT
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
Comment 1 Berend-Jan Wever 2011-04-04 04:54:44 PDT
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
                ...
Comment 2 Ryosuke Niwa 2011-04-07 08:11:34 PDT
Sorry, wrong revision number.
Comment 3 Ryosuke Niwa 2011-04-08 09:47:22 PDT
Created attachment 88833 [details]
fixes the bug
Comment 4 Ryosuke Niwa 2011-04-08 09:49:13 PDT
This is currently one of top crashers and blocks Chromium release.
Comment 5 Tony Chang 2011-04-08 09:59:45 PDT
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.
Comment 6 Darin Adler 2011-04-08 10:00:32 PDT
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.
Comment 7 Darin Adler 2011-04-08 10:01:06 PDT
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.
Comment 8 Enrica Casucci 2011-04-08 10:07:01 PDT
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?
Comment 9 Ryosuke Niwa 2011-04-08 10:07:42 PDT
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!
Comment 10 Ryosuke Niwa 2011-04-08 10:09:57 PDT
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.
Comment 11 Enrica Casucci 2011-04-08 10:11:10 PDT
(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.
Comment 12 Ryosuke Niwa 2011-04-08 10:18:06 PDT
(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);
Comment 13 Enrica Casucci 2011-04-08 10:35:12 PDT
(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!
Comment 14 Ryosuke Niwa 2011-04-08 12:29:27 PDT
Committed r83322: <http://trac.webkit.org/changeset/83322>