Call insertedInto or removedFrom before childrenChanged

Source/core/dom/ContainerNode.cpp

Index: Source/core/dom/ContainerNode.cpp
diff --git a/Source/core/dom/ContainerNode.cpp b/Source/core/dom/ContainerNode.cpp
index 8964573589802af05f0155a52ba02ac8b493e9a8..e80732962d4221c928e6e49cecf2e14c82ea2477 100644
--- a/Source/core/dom/ContainerNode.cpp
+++ b/Source/core/dom/ContainerNode.cpp
@@ -303,6 +303,8 @@ void ContainerNode::parserInsertBefore(PassRefPtrWillBeRawPtr<Node> newChild, No
     if (nextChild.previousSibling() == newChild || &nextChild == newChild) // nothing to do
         return;
 
+    RefPtrWillBeRawPtr<Node> protect(this);
+
     if (document() != newChild->document())
         document().adoptNode(newChild.get(), ASSERT_NO_EXCEPTION);
 
@@ -312,10 +314,7 @@ void ContainerNode::parserInsertBefore(PassRefPtrWillBeRawPtr<Node> newChild, No
 
     ChildListMutationScope(*this).childAdded(*newChild);
 
-    ChildrenChange change = {ChildInserted, newChild->previousSibling(), &nextChild, ChildrenChangeSourceParser};
-    childrenChanged(change);
-
-    notifyNodeInserted(*newChild);
+    notifyNodeInserted(*newChild, ChildrenChangeSourceParser);
 }
 
 PassRefPtrWillBeRawPtr<Node> ContainerNode::replaceChild(PassRefPtrWillBeRawPtr<Node> newChild, PassRefPtrWillBeRawPtr<Node> oldChild, ExceptionState& exceptionState)
@@ -568,9 +567,9 @@ PassRefPtrWillBeRawPtr<Node> ContainerNode::removeChild(PassRefPtrWillBeRawPtr<N
         Node* prev = child->previousSibling();
         Node* next = child->nextSibling();
         removeBetween(prev, next, *child);
+        notifyNodeRemoved(*child);
         ChildrenChange change = {ChildRemoved, prev, next, ChildrenChangeSourceAPI};
         childrenChanged(change);
-        notifyNodeRemoved(*child);
     }
     dispatchSubtreeModifiedEvent();
     return child;
@@ -617,8 +616,8 @@ void ContainerNode::parserRemoveChild(Node& oldChild)
     removeBetween(prev, next, oldChild);
 
     ChildrenChange change = {ChildRemoved, prev, next, ChildrenChangeSourceParser};
-    childrenChanged(change);
     notifyNodeRemoved(oldChild);
+    childrenChanged(change);
 }
 
 // this differs from other remove functions because it forcibly removes all the children,
@@ -653,24 +652,30 @@ void ContainerNode::removeChildren()
         document().nodeChildrenWillBeRemoved(*this);
     }
 
-
+    // FIXME: Remove this NodeVector. Right now WebPluginContainerImpl::m_element is a
+    // raw ptr which means the code below can drop the last ref to a plugin element and
+    // then the code in UpdateSuspendScope::performDeferredWidgetTreeOperations will
+    // try to destroy the plugin which will be a use-after-free. We should use a RefPtr
+    // in the WebPluginContainerImpl instead.
     NodeVector removedChildren;
     {
         HTMLFrameOwnerElement::UpdateSuspendScope suspendWidgetHierarchyUpdates;
+
         {
             NoEventDispatchAssertion assertNoEventDispatch;
-            removedChildren.reserveInitialCapacity(countChildren());
-            while (m_firstChild) {
-                removedChildren.append(m_firstChild);
-                removeBetween(0, m_firstChild->nextSibling(), *m_firstChild);
+            ScriptForbiddenScope forbidScript;
+
+            removedChildren.reserveInitialCapacity(childNodeCount());
+
+            while (RefPtrWillBeRawPtr<Node> child = m_firstChild) {
+                removeBetween(0, child->nextSibling(), *child);
+                removedChildren.append(child.get());
+                notifyNodeRemoved(*child);
             }
         }
 
         ChildrenChange change = {AllChildrenRemoved, nullptr, nullptr, ChildrenChangeSourceAPI};
         childrenChanged(change);
-
-        for (size_t i = 0; i < removedChildren.size(); ++i)
-            notifyNodeRemoved(*removedChildren[i]);
     }
 
     dispatchSubtreeModifiedEvent();
@@ -748,11 +753,11 @@ void ContainerNode::parserAppendChild(PassRefPtrWillBeRawPtr<Node> newChild)
     ASSERT(!newChild->isDocumentFragment());
     ASSERT(!isHTMLTemplateElement(this));
 
+    RefPtrWillBeRawPtr<Node> protect(this);

[adamk, 2014/07/24 21:35:59]

What's this about?

[esprehn, 2014/07/24 22:50:18]

Nothing is protecting |this| in these parser methods like in the non-parser ones
even though they run script. I added the protects just like in my original patch
(which you also reviewed). I guess I can leave that out?

+
     if (document() != newChild->document())
         document().adoptNode(newChild.get(), ASSERT_NO_EXCEPTION);
 
-    Node* last = m_lastChild;
-
     {
         NoEventDispatchAssertion assertNoEventDispatch;
         ScriptForbiddenScope forbidScript;
@@ -763,12 +768,10 @@ void ContainerNode::parserAppendChild(PassRefPtrWillBeRawPtr<Node> newChild)
         ChildListMutationScope(*this).childAdded(*newChild);
     }
 
-    ChildrenChange change = {ChildInserted, last, nullptr, ChildrenChangeSourceParser};
-    childrenChanged(change);
-    notifyNodeInserted(*newChild);
+    notifyNodeInserted(*newChild, ChildrenChangeSourceParser);
 }
 
-void ContainerNode::notifyNodeInserted(Node& root)
+void ContainerNode::notifyNodeInserted(Node& root, ChildrenChangeSource source)
 {
     ASSERT(!NoEventDispatchAssertion::isEventDispatchForbidden());
 
@@ -780,6 +783,15 @@ void ContainerNode::notifyNodeInserted(Node& root)
     NodeVector postInsertionNotificationTargets;
     notifyNodeInsertedInternal(root, postInsertionNotificationTargets);
 
+    // ShadowRoots are not real children, we don't need to tell host that it's

[adamk, 2014/07/24 21:35:59]

Why are you changing this behavior in this patch? It seems unrelated...

[esprehn, 2014/07/24 22:50:18]

I'm not changing any behavior, I moved childrenChanged into notifyNodeInserted
which means we'd call childrenChanged on ShadowRoot's since
ElementShadow::addShadowRoot calls notifyNodeInserted(), but calling
childrenChanged on a ShadowRoot right when it's created is not correct and would
change behavior.

+    // children changed when one is added.
+    // FIXME: We should have a separate code path for ShadowRoot since it only
+    // needs to call insertedInto and the rest of this logic is not needed.
+    if (!root.isShadowRoot()) {
+        ChildrenChange change = {ChildInserted, root.previousSibling(), root.nextSibling(), source};
+        childrenChanged(change);
+    }
+
     for (size_t i = 0; i < postInsertionNotificationTargets.size(); ++i) {
         Node* targetNode = postInsertionNotificationTargets[i].get();
         if (targetNode->inDocument())
@@ -1191,9 +1203,6 @@ void ContainerNode::updateTreeAfterInsertion(Node& child)
 
     ChildListMutationScope(*this).childAdded(child);
 
-    ChildrenChange change = {ChildInserted, child.previousSibling(), child.nextSibling(), ChildrenChangeSourceAPI};
-    childrenChanged(change);
-
     notifyNodeInserted(child);
 
     dispatchChildInsertionEvents(child);