Bug 67668 - Crashes in WebCore::EditCommand::apply(), DeleteSelectionCommand::doApply()
Summary: Crashes in WebCore::EditCommand::apply(), DeleteSelectionCommand::doApply()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Major
Assignee: Nobody
URL:
Keywords:
Depends on: 67762 67763 67765 67766 67767
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-06 13:29 PDT by Abhishek Arya
Modified: 2011-09-12 01:43 PDT (History)
4 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Abhishek Arya 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
Comment 1 Abhishek Arya 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
Comment 2 Shinya Kawanaka 2011-09-07 23:19:38 PDT
Created attachment 106698 [details]
Patch for testcase1
Comment 3 Kent Tamura 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.
Comment 4 Abhishek Arya 2011-09-07 23:29:39 PDT
Please keep the fixing bandwagon on remaining testcases and also merging to m14 835 branch.
Comment 5 Ryosuke Niwa 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.
Comment 6 Shinya Kawanaka 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...
Comment 7 Abhishek Arya 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. :)
Comment 8 Ryosuke Niwa 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.
Comment 9 Shinya Kawanaka 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.
Comment 10 Cris Neckar 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.
Comment 11 Ryosuke Niwa 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.
Comment 12 Abhishek Arya 2011-09-08 21:11:38 PDT
just a fyi, we need to merge these to both m14 and m15 branches.
Comment 13 Abhishek Arya 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*)
Comment 14 Cris Neckar 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.
Comment 15 Ryosuke Niwa 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.
Comment 16 Cris Neckar 2011-09-09 13:43:08 PDT
Ah, no worries.
Comment 17 Shinya Kawanaka 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)
Comment 18 Ryosuke Niwa 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.
Comment 19 Shinya Kawanaka 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.