Bug 67767

Summary:

Crashed in WebCore::AppendNodeCommand::create(WTF::PassRefPtr<WebCore::ContainerNode>, WTF::PassRefPtr<WebCore::Node>)

Product:

WebKit

Reporter:

Shinya Kawanaka <shinyak>

Component:

HTML Editing

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

jchaffraix, rniwa, shinyak, tkent, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

67668

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

Shinya Kawanaka

Reported 2011-09-08 00:03:46 PDT

testcase <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

Attachments
Patch (3.65 KB, patch) 2011-09-08 06:21 PDT, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Patch (3.88 KB, patch) 2011-09-09 00:50 PDT, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Patch (3.87 KB, patch) 2011-09-09 01:09 PDT, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Shinya Kawanaka

Comment 1 2011-09-08 06:21:13 PDT

Created attachment 106728 [details] Patch

Darin Adler

Comment 2 2011-09-08 08:39:21 PDT

Comment on attachment 106728 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106728&action=review This looks OK. It's a little strange to just do nothing if the node is not an element and the patch would be better if it explained why that is the correct thing to do, not just that it won’t crash. > LayoutTests/editing/execCommand/ident-crashes-topnode-is-text.html:9 > + document.writeln("This test ensures WebKit does not crash.<br><br>PASS"); The wording here is wrong. The test does not ensure that WebKit does not crash. The test has no effect on WebKit. It's good to say that the test has passed if it doesn’t crash, and also probably good to say as clearly as possible why there was a time when the test would crash. > LayoutTests/editing/execCommand/ident-crashes-topnode-is-text.html:12 > +<meta content="2"/><body onLoad="runTest();"> What is the significance of this "meta" element?

Shinya Kawanaka

Comment 3 2011-09-09 00:50:10 PDT

Created attachment 106847 [details] Patch

Shinya Kawanaka

Comment 4 2011-09-09 00:54:49 PDT

(In reply to comment #2) > (From update of attachment 106728 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=106728&action=review > > This looks OK. It's a little strange to just do nothing if the node is not an element and the patch would be better if it explained why that is the correct thing to do, not just that it won’t crash. I added some explanation in a test case. > > > LayoutTests/editing/execCommand/ident-crashes-topnode-is-text.html:9 > > + document.writeln("This test ensures WebKit does not crash.<br><br>PASS"); > > The wording here is wrong. The test does not ensure that WebKit does not crash. The test has no effect on WebKit. Fixed. > > It's good to say that the test has passed if it doesn’t crash, and also probably good to say as clearly as possible why there was a time when the test would crash. > > > LayoutTests/editing/execCommand/ident-crashes-topnode-is-text.html:12 > > +<meta content="2"/><body onLoad="runTest();"> > > What is the significance of this "meta" element? It did have no meaning. I removed it.

Kent Tamura

Comment 5 2011-09-09 00:56:31 PDT

(In reply to comment #3) > Created an attachment (id=106847) [details] > Patch Your patch already had r+ by Darin. So you may land the updated patch without review if you'd like to do so. 1. Update "Reviewed by" lines in ChangeLog entries for the reviewer. Reviewed by Darin Adler. 2. Post the path to this bug. 3. Don't set "review?", set "commit-queue?" instead.

Shinya Kawanaka

Comment 6 2011-09-09 01:09:24 PDT

Created attachment 106849 [details] Patch

Shinya Kawanaka

Comment 7 2011-09-09 01:10:32 PDT

(In reply to comment #5) > (In reply to comment #3) > > Created an attachment (id=106847) [details] [details] > > Patch > > Your patch already had r+ by Darin. > So you may land the updated patch without review if you'd like to do so. > 1. Update "Reviewed by" lines in ChangeLog entries for the reviewer. > Reviewed by Darin Adler. > 2. Post the path to this bug. > 3. Don't set "review?", set "commit-queue?" instead. Thanks for telling me the process! I updated the 'Reviewed by' lines in ChangeLog, and set commit-queue? mark.

WebKit Review Bot

Comment 8 2011-09-09 02:12:25 PDT

Comment on attachment 106849 [details] Patch Clearing flags on attachment: 106849 Committed r94841: <http://trac.webkit.org/changeset/94841>

WebKit Review Bot

Comment 9 2011-09-09 02:12:30 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.