| Summary: | Crashes in WebCore::EditCommand::apply(), DeleteSelectionCommand::doApply() | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Abhishek Arya <inferno> | ||||
| Component: | HTML Editing | Assignee: | Nobody <webkit-unassigned> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Major | CC: | cachobot, cdn, rniwa, shinyak | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| Bug Depends on: | 67762, 67763, 67765, 67766, 67767 | ||||||
| Bug Blocks: | |||||||
| Attachments: |
|
||||||
testcase4:: [WebCore::ApplyStyleCommand::doApply()]
<card id="edit" contentEditable="true">A<script>
edit.focus();
document.execCommand("SelectAll");
document.execCommand("RemoveFormat");
</script>
testcase5:: [WebCore::ApplyStyleCommand::applyBlockStyle(WebCore::EditingStyle*) ]
<mfrac id="div" contenteditable="true">≌<script>
div.focus();
document.execCommand("SelectAll");
document.execCommand("RemoveFormat");
</script>
testcase6:: [WebCore::AppendNodeCommand::create(WTF::PassRefPtr<WebCore::ContainerNode>, WTF::PassRefPtr<WebCore::Node>) ]
<script>
function runTest()
{
window.getSelection().setBaseAndExtent(start, 0, null, 0);
document.execCommand("Indent");
}
</script>
<meta content="2"/><body onLoad="runTest();">
><defs contenteditable="true" id="start">
<rt>AAAAAAA0A0AAAA00
Created attachment 106698 [details]
Patch for testcase1
Comment on attachment 106698 [details] Patch for testcase1 View in context: https://bugs.webkit.org/attachment.cgi?id=106698&action=review > LayoutTests/ChangeLog:11 > + * editing/inserting/insert-67668-crash-expected.txt: Added. > + * editing/inserting/insert-67668-crash.html: Added. I do not like such cryptic test name. Please keep the fixing bandwagon on remaining testcases and also merging to m14 835 branch. Comment on attachment 106698 [details] Patch for testcase1 (In reply to comment #3) > (From update of attachment 106698 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=106698&action=review > > > LayoutTests/ChangeLog:11 > > + * editing/inserting/insert-67668-crash-expected.txt: Added. > > + * editing/inserting/insert-67668-crash.html: Added. > > I do not like such cryptic test name. I agree. Please fix the filename before landing it. (In reply to comment #5) > (From update of attachment 106698 [details]) > (In reply to comment #3) > > (From update of attachment 106698 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=106698&action=review > > > > > LayoutTests/ChangeLog:11 > > > + * editing/inserting/insert-67668-crash-expected.txt: Added. > > > + * editing/inserting/insert-67668-crash.html: Added. > > > > I do not like such cryptic test name. > > I agree. Please fix the filename before landing it. Since the reason of these crashes are different, I would like to divide this bug into several bugs first. Then I upload patches again... (In reply to comment #6) > (In reply to comment #5) > > (From update of attachment 106698 [details] [details]) > > (In reply to comment #3) > > > (From update of attachment 106698 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=106698&action=review > > > > > > > LayoutTests/ChangeLog:11 > > > > + * editing/inserting/insert-67668-crash-expected.txt: Added. > > > > + * editing/inserting/insert-67668-crash.html: Added. > > > > > > I do not like such cryptic test name. > > > > I agree. Please fix the filename before landing it. > > Since the reason of these crashes are different, I would like to divide this bug into several bugs first. > Then I upload patches again... I just wanted to make your merging task easier. :) (In reply to comment #6) > Since the reason of these crashes are different, I would like to divide this bug into several bugs first. > Then I upload patches again... Sounds good to me. (In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #5) > > > (From update of attachment 106698 [details] [details] [details]) > > > (In reply to comment #3) > > > > (From update of attachment 106698 [details] [details] [details] [details]) > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=106698&action=review > > > > > > > > > LayoutTests/ChangeLog:11 > > > > > + * editing/inserting/insert-67668-crash-expected.txt: Added. > > > > > + * editing/inserting/insert-67668-crash.html: Added. > > > > > > > > I do not like such cryptic test name. > > > > > > I agree. Please fix the filename before landing it. > > > > Since the reason of these crashes are different, I would like to divide this bug into several bugs first. > > Then I upload patches again... > > I just wanted to make your merging task easier. :) I think we can easily track bugs by seeing 'Depends on' field. I am not convinced that the core issue here is actually separate bugs. It seems like there is a common theme with all of these. More specifically each of the test cases involves creating a new selection and then calling set position on it (some call setposition as a result of a different call.. testcase 6 etc) I suspect we will need to figure out the underlying issue for some of these as a simple null check still hits tons of asserts for a lot of them. This isn't an argument against the patch just noting that I think there is a better common fix. (In reply to comment #10) > I am not convinced that the core issue here is actually separate bugs. It seems like there is a common theme with all of these. More specifically each of the test cases involves creating a new selection and then calling set position on it (some call setposition as a result of a different call.. testcase 6 etc) I'm confused. var sel = window.getSelection(); doesn't create a new selection. It obtains the selection of the current frame. And sel.setPosition(div, 0) modifies the selection of the current frame. As far as I can tell, most of these test cases are completely unrelated. just a fyi, we need to merge these to both m14 and m15 branches. Guys, can you handle some more. It is awesome to knock these null ptrs
1)
<dl><div id="div" contenteditable="true"A><script>
div.focus();
document.execCommand("InsertUnorderedList");
</script>
Crash
WebCore::InsertListCommand::unlistifyParagraph(WebCore::VisiblePosition const&, WebCore::HTMLElement*, WebCore::Node*)
WebCore::InsertListCommand::doApplyForSingleParagraph(bool, WebCore::QualifiedName const&, WebCore::Range*)
WebCore::InsertListCommand::doApply()
WebCore::EditCommand::apply()
WebCore::executeInsertUnorderedList(WebCore::Frame*, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) third_party/WebKit/Source/WebCore/editing/EditorCommand.cpp:0
WebCore::Editor::Command::execute(WTF::String const&, WebCore::Event*) const
2)
><a id="anchor" href="http://www.google.com/"><feDisplacementMap>A0A0AAAA0AA<base id="paste" contenteditable="true"><br><script>
var sel = window.getSelection();
var range = document.createRange();
range.selectNodeContents(anchor);
sel.addRange(range);
document.execCommand("Copy");
paste.focus();
document.execCommand("Paste");
</script>
WebCore::nextCandidate(WebCore::Position const&)
WebCore::ReplaceSelectionCommand::positionAtStartOfInsertedContent()
WebCore::ReplaceSelectionCommand::doApply()
WebCore::EditCommand::apply()
WebCore::Editor::replaceSelectionWithFragment(WTF::PassRefPtr<WebCore::DocumentFragment>, bool, bool, bool)
WebCore::Editor::handleTextEvent(WebCore::TextEvent*)
Ryosuke:
DOMSelection* DOMWindow::getSelection()
{
if (!m_selection)
m_selection = DOMSelection::create(m_frame);
...
When there is no m_selection on the DOMWindow a new DOMSelection is created. This is the case with all of these that I have stepped through.
(In reply to comment #14) > DOMSelection* DOMWindow::getSelection() > { > if (!m_selection) > m_selection = DOMSelection::create(m_frame); > ... > > When there is no m_selection on the DOMWindow a new DOMSelection is created. This is the case with all of these that I have stepped through. Yes, but DOMSelection is just a wrapper for FrameSelection, which is a different object from DOMSelection and is always available on frame. Ah, no worries. (In reply to comment #13) > Guys, can you handle some more. It is awesome to knock these null ptrs I could repro (1), but I couldn't repro (2). So I've filed (1) bug only. (Bug 67918) Let's not make this bug a master bug for all editing crashes. Please file new bugs for new crashes. (In reply to comment #18) > Let's not make this bug a master bug for all editing crashes. Please file new bugs for new crashes. OK. The all bugs have been closed now, so I think we should close this bug. |
testcase1:: <feSpotLight><sub id="div" contenteditable="true"><script> var sel = window.getSelection(); sel.setPosition(div, 0); document.execCommand("InsertHTML", false, "<dl>"); </script> testcase2:: ><meter contenteditable><span id="wrapper">><script> var sel = window.getSelection(); sel.setPosition(document.getElementById("wrapper"), 1); document.execCommand("InsertParagraph", false, null); </script> testcase3:: <div contenteditable="true" id="div"><hkern><span contenteditable="false"><dl>000A0<script> var sel = window.getSelection(); sel.setPosition(div, 2000000000); document.execCommand("Delete"); </script> These might be contributing to the top crashers in chromium