Bug 129098 – WebKit crashes during execution of ReplaceSelectionCommand

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

[Rob Płóciennik]

Editing commands executed during ReplaceSelectionCommand::moveNodeOutOfAncestor() (utilized by makeInsertedContentRoundTrippableWithHTMLTreeBuilder and responsible for moving prohibited children out of their ancestors) are not applied properly when attempting to modify a non-editable element. As a result, the code that follows causes a crash.

[Rob Płóciennik]

Created attachment 224747 [details]
Proposed patch

[WebKit Commit Bot]

Attachment 224747 [details] did not pass style-queue:


ERROR: Source/WebCore/editing/CompositeEditCommand.cpp:1517:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Rob Płóciennik]

(In reply to comment #2)
> ERROR: Source/WebCore/editing/CompositeEditCommand.cpp:1517:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]

This in fact looks like a false positive.

[Rob Płóciennik]

Created attachment 224754 [details]
Simple test case (.html)

Attached a simple test case.

Inserting a horizontal rule anywhere in the paragraph should cause a crash.

[Rob Płóciennik]

The aforementioned crash is also related to the commit e32f78a11ed2f7bd9b46bb82fdcb426c3c65aace.

[Rob Płóciennik]

(In reply to comment #5)
> The aforementioned crash is also related to the commit e32f78a11ed2f7bd9b46bb82fdcb426c3c65aace.

http://trac.webkit.org/changeset/145253

[Ryosuke Niwa]

Comment on attachment 224747 [details]
Proposed patch

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

> Source/WebCore/ChangeLog:12
> +        Editing commands executed during
> +        ReplaceSelectionCommand::moveNodeOutOfAncestor() (utilized by
> +        makeInsertedContentRoundTrippableWithHTMLTreeBuilder and responsible for
> +        moving prohibited children out of their ancestors) are not applied
> +        properly when attempting to modify a non-editable element.

We should never be modifying non-editable element.
That's what we need to fix.
We shouldn't be working-around our bugs like this by ignoring contenteditable value.
r-.

[Marta Pawlowska]

I believe that final result of the code is correct according to w3c spec.

According to spec final effect of that Robert’s solution is correct. Even if at the end we move child to non-editable parent (after split).

Specification details:
https://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#the-inserthorizontalrule-command
-> step 2. If "p" is not an allowed child of the editing host of node, abort these steps. 
  --> https://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#fix-disallowed-ancestors <-- p is not allowed child of p
-> step 4: While node is not an allowed child of its parent, split the parent of the one-node list consisting of node. 
  --> split the parent:
    ---> https://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#split-the-parent

    
Do you have any other idea to implement it since embedding <hr> in <p> is also bad...

[Rob Płóciennik]

> Do you have any other idea to implement it since embedding <hr> in <p> is also bad...

This, I believe, relates to the solution found in Blink, which leaves the <hr> child inside <p> whenever it would end up messing with a non-editable ancestor. Besides the obvious advantage it seems to me that such approach effectively undermines the whole idea of moving out prohibited child elements.

[Marta Pawlowska]

w3c specification says specifically that we should split the parent and we should move child to grandpa ;) (parent of current parent), specification does not say if new parent must be editable. That is why I think your patch is correct.

On the other hand leaving hr nested in p is violation of older html standard so I do not like blink solution.

[Rob Płóciennik]

Our solution made a bad impression by admitting that we end up implicitly inserting a node under a non-editable element, yet we found this approach to be the lesser evil.
Also, the fact that approach similar to ours has been adopted in Gecko made us believe this was the right way.

Additionally, I would like to point out that a manner in which this goal is achieved is no different to approaches already present in the code.

[Ryosuke Niwa]

We DO NOT follow the editing specifications. The existing editing API specification has NOT been thoroughly reviewed by implementors including us, WebKit.

In fact, our current implementation is so different from the specification that following the spec. in some edge cases is almost never the right thing to do.

[Ryosuke Niwa]

(In reply to comment #11)
> Our solution made a bad impression by admitting that we end up implicitly inserting a node under a non-editable element, yet we found this approach to be the lesser evil.

It didn't make a bad impression. I noticed this without reading the change log. The only reason I commented on the change log was because you explicitly mentioned it. I would have r-ed it regardless.

We shouldn't be modifying a node outside of a root content editable node. In fact, as it stands, we support -webkit-user-modify CSS property to control the editability so we must constantly query whether a given node is editable or not since a mere act of inserting a new node could affect the editability in the presence of that CSS property.

[Ryosuke Niwa]

Comment on attachment 224747 [details]
Proposed patch

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

> LayoutTests/editing/execCommand/insertHorizontalRule-into-paragraph-crash-expected.txt:23
> +|   <p>
> +|     contenteditable="true"
> +|     id="testParagraph"
> +|     "Foo"
> +|   <hr>
> +|   <#selection-caret>
> +|   <p>
> +|     contenteditable="true"
> +|     "Bar"

This output is incorrect. The hr element must appear inside a single p element between "Foo" and "Bar".

We need to make insertHorizontalRule fail or simply strip the hr element
in makeInsertedContentRoundTrippableWithHTMLTreeBuilder
when we can't modify the root editable element in cases like this.

[Rob Płóciennik]

Created attachment 226682 [details]
Proposed patch

This approach attempts to make insertHorizontalRule fail when faced with either inserting a forbidden child or placing it under a non-editable element.

[Rob Płóciennik]

Created attachment 226683 [details]
Simple test case

[Ryosuke Niwa]

Comment on attachment 226682 [details]
Proposed patch

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

> LayoutTests/editing/execCommand/insertHorizontalRule-into-paragraph-crash.html:28
> +Markup.waitUntilDone();
> +
> +function runTests()
> +{
> +    Markup.dump("content", "Before execCommand('insertHorizontalRule')");
> +
> +    var node = document.getElementById("testParagraph").firstChild;
> +
> +    setSelectionCommand(node, 3, node, 3);
> +    document.execCommand("insertHorizontalRule", false);
> +
> +    Markup.dump("content", "After execCommand('insertHorizontalRule')");
> +
> +    Markup.notifyDone();
> +}

Just move all the script elements to after #content and run the code there.
Then we wouldn't need waitUntilDone/notifyDone calls.

> Source/WebCore/editing/CompositeEditCommand.cpp:232
> +
> +        // If the command has failed, unapply it entirely.
> +        if (hasFailed())
> +            composition()->unapply();

I'm not sure if this is desirable.  I think the user would like to see things happen even if failed to complete.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1158
> +    if (!makeInsertedContentRoundTrippableWithHTMLTreeBuilder(insertedNodes)) {
> +        this->reportFailure();
> +        return;
> +    }

I think it's better to simply exit here.

[Rob Płóciennik]

Created attachment 227173 [details]
Proposed patch

[Rob Płóciennik]

Comment on attachment 227173 [details]
Proposed patch

The following patch, instead of undoing the whole command, attempts to avoid inserting unmovable forbidden child nodes while executing the ReplaceSelectionCommand, using a method derived from existing code.
This approach has been chosen (instead of removing the offending nodes /after/ inserting them) to prevent the registration of undo steps for commands that have no effect. This behavior has been highlighted in a new test.

[Rob Płóciennik]

Any comments appreciated.

[Ryosuke Niwa]

Comment on attachment 227173 [details]
Proposed patch

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

> Source/WebCore/ChangeLog:18
> +        The following patch attempts to prevent insertion of unmovable forbidden
> +        child nodes during execution of ReplaceSelectionCommand. Additionally,
> +        it prevents the registration of undo steps for CompositeEditCommands that
> +        contain no commands (and have no effect).

Why are we doing this in the same patch?  We should split this into a separate patch if anything.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1165
> +    // A failure to unapply the last command means the text node split wasn't
> +    // the last command and warrants a crash.

Why?  It's extremely bad to crash in the release. A broken editing behavior is much better than losing all the data.

[Ryosuke Niwa]

Comment on attachment 227173 [details]
Proposed patch

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

> LayoutTests/editing/execCommand/insertHorizontalRule-into-paragraph-undo.html:17
> +    + "Performing undo twice should remove both 'Foo' and 'Baz' since the "
> +    + "failing InsertHorizontalRule command should not have its undo steps "
> +    + "registered.");

Why is InsertHorizontalRule so special?  There are other editing commands that fail to do anything useful in these edge cases.

[Rob Płóciennik]

(In reply to comment #22)
> Why is InsertHorizontalRule so special?  There are other editing commands that fail to do anything useful in these edge cases.

If there are really that many commands that could end up in registering an empty undo step then I see no reason for complicating things for the sake of simple ReplaceSelectionCommand. Let's try a simpler approach then...

[Rob Płóciennik]

Created attachment 228048 [details]
[test] Proposed patch

[Build Bot]

Comment on attachment 228048 [details]
[test] Proposed patch

Attachment 228048 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5957258252910592

New failing tests:
editing/pasteboard/paste-text-011.html
editing/pasteboard/paste-text-016.html
editing/pasteboard/block-wrappers-necessary.html

[Build Bot]

Created attachment 228055 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 228048 [details]
[test] Proposed patch

Attachment 228048 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5429642458562560

New failing tests:
editing/pasteboard/paste-text-011.html
editing/pasteboard/paste-text-016.html
editing/pasteboard/block-wrappers-necessary.html

[Build Bot]

Created attachment 228060 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Rob Płóciennik]

Created attachment 229131 [details]
Proposed patch

Let's try that again...

[Rob Płóciennik]

Comment on attachment 229131 [details]
Proposed patch

The following approach prevents the insertion of unmovable forbidden child nodes that otherwise would have to be removed.

[Rob Płóciennik]

*** This bug has been marked as a duplicate of bug 132633 ***

[Csaba Osztrogonác]

Comment on attachment 229131 [details]
Proposed patch

Cleared review? from attachment 229131 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).