[Webkit-unassigned] [Bug 121694] Assertion failure in Range::nodeWillBeRemoved

Thu Jan 16 10:56:24 PST 2014

https://bugs.webkit.org/show_bug.cgi?id=121694

--- Comment #7 from Ryosuke Niwa <rniwa at webkit.org>  2014-01-16 10:53:59 PST ---
(From update of attachment 221373)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.