Bug 38232 - REGRESSION: crash in WebCore::CompositeEditCommand::splitTreeToNode when indenting in an empty li
Summary: REGRESSION: crash in WebCore::CompositeEditCommand::splitTreeToNode when inde...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P5 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-27 17:10 PDT by Ojan Vafai
Modified: 2010-05-18 20:30 PDT (History)
5 users (show)

See Also:


Attachments
test case (174 bytes, text/html)
2010-04-27 17:10 PDT, Ojan Vafai
no flags Details
Patch (4.28 KB, patch)
2010-04-27 22:16 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (4.65 KB, patch)
2010-05-05 20:04 PDT, Tony Chang
ojan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2010-04-27 17:10:11 PDT
Created attachment 54477 [details]
test case

Encountered in Google Docs. See http://crbug.com/42633 for more details.

Test case attached.

Regression range: 53845:53990

Likely a result of http://trac.webkit.org/changeset/53927.

Possibly related to bug 38231 even though that's not a recent regression.
Comment 1 Tony Chang 2010-04-27 22:16:14 PDT
Created attachment 54522 [details]
Patch
Comment 2 Tony Chang 2010-04-27 22:19:42 PDT
(In reply to comment #0)
> Likely a result of http://trac.webkit.org/changeset/53927.

Yup, that change was completely wrong.  The guy who wrote it didn't know what he was doing.  Hopefully that guy understands this better now.

The new patch reverts r53927 and adds a comment explaining what the code is supposed to do.  It includes a new fix to the problem.

The problem was that some nodes are block elements even though they can't contain text or other elements (e.g., <hr>).  In this case, we would try to split the <hr>, but that doesn't make sense.  Rather than try to split, we just treat the individual node as a block.
Comment 3 Ojan Vafai 2010-04-28 07:47:24 PDT
Comment on attachment 54522 [details]
Patch

The code changes look correct to me.

> +++ b/LayoutTests/editing/execCommand/crash-indenting-list-item.html
> @@ -0,0 +1,9 @@
> +<div contentEditable><ul><li id='foo'></li></ul></div>
> +<script>
> +if (window.layoutTestController)
> +    layoutTestController.dumpAsText();
> +window.getSelection().setBaseAndExtent(foo, 0, foo, 0);
> +// This test passes if it does not crash.
> +document.execCommand('indent', false, null);
> +document.getElementById("foo").innerText = "PASSED";
> +</script>

We now have dump-as-markup that's perfect for something like this. Ideally this test would also show that the li got indented (which dump-as-markup would show). All you need to do to make this work with dump-as-markup is add the following to the top of the file:
<script src="../../resources/dump-as-markup.js"></script>
Comment 4 Tony Chang 2010-05-05 20:04:56 PDT
Created attachment 55199 [details]
Patch
Comment 5 Tony Chang 2010-05-05 20:05:28 PDT
(In reply to comment #4)
> Created an attachment (id=55199) [details]
> Patch

Switched to a dump-as-markup test as Ojan suggested.
Comment 6 Ojan Vafai 2010-05-18 10:53:49 PDT
Comment on attachment 55199 [details]
Patch

This looks good. The old code was clearly violating an assert in splitTreeToNode. I would bet that an audit of call calls to splitTreeToNode would find other cases like this. I almost wonder if splitTreeToNode should just deal with this case (i.e. not split and just return the passed in Node), but I'm on the fence. This is certainly fine as a fix to the crash.
Comment 7 Tony Chang 2010-05-18 20:30:43 PDT
Committed r59739: <http://trac.webkit.org/changeset/59739>