67918 – Crashes in WebCore::InsertListCommand::unlistifyParagraph

RESOLVED FIXED 67918

Crashes in WebCore::InsertListCommand::unlistifyParagraph

https://bugs.webkit.org/show_bug.cgi?id=67918

Summary Crashes in WebCore::InsertListCommand::unlistifyParagraph

Shinya Kawanaka

Reported 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

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
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Shinya Kawanaka

Comment 1 2011-09-11 22:26:11 PDT

Created attachment 107021 [details] Patch

Ryosuke Niwa

Comment 2 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.

Ryosuke Niwa

Comment 3 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?

Ryosuke Niwa

Comment 4 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.

Shinya Kawanaka

Comment 5 2011-09-12 01:35:00 PDT

Created attachment 107030 [details] Patch

Shinya Kawanaka

Comment 6 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.

Shinya Kawanaka

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

Ryosuke Niwa

Comment 8 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.

Annie Sullivan

Comment 9 2011-09-12 16:18:59 PDT

*** Bug 67966 has been marked as a duplicate of this bug. ***

Shinya Kawanaka

Comment 10 2011-09-12 19:35:43 PDT

Created attachment 107130 [details] Patch

Shinya Kawanaka

Comment 11 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.

Ryosuke Niwa

Comment 12 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.

Ryosuke Niwa

Comment 13 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"

Shinya Kawanaka

Comment 14 2011-09-12 20:36:49 PDT

Created attachment 107134 [details] Patch

Shinya Kawanaka

Comment 15 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.

Ryosuke Niwa

Comment 16 2011-09-12 20:45:23 PDT

Does the test case still work?

Shinya Kawanaka

Comment 17 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.

Ryosuke Niwa

Comment 18 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!

WebKit Review Bot

Comment 19 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>

WebKit Review Bot

Comment 20 2011-09-12 22:56:42 PDT

All reviewed patches have been landed. Closing bug.

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

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component HTML Editing

Assignee

Nobody

Reported

2011-09-11 20:58 PDT

Modified

2011-09-12 22:56 PDT History

CC List

4 users Show

URL

Keywords

Duplicates (1)

67966 View as bug list

Depends on

Blocks