Bug 67764 – Crash in WebCore::CompositeEditCommand::insertNodeAt

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

[Shinya Kawanaka]

ASSERT(destination.deepEquivalent().anchorNode()->inDocument()); fails.

<div contenteditable="true" id="div"><hkern><span contenteditable="false"><dl>000A0<script>
var sel = window.getSelection();
sel.setPosition(div, 2000000000);
document.execCommand("Delete");
</script>

[Annie Sullivan]

I'm going to look into this today. A quick glance in the debugger shows that mergeParagraphs() gets called with the following selections (S for startParagraph, E for endParagraph, and D for destination):

BODY	0x131d05cb0
SE	DIV	0x131d05ff0
D		HKERN	0x131d064b0
			SPAN	0x131d066a0
				DL	0x131d06980
					#text	0x131d07230 "000A0"
					SCRIPT	0x131d07310
						#text	0x131d07550 "\nvar sel = window.getSelection();\nsel.setPosition(div, 2000000000);\ndocument.execCommand("Delete");\n"
afterChildren, offset:0

It seems weird to merge paragraphs that aren't adjacent. What ends up happening is that cleanupAfterDeletion removes the DIV from the document entirely and then the ASSERT fires. The comments indicate that cleanupAfterDeletion is meant to help with list/table problems, so it seems like the codepath is getting called for something it wasn't intended to handle.

I am also going to try and reduce this test case; I think it was generated by a fuzzer, and hopefully we don't need an svg element, a contenteditable=false span, a block tag within a span, and an invalid position to reproduce the problem.

[Ryosuke Niwa]

Sigh... mergeParagraphs again. I personally won't spend too much time on crashes caused by mergeParagraphs because they're insanely hard to fix correctly. I'd focus on other high priority bugs if I were you.

[Annie Sullivan]

Created attachment 107082 [details]
Patch

It looks like changing the mergeParagraphs algorithm for an edge case like this introduces a lot of risk. So this patch changes the ASSERT to an early bail, which fixes the crash.

[Ryosuke Niwa]

Comment on attachment 107082 [details]
Patch

This is wrong. We need to fix cleanupAfterDeletion not to remove the destination.

[Annie Sullivan]

(In reply to comment #4)
> (From update of attachment 107082 [details])
> This is wrong. We need to fix cleanupAfterDeletion not to remove the destination.

I definitely agree it's not an ideal fix. It does fix a potential crash without adding a lot of code, though. Do you think it's worth spending the time to fix this correctly? I wasn't able to reduce the test case further than what's in the layout test: <div contenteditable><hkern><span contenteditable="false">text

[Ryosuke Niwa]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 107082 [details] [details])
> > This is wrong. We need to fix cleanupAfterDeletion not to remove the destination.
> 
> I definitely agree it's not an ideal fix. It does fix a potential crash without adding a lot of code, though. Do you think it's worth spending the time to fix this correctly? I wasn't able to reduce the test case further than what's in the layout test: <div contenteditable><hkern><span contenteditable="false">text

Yes. That assertion has caught us many bugs in the past, and I'm very reluctant to remove it. I'm okay with adding an early exit there in addition to the assertion but we shouldn't be removing the assertion.

[Parag Radke]

Created attachment 116615 [details]
Proposed patch

Added a check for the root editability of the parent node. If parent is not root editable, we should not remove its child and hence not prune it

[Ryosuke Niwa]

Comment on attachment 116615 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116615&action=review

> Source/WebCore/ChangeLog:8
> +        Added a check for root editability of node's parent.

You should explain why this fixes the crash and why your fix is correct.

> Source/WebCore/editing/CompositeEditCommand.cpp:232
> +    if (parent && !parent->rootEditableElement())

I think what you're trying to check is that parent->isContentEditable().

> LayoutTests/ChangeLog:8
> +        Added a test case for the crash-67764.

crash-67764 doesn't tell me anything about the kind of crash you're fixing. Please explain rather than giving the bug number.
Also, the bug number is redundant because it's already given at line 4.

> LayoutTests/editing/deleting/delete-block-merge-contents-025.html:6
> +var sel = window.getSelection();
> +sel.setPosition(div, 2000000000);
> +document.execCommand("Delete");

This should be converted to a dumpAsText test. Just do:
if (window.layoutTestController)
    layoutTestController.dumpAsText();

[Parag Radke]

Created attachment 129024 [details]
Updated Patch

This patch is to make an early exit without removing the ASSERT.

[Ryosuke Niwa]

Comment on attachment 129024 [details]
Updated Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129024&action=review

r- due to the bad test description and comment.

> Source/WebCore/editing/CompositeEditCommand.cpp:1037
> +            // for VisiblePositions V1 and V2, it's possible V1 != V2 but still editing code treat them as same. 
> +            // Ref : FIXME @ Position.h :inline bool operator==(const Position& a, const Position& b) 

I don't understand why the fact two VisiblePositions being treated same is resulting in this code since that's not true.
I think what you meant is Position. But the fact multiple Positions can be treated as the same position is a well-known fact and doesn't deserve a comment.
Instead, you should explain why you're calling rendersInDifferentPosition in this particular situation.

> LayoutTests/editing/deleting/delete-block-merge-contents-025-expected.txt:1
> +This is for bug-67764,to Pass this testcase it should not crash.

Please explain what the test is doing. Saying that it's for the bug 67764 doesn't explain anything about the nature of the test.

[Parag Radke]

Created attachment 129241 [details]
Updated Patch

Updated patch with review comments implemented.

[Ryosuke Niwa]

Comment on attachment 129241 [details]
Updated Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129241&action=review

> Source/WebCore/ChangeLog:8
> +        If caret position after deletion and destination position where to move paragraph

Nit: "where to move paragraph" is making this sentence more confusing. "destination position" should be clear enough.

> Source/WebCore/ChangeLog:10
> +        Hence ASSERT.

The bug summary says crash though?

[Parag Radke]

Created attachment 129416 [details]
Updated Patch

Updated patch with all review comments implemented.

[Ryosuke Niwa]

Comment on attachment 129416 [details]
Updated Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129416&action=review

Please address nits below.

> Source/WebCore/ChangeLog:8
> +        If caret position after deletion and destination position coinsides than

Nit: s/ than/, then/

> LayoutTests/editing/deleting/delete-block-merge-contents-025-expected.txt:1
> +This is to test a usecase in which caret position after deletion and the destination position where to move the paragraph, coinsides. To pass this testcase it should not crash.

Same comment about "where to move the paragraph, ".

> LayoutTests/editing/deleting/delete-block-merge-contents-025.html:1
> +<html>

No DOCTYPE?

[Parag Radke]

Created attachment 129425 [details]
Updated Patch

Updated patch with all review comments implemented.

[Ryosuke Niwa]

Comment on attachment 129425 [details]
Updated Patch

Great. Thanks for the patch! r=me.

[WebKit Review Bot]

Comment on attachment 129425 [details]
Updated Patch

Clearing flags on attachment: 129425

Committed r109218: <http://trac.webkit.org/changeset/109218>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.