RESOLVED FIXED Bug 67767
Crashed in WebCore::AppendNodeCommand::create(WTF::PassRefPtr<WebCore::ContainerNode>, WTF::PassRefPtr<WebCore::Node>) 
https://bugs.webkit.org/show_bug.cgi?id=67767
Summary Crashed in WebCore::AppendNodeCommand::create(WTF::PassRefPtr<WebCore::Contai...
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
DetailsFormatted DiffDiff
Patch (3.88 KB, patch)
2011-09-09 00:50 PDT, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch (3.87 KB, patch)
2011-09-09 01:09 PDT, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
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.