Bug 67918 - Crashes in WebCore::InsertListCommand::unlistifyParagraph
Summary: Crashes in WebCore::InsertListCommand::unlistifyParagraph
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 67966 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-09-11 20:58 PDT by Shinya Kawanaka
Modified: 2011-09-12 22:56 PDT (History)
4 users (show)

See Also:


Attachments
Patch (3.80 KB, patch)
2011-09-11 22:26 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (3.78 KB, patch)
2011-09-12 01:35 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (3.84 KB, patch)
2011-09-12 19:35 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (4.45 KB, patch)
2011-09-12 20:36 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shinya Kawanaka 2011-09-11 20:58:22 PDT
repro
<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
Comment 1 Shinya Kawanaka 2011-09-11 22:26:11 PDT
Created attachment 107021 [details]
Patch
Comment 2 Ryosuke Niwa 2011-09-11 22:50:52 PDT
Comment on attachment 107021 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107021&action=review

> Source/WebCore/editing/htmlediting.cpp:669
> -        if (n->hasTagName(liTag) || isListElement(n->parentNode()))
> +        if (n->hasTagName(liTag) || (isListElement(n->parentNode()) && n->parentNode()->rendererIsEditable()))

This change seems wrong. You should be checking that n != root instead.
Comment 3 Ryosuke Niwa 2011-09-11 22:51:36 PDT
Comment on attachment 107021 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107021&action=review

> LayoutTests/editing/execCommand/insert-list-in-noneditable-list-parent.html:9
> +document.writeln('execCommand("InsertUnorderedList") for contenteditable root element was crashing.<br><br>');
> +document.writeln('The test has passed if it does not crash.<br><br>');

Why do we need so many brs?
Comment 4 Ryosuke Niwa 2011-09-11 22:52:32 PDT
Comment on attachment 107021 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107021&action=review

>> LayoutTests/editing/execCommand/insert-list-in-noneditable-list-parent.html:9
>> +document.writeln('The test has passed if it does not crash.<br><br>');
> 
> Why do we need so many brs?

Do we do correct thing?  i.e. we should get <ul><li><br></li></ul> inside the div after executing InsertUnorderedList.  You can use dump-as-markup to verify this.
Comment 5 Shinya Kawanaka 2011-09-12 01:35:00 PDT
Created attachment 107030 [details]
Patch
Comment 6 Shinya Kawanaka 2011-09-12 01:35:57 PDT
(In reply to comment #2)
> (From update of attachment 107021 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107021&action=review
> 
> > Source/WebCore/editing/htmlediting.cpp:669
> > -        if (n->hasTagName(liTag) || isListElement(n->parentNode()))
> > +        if (n->hasTagName(liTag) || (isListElement(n->parentNode()) && n->parentNode()->rendererIsEditable()))
> 
> This change seems wrong. You should be checking that n != root instead.

Fixed.
Comment 7 Shinya Kawanaka 2011-09-12 01:36:53 PDT
(In reply to comment #4)
> (From update of attachment 107021 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107021&action=review
> 
> >> LayoutTests/editing/execCommand/insert-list-in-noneditable-list-parent.html:9
> >> +document.writeln('The test has passed if it does not crash.<br><br>');
> > 
> > Why do we need so many brs?
> 
> Do we do correct thing?  i.e. we should get <ul><li><br></li></ul> inside the div after executing InsertUnorderedList.  You can use dump-as-markup to verify this.

I'm using dump-as-markup for the expected result now. The result seems OK.
Comment 8 Ryosuke Niwa 2011-09-12 09:05:02 PDT
Comment on attachment 107030 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107030&action=review

> LayoutTests/editing/execCommand/insert-list-in-noneditable-list-parent.html:1
> +<script src="../../resources/dump-as-markup.js"></script>

Can we have <!DOCTYPE  html>? or would that break the test?

> Source/WebCore/ChangeLog:8
> +        execCommand("InsertUnorderedList") was crashing if the parent node of the target is a kind of list element and it is not contenteditable.

Nit: This line seems too long.
Comment 9 Annie Sullivan 2011-09-12 16:18:59 PDT
*** Bug 67966 has been marked as a duplicate of this bug. ***
Comment 10 Shinya Kawanaka 2011-09-12 19:35:43 PDT
Created attachment 107130 [details]
Patch
Comment 11 Shinya Kawanaka 2011-09-12 19:38:42 PDT
(In reply to comment #8)
> (From update of attachment 107030 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107030&action=review
> 
> > LayoutTests/editing/execCommand/insert-list-in-noneditable-list-parent.html:1
> > +<script src="../../resources/dump-as-markup.js"></script>
> 
> Can we have <!DOCTYPE  html>? or would that break the test?

I've added it. It did not break the test. The test fails without my patch, and passes with my patch.

> > Source/WebCore/ChangeLog:8
> > +        execCommand("InsertUnorderedList") was crashing if the parent node of the target is a kind of list element and it is not contenteditable.
> 
> Nit: This line seems too long.

Fixed.
Comment 12 Ryosuke Niwa 2011-09-12 20:09:09 PDT
Annie pointed out that we should probably add test cases for

<ul><div id="div" contenteditable="true"A><script>
div.focus();
document.execCommand("InsertUnorderedList");
</script>

and

<ol><div id="div" contenteditable="true"A><script>
div.focus();
document.execCommand("InsertUnorderedList");
</script>

because she's working on a patch to make isListElement return false for dl.
Comment 13 Ryosuke Niwa 2011-09-12 20:10:17 PDT
(In reply to comment #12)
> Annie pointed out that we should probably add test cases for

I meant to say "I pointed out to Annie"
Comment 14 Shinya Kawanaka 2011-09-12 20:36:49 PDT
Created attachment 107134 [details]
Patch
Comment 15 Shinya Kawanaka 2011-09-12 20:38:05 PDT
I added test cases for ul and ol. Could you review it again?

(In reply to comment #12)
> Annie pointed out that we should probably add test cases for
> 
> <ul><div id="div" contenteditable="true"A><script>
> div.focus();
> document.execCommand("InsertUnorderedList");
> </script>
> 
> and
> 
> <ol><div id="div" contenteditable="true"A><script>
> div.focus();
> document.execCommand("InsertUnorderedList");
> </script>
> 
> because she's working on a patch to make isListElement return false for dl.
Comment 16 Ryosuke Niwa 2011-09-12 20:45:23 PDT
Does the test case still work?
Comment 17 Shinya Kawanaka 2011-09-12 20:50:37 PDT
(In reply to comment #16)
> Does the test case still work?

Yes. I have tested 4 patterns: ul only, ol only, dl only, and all of them.
All patterns crash without my patch, and they passes with my patch.
Comment 18 Ryosuke Niwa 2011-09-12 20:59:44 PDT
Comment on attachment 107134 [details]
Patch

(In reply to comment #17)
> (In reply to comment #16)
> > Does the test case still work?
> 
> Yes. I have tested 4 patterns: ul only, ol only, dl only, and all of them.
> All patterns crash without my patch, and they passes with my patch.

Great!
Comment 19 WebKit Review Bot 2011-09-12 22:56:36 PDT
Comment on attachment 107134 [details]
Patch

Clearing flags on attachment: 107134

Committed r95017: <http://trac.webkit.org/changeset/95017>
Comment 20 WebKit Review Bot 2011-09-12 22:56:42 PDT
All reviewed patches have been landed.  Closing bug.