Bug 67668

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:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

67762, 67763, 67765, 67766, 67767

Bug Blocks:

Attachments:

Description	Flags
Patch for testcase1	rniwa: review+, rniwa: commit-queue-

Abhishek Arya

Reported 2011-09-06 13:29:05 PDT

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

Attachments
Patch for testcase1 (3.51 KB, patch) 2011-09-07 23:19 PDT, Shinya Kawanaka	rniwa: review+ rniwa: commit-queue-	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Abhishek Arya

Comment 1 2011-09-06 13:31:11 PDT

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">&bcong;<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

Shinya Kawanaka

Comment 2 2011-09-07 23:19:38 PDT

Created attachment 106698 [details] Patch for testcase1

Kent Tamura

Comment 3 2011-09-07 23:28:30 PDT

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.

Abhishek Arya

Comment 4 2011-09-07 23:29:39 PDT

Please keep the fixing bandwagon on remaining testcases and also merging to m14 835 branch.

Ryosuke Niwa

Comment 5 2011-09-07 23:31:08 PDT

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.

Shinya Kawanaka

Comment 6 2011-09-07 23:37:48 PDT

(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...

Abhishek Arya

Comment 7 2011-09-07 23:39:43 PDT

(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. :)

Ryosuke Niwa

Comment 8 2011-09-07 23:41:02 PDT

(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.

Shinya Kawanaka

Comment 9 2011-09-07 23:43:48 PDT

(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.

Cris Neckar

Comment 10 2011-09-08 14:10:13 PDT

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.

Ryosuke Niwa

Comment 11 2011-09-08 14:15:56 PDT

(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.

Abhishek Arya

Comment 12 2011-09-08 21:11:38 PDT

just a fyi, we need to merge these to both m14 and m15 branches.

Abhishek Arya

Comment 13 2011-09-09 11:21:06 PDT

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*)

Cris Neckar

Comment 14 2011-09-09 13:20:10 PDT

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.

Ryosuke Niwa

Comment 15 2011-09-09 13:29:32 PDT

(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.

Cris Neckar

Comment 16 2011-09-09 13:43:08 PDT

Ah, no worries.

Shinya Kawanaka

Comment 17 2011-09-11 21:00:08 PDT

(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)

Ryosuke Niwa

Comment 18 2011-09-11 22:07:40 PDT

Let's not make this bug a master bug for all editing crashes. Please file new bugs for new crashes.

Shinya Kawanaka

Comment 19 2011-09-12 01:43:14 PDT

(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.

Note You need to log in before you can comment on or make changes to this bug.