Bug 121694 – Assertion failure in Range::nodeWillBeRemoved

• Component: DOM
• Status: RESOLVED FIXED

[Ryosuke Niwa]

See https://chromium.googlesource.com/chromium/blink/+/407c1d7b2c45974aa614b3f847ffe9e8fce205fa

Confirmed the assertion reproduces in WebKit

[László Langó]

Created attachment 220625 [details]
Patch

[Ryosuke Niwa]

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.

[László Langó]

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

[László Langó]

Created attachment 221248 [details]
Patch

[Ryosuke Niwa]

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.

[László Langó]

Created attachment 221373 [details]
Patch

[Ryosuke Niwa]

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.

[László Langó]

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

[Ryosuke Niwa]

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

[Darin Adler]

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

[Ryosuke Niwa]

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

[László Langó]

Created attachment 221643 [details]
Patch

[Andrei Bucur]

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.

[László Langó]

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

[László Langó]

(In reply to comment #14)
> *... I don't know is that a problem or not, but I see your point.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[László Langó]

Ohh, I forgot to change the expected text after I changed the description.

[László Langó]

Created attachment 221653 [details]
Patch

[Ryosuke Niwa]

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.

[László Langó]

Created attachment 221749 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 221749 [details]
patch for landing

Clearing flags on attachment: 221749

Committed r162492: <http://trac.webkit.org/changeset/162492>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.