Bug 129098 - WebKit crashes during execution of ReplaceSelectionCommand
Summary: WebKit crashes during execution of ReplaceSelectionCommand
Status: RESOLVED DUPLICATE of bug 132633
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:
 
Reported: 2014-02-20 05:02 PST by Rob Płóciennik
Modified: 2014-07-16 03:09 PDT (History)
4 users (show)

See Also:


Attachments
Proposed patch (14.91 KB, patch)
2014-02-20 05:05 PST, Rob Płóciennik
rniwa: review-
Details | Formatted Diff | Diff
Simple test case (.html) (1.64 KB, text/html)
2014-02-20 06:43 PST, Rob Płóciennik
no flags Details
Proposed patch (11.10 KB, patch)
2014-03-14 04:34 PDT, Rob Płóciennik
no flags Details | Formatted Diff | Diff
Simple test case (1.80 KB, text/html)
2014-03-14 04:34 PDT, Rob Płóciennik
no flags Details
Proposed patch (19.63 KB, patch)
2014-03-19 03:15 PDT, Rob Płóciennik
no flags Details | Formatted Diff | Diff
[test] Proposed patch (11.95 KB, patch)
2014-03-28 07:09 PDT, Rob Płóciennik
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion (514.48 KB, application/zip)
2014-03-28 09:34 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 (485.54 KB, application/zip)
2014-03-28 10:08 PDT, Build Bot
no flags Details
Proposed patch (10.91 KB, patch)
2014-04-11 07:33 PDT, Rob Płóciennik
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rob Płóciennik 2014-02-20 05:02:45 PST
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.
Comment 1 Rob Płóciennik 2014-02-20 05:05:42 PST
Created attachment 224747 [details]
Proposed patch
Comment 2 WebKit Commit Bot 2014-02-20 05:07:03 PST
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.
Comment 3 Rob Płóciennik 2014-02-20 05:14:46 PST
(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.
Comment 4 Rob Płóciennik 2014-02-20 06:43:46 PST
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.
Comment 5 Rob Płóciennik 2014-02-20 06:45:59 PST
The aforementioned crash is also related to the commit e32f78a11ed2f7bd9b46bb82fdcb426c3c65aace.
Comment 6 Rob Płóciennik 2014-02-20 06:59:23 PST
(In reply to comment #5)
> The aforementioned crash is also related to the commit e32f78a11ed2f7bd9b46bb82fdcb426c3c65aace.

http://trac.webkit.org/changeset/145253
Comment 7 Ryosuke Niwa 2014-02-20 13:45:33 PST
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-.
Comment 8 Marta Pawlowska 2014-02-24 08:42:25 PST
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...
Comment 9 Rob Płóciennik 2014-02-24 23:43:46 PST
> 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.
Comment 10 Marta Pawlowska 2014-02-24 23:57:26 PST
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.
Comment 11 Rob Płóciennik 2014-02-25 00:10:03 PST
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.
Comment 12 Ryosuke Niwa 2014-02-25 03:49:55 PST
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.
Comment 13 Ryosuke Niwa 2014-02-25 03:54:28 PST
(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.
Comment 14 Ryosuke Niwa 2014-02-25 04:00:08 PST
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.
Comment 15 Rob Płóciennik 2014-03-14 04:34:03 PDT
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.
Comment 16 Rob Płóciennik 2014-03-14 04:34:58 PDT
Created attachment 226683 [details]
Simple test case
Comment 17 Ryosuke Niwa 2014-03-14 17:43:28 PDT
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.
Comment 18 Rob Płóciennik 2014-03-19 03:15:00 PDT
Created attachment 227173 [details]
Proposed patch
Comment 19 Rob Płóciennik 2014-03-19 05:24:46 PDT
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.
Comment 20 Rob Płóciennik 2014-03-21 07:40:51 PDT
Any comments appreciated.
Comment 21 Ryosuke Niwa 2014-03-28 00:50:14 PDT
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.
Comment 22 Ryosuke Niwa 2014-03-28 00:53:20 PDT
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.
Comment 23 Rob Płóciennik 2014-03-28 01:52:40 PDT
(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...
Comment 24 Rob Płóciennik 2014-03-28 07:09:58 PDT
Created attachment 228048 [details]
[test] Proposed patch
Comment 25 Build Bot 2014-03-28 09:34:22 PDT
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
Comment 26 Build Bot 2014-03-28 09:34:25 PDT
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
Comment 27 Build Bot 2014-03-28 10:08:06 PDT
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
Comment 28 Build Bot 2014-03-28 10:08:09 PDT
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
Comment 29 Rob Płóciennik 2014-04-11 07:33:30 PDT
Created attachment 229131 [details]
Proposed patch

Let's try that again...
Comment 30 Rob Płóciennik 2014-04-11 11:06:35 PDT
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.
Comment 31 Rob Płóciennik 2014-06-09 00:19:00 PDT

*** This bug has been marked as a duplicate of bug 132633 ***
Comment 32 Csaba Osztrogonác 2014-07-16 03:09:48 PDT
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).