See https://chromium.googlesource.com/chromium/blink/+/407c1d7b2c45974aa614b3f847ffe9e8fce205fa Confirmed the assertion reproduces in WebKit
Created attachment 220625 [details] Patch
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.
(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.
Created attachment 221248 [details] Patch
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.
Created attachment 221373 [details] Patch
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.
(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. :)
(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.
(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?
(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.
Created attachment 221643 [details] Patch
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.
(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.
(In reply to comment #14) > *... I don't know is that a problem or not, but I see your point.
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
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 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
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
Ohh, I forgot to change the expected text after I changed the description.
Created attachment 221653 [details] Patch
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.
Created attachment 221749 [details] patch for landing
Comment on attachment 221749 [details] patch for landing Clearing flags on attachment: 221749 Committed r162492: <http://trac.webkit.org/changeset/162492>
All reviewed patches have been landed. Closing bug.