Bug 67767 - Crashed in WebCore::AppendNodeCommand::create(WTF::PassRefPtr<WebCore::ContainerNode>, WTF::PassRefPtr<WebCore::Node>)
Summary: Crashed in WebCore::AppendNodeCommand::create(WTF::PassRefPtr<WebCore::Contai...
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:
Depends on:
Blocks: 67668
  Show dependency treegraph
 
Reported: 2011-09-08 00:03 PDT by Shinya Kawanaka
Modified: 2011-09-09 02:12 PDT (History)
5 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Shinya Kawanaka 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
Comment 1 Shinya Kawanaka 2011-09-08 06:21:13 PDT
Created attachment 106728 [details]
Patch
Comment 2 Darin Adler 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?
Comment 3 Shinya Kawanaka 2011-09-09 00:50:10 PDT
Created attachment 106847 [details]
Patch
Comment 4 Shinya Kawanaka 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.
Comment 5 Kent Tamura 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.
Comment 6 Shinya Kawanaka 2011-09-09 01:09:24 PDT
Created attachment 106849 [details]
Patch
Comment 7 Shinya Kawanaka 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.
Comment 8 WebKit Review Bot 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>
Comment 9 WebKit Review Bot 2011-09-09 02:12:30 PDT
All reviewed patches have been landed.  Closing bug.