Summary: | REGRESSION(r81887): Crash in SplitElement | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||
Component: | HTML Editing | Assignee: | Ryosuke Niwa <rniwa> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Critical | CC: | adele, ap, darin, enrica, ojan, tkent, tony, yutak | ||||||
Priority: | P1 | Keywords: | HasReduction | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Other | ||||||||
OS: | Other | ||||||||
Bug Depends on: | 58074, 58078, 58081 | ||||||||
Bug Blocks: | 58158 | ||||||||
Attachments: |
|
Description
Ryosuke Niwa
2011-04-04 00:13:39 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
...
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> |