Bug 121694 - Assertion failure in Range::nodeWillBeRemoved
Summary: Assertion failure in Range::nodeWillBeRemoved
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: BlinkMergeCandidate
Depends on:
Blocks:
 
Reported: 2013-09-20 10:46 PDT by Ryosuke Niwa
Modified: 2014-01-21 18:27 PST (History)
9 users (show)

See Also:


Attachments
Patch (3.90 KB, patch)
2014-01-08 04:36 PST, László Langó
no flags Details | Formatted Diff | Diff
Patch (4.00 KB, patch)
2014-01-15 02:13 PST, László Langó
no flags Details | Formatted Diff | Diff
Patch (5.27 KB, patch)
2014-01-16 06:28 PST, László Langó
no flags Details | Formatted Diff | Diff
Patch (4.80 KB, patch)
2014-01-20 03:20 PST, László Langó
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (455.79 KB, application/zip)
2014-01-20 04:30 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (484.00 KB, application/zip)
2014-01-20 04:58 PST, Build Bot
no flags Details
Patch (4.74 KB, patch)
2014-01-20 05:14 PST, László Langó
no flags Details | Formatted Diff | Diff
patch for landing (4.81 KB, patch)
2014-01-21 08:17 PST, László Langó
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2013-09-20 10:46:07 PDT
See https://chromium.googlesource.com/chromium/blink/+/407c1d7b2c45974aa614b3f847ffe9e8fce205fa

Confirmed the assertion reproduces in WebKit
Comment 1 László Langó 2014-01-08 04:36:51 PST
Created attachment 220625 [details]
Patch
Comment 2 Ryosuke Niwa 2014-01-13 08:51:44 PST
Comment on attachment 220625 [details]
Patch

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

Were you able to reproduce the assertion failure in WebKit without the fix?

> Source/WebCore/ChangeLog:8
> +        This patch changes Range::nodeWillBeRemoved() to accept remove node by replacing ASSERT to if-statement. Range::nodeWillBeRemoved() might be called with removed node, Node::parentNode() == nullptr, when DOMNodeRemovedFromDocument event handler calls removeChild(), directly or indirectly, for node being removed.

Please wrap the line somewhere.

> Source/WebCore/dom/Range.cpp:2092
> -    ASSERT(node->parentNode());
> +
> +    // FIXME: Once DOMNodeRemovedFromDocument mutation event removed, we
> +    // should change following if-statement to ASSERT(!node->parentNode).
> +    if (!node->parentNode())
> +        return;

This is a wrong fix. The problem here is that we're calling child.document().nodeWillBeRemoved after the node had already been removed in willRemoveChild (ContainerNode.cpp).
What we need to do instead is for every call site of dispatchChildRemovalEvents to check whether the mutation events had removed the parent,
and avoid calling Document::nodeWillBeRemoved if they did.
Comment 3 László Langó 2014-01-14 05:19:37 PST
(In reply to comment #2)
> (From update of attachment 220625 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=220625&action=review
> 
> Were you able to reproduce the assertion failure in WebKit without the fix?
> 

Yes, i got the assertion in EFL debug build.

> This is a wrong fix. The problem here is that we're calling child.document().nodeWillBeRemoved after the node had already been removed in willRemoveChild (ContainerNode.cpp). What we need to do instead is for every call site of dispatchChildRemovalEvents to check whether the mutation events had removed the parent, and avoid calling Document::nodeWillBeRemoved if they did.

I'm trying to fix this in that way.
Comment 4 László Langó 2014-01-15 02:13:04 PST
Created attachment 221248 [details]
Patch
Comment 5 Ryosuke Niwa 2014-01-15 09:21:47 PST
Comment on attachment 221248 [details]
Patch

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

> Source/WebCore/dom/ContainerNode.cpp:490
> +    if (child.parentNode())

We should check that child.parentNode() == this as done in ContainerNode::removeChild.

> LayoutTests/fast/dom/Range/remove-twice-crash.html:8
> +function $(id) { return document.getElementById(id); }

Please don't add this function.  It makes the code less readable.

> LayoutTests/fast/dom/Range/remove-twice-crash.html:20
> +range.selectNode($('sample'));

Call getElementById directly here and store it in a local variable.
Also, sample is a terrible name for this element.

> LayoutTests/fast/dom/Range/remove-twice-crash.html:23
> +    $('sample').parentNode.removeChild($('sample'));

Ditto.

> LayoutTests/fast/dom/Range/remove-twice-crash.html:28
> +$('container').outerHTML = 'PASS; NOT CRASHED';

We should add a description as to what kind of behavior this test is testing.
Otherwise the expected result doesn't tell us anything about this test.
Comment 6 László Langó 2014-01-16 06:28:08 PST
Created attachment 221373 [details]
Patch
Comment 7 Ryosuke Niwa 2014-01-16 10:53:59 PST
Comment on attachment 221373 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        This patch changes Range::nodeWillBeRemoved() to accept remove node
> +        by replacing ASSERT to if-statement. Range::nodeWillBeRemoved() might
> +        be called with removed node, Node::parentNode() == nullptr, when
> +        DOMNodeRemovedFromDocument event handler calls removeChild(), directly
> +        or indirectly, for node being removed.

This change log description is no longer accurate. Please revise it.

> Source/WebCore/dom/ContainerNode.cpp:493
>          disconnectSubframesIfNeeded(toContainerNode(child), RootAndDescendants);

On my second thought, we probably shouldn't be calling this function if child.parentNode() != this so the above if statement should probably be an early exit instead.

> Source/WebCore/dom/ContainerNode.h:174
> +    void willRemoveChild(Node& child);

This should probably appear immediately after notifyChildRemoved in accordance to the ordering in cpp file.

> LayoutTests/fast/dom/Range/remove-twice-crash-expected.txt:1
> +Range::nodeWillBeRemoved() might be called with removed node, Node::parentNode() == nullptr, when DOMNodeRemovedFromDocument event handler calls removeChild(), directly or indirectly, for node being removed.

I don't think "Node::parentNode() == nullptr" and "directly or indirectly" are adding any value here.
They're confusing as far as I read it.

> LayoutTests/fast/dom/Range/remove-twice-crash.html:4
> +<head>
> +<title>Test for Range::nodeWillBeRemoved()</title>
> +</head>

We don't need a title/head.

> LayoutTests/fast/dom/Range/remove-twice-crash.html:38
> +var mainDiv = document.getElementById('mainDiv');
> +mainDiv.outerHTML = 'PASS; NOT CRASHED';

It seems that the local variable mainDiv is unnecessary.
Comment 8 László Langó 2014-01-17 04:38:59 PST
(In reply to comment #7)
> (From update of attachment 221373 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=221373&action=review
> 
> > Source/WebCore/dom/ContainerNode.cpp:493
> >          disconnectSubframesIfNeeded(toContainerNode(child), RootAndDescendants);
> 
> On my second thought, we probably shouldn't be calling this function if child.parentNode() != this so the above if statement should probably be an early exit instead.
> 

I don't understand this. The above if is that

 Source/WebCore/dom/ContainerNode.cpp:492
     if (child.isContainerNode())

But this is at the end of the method, how could be this an early exit? Can you expose this for me? I'm rookie in DOM, but want to understand. :)
Comment 9 Ryosuke Niwa 2014-01-17 12:00:13 PST
(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 221373 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=221373&action=review
> > 
> > > Source/WebCore/dom/ContainerNode.cpp:493
> > >          disconnectSubframesIfNeeded(toContainerNode(child), RootAndDescendants);
> > 
> > On my second thought, we probably shouldn't be calling this function if child.parentNode() != this so the above if statement should probably be an early exit instead.
> > 
> 
> I don't understand this. The above if is that
> 
>  Source/WebCore/dom/ContainerNode.cpp:492
>      if (child.isContainerNode())
> 
> But this is at the end of the method, how could be this an early exit? Can you expose this for me? I'm rookie in DOM, but want to understand. :)

What I'm saying is that that condition is not sufficient.  If child had already been removed or inserted elsewhere, we should not be calling disconnectSubframesIfNeeded either.

Which means that we should have
if (child.parentNode() == this)
    return;
right before we call nodeWillBeRemoved instead.
Comment 10 Darin Adler 2014-01-17 12:19:28 PST
(In reply to comment #9)
> If child had already been removed or inserted elsewhere, we should not be calling disconnectSubframesIfNeeded either.

Agreed.

> Which means that we should have
> if (child.parentNode() == this)
>     return;
> right before we call nodeWillBeRemoved instead.

Even that doesn’t seem quite right to me for the case where the node was removed and reinserted into the same parent. Although perhaps in that case nodeWillBeRemoved will be harmless?
Comment 11 Ryosuke Niwa 2014-01-17 12:21:03 PST
(In reply to comment #10)
> (In reply to comment #9)
> > If child had already been removed or inserted elsewhere, we should not be calling disconnectSubframesIfNeeded either.
> 
> Agreed.
> 
> > Which means that we should have
> > if (child.parentNode() == this)
> >     return;
> > right before we call nodeWillBeRemoved instead.
> 
> Even that doesn’t seem quite right to me for the case where the node was removed and reinserted into the same parent. Although perhaps in that case nodeWillBeRemoved will be harmless?

I think we proceed to remove the child anyway in that case.
Comment 12 László Langó 2014-01-20 03:20:04 PST
Created attachment 221643 [details]
Patch
Comment 13 Andrei Bucur 2014-01-20 04:04:21 PST
Comment on attachment 221643 [details]
Patch

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

> Source/WebCore/dom/ContainerNode.cpp:491
> +    if (child.parentNode() != this)

Question: Should willRemoveChildren be patched as well? It's a similar function processing a list of nodes.
Comment 14 László Langó 2014-01-20 04:20:36 PST
(In reply to comment #13)
> (From update of attachment 221643 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=221643&action=review
> 
> > Source/WebCore/dom/ContainerNode.cpp:491
> > +    if (child.parentNode() != this)
> 
> Question: Should willRemoveChildren be patched as well? It's a similar function processing a list of nodes.

Good question. I don't see any connection between Range::nodeWillBeRemoved() and willRemoveChildren functions. And there is no ASSERT(node->parentNode()); in Range::nodeChildrenWillBeRemoved. I don't no is that a problem or not, but I see your point. To be honest, I think Range::nodeChildrenWillBeRemoved should call Range::nodeWillBeRemoved(), if it can, because that would be more readable.
Comment 15 László Langó 2014-01-20 04:24:57 PST
(In reply to comment #14)
> *... I don't know is that a problem or not, but I see your point.
Comment 16 Build Bot 2014-01-20 04:30:17 PST
Comment on attachment 221643 [details]
Patch

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

New failing tests:
fast/dom/Range/remove-twice-crash.html
Comment 17 Build Bot 2014-01-20 04:30:19 PST
Created attachment 221646 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 18 Build Bot 2014-01-20 04:58:36 PST
Comment on attachment 221643 [details]
Patch

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

New failing tests:
fast/dom/Range/remove-twice-crash.html
Comment 19 Build Bot 2014-01-20 04:58:39 PST
Created attachment 221648 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 20 László Langó 2014-01-20 05:11:49 PST
Ohh, I forgot to change the expected text after I changed the description.
Comment 21 László Langó 2014-01-20 05:14:24 PST
Created attachment 221653 [details]
Patch
Comment 22 Ryosuke Niwa 2014-01-20 09:09:11 PST
Comment on attachment 221653 [details]
Patch

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

> LayoutTests/ChangeLog:7
> +

You should credit the original blink patch here.
Comment 23 László Langó 2014-01-21 08:17:04 PST
Created attachment 221749 [details]
patch for landing
Comment 24 WebKit Commit Bot 2014-01-21 18:27:22 PST
Comment on attachment 221749 [details]
patch for landing

Clearing flags on attachment: 221749

Committed r162492: <http://trac.webkit.org/changeset/162492>
Comment 25 WebKit Commit Bot 2014-01-21 18:27:25 PST
All reviewed patches have been landed.  Closing bug.