Change the timing of event dispatching and <script> execution in Node::appendChild, insertBefore,...

third_party/WebKit/Source/core/dom/ContainerNode.cpp

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2001 Dirk Mueller (mueller@kde.org)
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2013 Apple Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
  * version 2 of the License, or (at your option) any later version.
@@ skipping 136 matching lines @@
         exceptionState.throwDOMException(HierarchyRequestError, "Nodes of type '" + newChild.nodeName() + "' may not be inserted inside nodes of type '" + nodeName() + "'.");
         return false;
     }
     return true;
 }
 
 template <typename Functor>
 void ContainerNode::insertNodeVector(const NodeVector& targets, Node* next, const Functor& mutator)
 {
     InspectorInstrumentation::willInsertDOMNode(this);
+    NodeVector postInsertionNotificationTargets;
+    {
+        EventDispatchForbiddenScope assertNoEventDispatch;
+        ScriptForbiddenScope forbidScript;
+        for (const auto& targetNode : targets) {
+            DCHECK(targetNode);
+            Node& child = *targetNode;
+            // TODO(tkent): mutator never returns false because scripts don't run in the loop.

[tkent, 2016/09/08 04:52:44]

Will remove unnecessary code in a follow-up CL.

+            if (!mutator(*this, child, next))
+                break;
+            ChildListMutationScope(*this).childAdded(child);
+            if (document().containsV1ShadowTree())
+                child.checkSlotChangeAfterInserted();
+            InspectorInstrumentation::didInsertDOMNode(&child);
+            notifyNodeInsertedInternal(child, postInsertionNotificationTargets);
+        }
+    }
+    for (const auto& targetNode : targets)
+        childrenChanged(ChildrenChange::forInsertion(*targetNode, ChildrenChangeSourceAPI));
+    for (const auto& descendant : postInsertionNotificationTargets) {
+        if (descendant->isConnected())
+            descendant->didNotifySubtreeInsertionsToDocument();
+    }
     for (const auto& targetNode : targets) {
-        DCHECK(targetNode);
-        {
-            EventDispatchForbiddenScope assertNoEventDispatch;
-            ScriptForbiddenScope forbidScript;
-
-            if (!mutator(*this, *targetNode, next))
-                break;
-        }
-        updateTreeAfterInsertion(*targetNode);
+        if (targetNode->parentNode() == this)

[hayato, 2016/09/08 06:18:55]

Should we check this condition?
It looks the original code is always calling dispatchChildInsertionEvents.

[tkent, 2016/09/08 06:31:08]

The DOM specification is unclear for this and we may dispatch unconditionally,
but this check is compatible with Firefox and Edge.

With the original code, JavaScript execution for an earlier child by <script> or
DOM mutation event can't remove a later child from the parent because the later
child isn't appended to the parent yet.

[hayato, 2016/09/08 06:48:11]

We chatted offline. 

We have decided to make this align to Firefox and Edge, though the spec is
unclear.

+            dispatchChildInsertionEvents(*targetNode);
     }
     dispatchSubtreeModifiedEvent();
 }
 
 class ContainerNode::AdoptAndInsertBefore {
 public:
     bool operator()(ContainerNode& container, Node& child, Node* next) const
     {
         DCHECK(next);
         // Due to arbitrary code running in response to a DOM mutation event
@@ skipping 986 matching lines @@
     }
 
     // Dispatch the DOMNodeRemovedFromDocument event to all descendants.
     if (c->isConnected() && document->hasListenerType(Document::DOMNODEREMOVEDFROMDOCUMENT_LISTENER)) {
         NodeChildRemovalTracker scope(child);
         for (; c; c = NodeTraversal::next(*c, &child))
             c->dispatchScopedEvent(MutationEvent::create(EventTypeNames::DOMNodeRemovedFromDocument, false));
     }
 }
 
-void ContainerNode::updateTreeAfterInsertion(Node& child)
-{
-    ChildListMutationScope(*this).childAdded(child);
-
-    notifyNodeInserted(child);
-
-    dispatchChildInsertionEvents(child);
-}
-
 bool ContainerNode::hasRestyleFlagInternal(DynamicRestyleFlags mask) const
 {
     return rareData()->hasRestyleFlag(mask);
 }
 
 bool ContainerNode::hasRestyleFlagsInternal() const
 {
     return rareData()->hasRestyleFlags();
 }
 
@@ skipping 181 matching lines @@
         return true;
 
     if (node->isElementNode() && toElement(node)->shadow())
         return true;
 
     return false;
 }
 #endif
 
 } // namespace blink