Notify nodes removal to Range/Selection after dispatching blur and mutation event

Notify nodes removal to Range/Selection after dispatching blur and mutation event

This patch changes notifying nodes removal to Range/Selection after dispatching blur and mutation event. In willRemoveChildren(), like willRemoveChild(); r115686 did same change, although it didn't change willRemoveChildren().

The issue 295010, use-after-free, is caused by setting removed node to Selection in mutation event handler.

BUG=295010
TEST=LayoutTests/fast/dom/Range/range-created-during-remove-children.html, LayoutTests/editing/selection/selection-change-in-mutation-event-by-remove-children.html, LayoutTests/editing/selection/selection-change-in-blur-event-by-remove-children.html
R=tkent@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=159007

[yosin_UTC9, 2013-10-04 04:36:26]

Could you review this patch?
Thanks in advance.

[tkent, 2013-10-04 05:21:34]

https://codereview.chromium.org/25389004/diff/1/Source/core/dom/ContainerNode...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/25389004/diff/1/Source/core/dom/ContainerNode...
Source/core/dom/ContainerNode.cpp:414:
container->document().nodeChildrenWillBeRemoved(container); // e.g. mutation
event listener can create a new range.
After this line, It is possible to dispatch 'blur' event or
'DOMSubtreeModfified' event.

[yosin_UTC9, 2013-10-04 08:09:30]

PTAL

https://codereview.chromium.org/25389004/diff/1/Source/core/dom/ContainerNode...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/25389004/diff/1/Source/core/dom/ContainerNode...
Source/core/dom/ContainerNode.cpp:414:
container->document().nodeChildrenWillBeRemoved(container); // e.g. mutation
event listener can create a new range.
On 2013/10/04 05:21:39, tkent wrote:
> After this line, It is possible to dispatch 'blur' event or
> 'DOMSubtreeModfified' event.

It is safe in 'DOMSubtreeModfified' event handler, because node is already
removed in tree. Setting removed node throws an exception.

For 'blur', you're right. I changed order in ContainerNode::removeChildren() as
Container::removeChild().

[tkent, 2013-10-06 23:46:54]

https://codereview.chromium.org/25389004/diff/1/Source/core/dom/ContainerNode...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/25389004/diff/1/Source/core/dom/ContainerNode...
Source/core/dom/ContainerNode.cpp:414:
container->document().nodeChildrenWillBeRemoved(container); // e.g. mutation
event listener can create a new range.
On 2013/10/04 08:09:30, Yoshifumi Inoue wrote:
> On 2013/10/04 05:21:39, tkent wrote:
> > After this line, It is possible to dispatch 'blur' event or
> > 'DOMSubtreeModfified' event.
> 
> It is safe in 'DOMSubtreeModfified' event handler, because node is already
> removed in tree. Setting removed node throws an exception.
> 
> For 'blur', you're right. I changed order in ContainerNode::removeChildren()
as
> Container::removeChild().

The comment in removeChildren says:

         // This must be later than willRemoveChildren, which might change focus
         // state of a child.
         document().removeFocusedElementOfSubtree(this, true);

It seems removeChild() is wrong.

Can we move only nodeChildrenWillBeRemoved() call after
Document::removeFocusedElementOfSubtree()?

[yosin_UTC9, 2013-10-07 05:53:51]

PTAL

[tkent, 2013-10-07 05:55:13]

lgtm

[yosin_UTC9, 2013-10-07 06:09:29]

Committed patchset #3 manually as r159007 (presubmit successful).