Bug 38232 – REGRESSION: crash in WebCore::CompositeEditCommand::splitTreeToNode when indenting in an empty li

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ojan Vafai]

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.

[Tony Chang]

Created attachment 54522 [details]
Patch

[Tony Chang]

(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.

[Ojan Vafai]

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>

[Tony Chang]

Created attachment 55199 [details]
Patch

[Tony Chang]

(In reply to comment #4)
> Created an attachment (id=55199) [details]
> Patch

Switched to a dump-as-markup test as Ojan suggested.

[Ojan Vafai]

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.

[Tony Chang]

Committed r59739: <http://trac.webkit.org/changeset/59739>