WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
67668
Crashes in WebCore::EditCommand::apply(), DeleteSelectionCommand::doApply()
https://bugs.webkit.org/show_bug.cgi?id=67668
Summary
Crashes in WebCore::EditCommand::apply(), DeleteSelectionCommand::doApply()
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">≌<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.
Top of Page
Format For Printing
XML
Clone This Bug