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
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 ...
Sorry, wrong revision number.
Created attachment 88833 [details] fixes the bug
This is currently one of top crashers and blocks Chromium release.
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 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 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 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?
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 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.
(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.
(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);
(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!
Committed r83322: <http://trac.webkit.org/changeset/83322>