Bug 67918

Summary:

Crashes in WebCore::InsertListCommand::unlistifyParagraph

Product:

WebKit

Reporter:

Shinya Kawanaka <shinyak>

Component:

HTML Editing

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

rniwa, shinyak, sullivan, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none

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.