Make removeChild, parserRemoveChild and removeChildren more consistent.

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

Index: third_party/WebKit/Source/core/dom/ContainerNode.cpp
diff --git a/third_party/WebKit/Source/core/dom/ContainerNode.cpp b/third_party/WebKit/Source/core/dom/ContainerNode.cpp
index a2ce787cc8e9cc6fcfb7bfad0ca1587b978746c2..d144a58419750cbeeec3b5f912afe5d72813fa79 100644
--- a/third_party/WebKit/Source/core/dom/ContainerNode.cpp
+++ b/third_party/WebKit/Source/core/dom/ContainerNode.cpp
@@ -438,43 +438,6 @@ Node* ContainerNode::replaceChild(Node* newChild,
   return oldChild;
 }
 
-void ContainerNode::willRemoveChild(Node& child) {
-  DCHECK_EQ(child.parentNode(), this);
-  ChildListMutationScope(*this).willRemoveChild(child);
-  child.notifyMutationObserversNodeWillDetach();
-  dispatchChildRemovalEvents(child);
-  ChildFrameDisconnector(child).disconnect();
-  if (document() != child.document()) {
-    // |child| was moved another document by DOM mutation event handler.
-    return;
-  }
-
-  // |nodeWillBeRemoved()| must be run after |ChildFrameDisconnector|, because
-  // |ChildFrameDisconnector| can run script which may cause state that is to
-  // be invalidated by removing the node.
-  ScriptForbiddenScope scriptForbiddenScope;
-  EventDispatchForbiddenScope assertNoEventDispatch;
-  // e.g. mutation event listener can create a new range.
-  document().nodeWillBeRemoved(child);
-}
-
-void ContainerNode::willRemoveChildren() {
-  NodeVector children;
-  getChildNodes(*this, children);
-
-  ChildListMutationScope mutation(*this);
-  for (const auto& node : children) {
-    DCHECK(node);
-    Node& child = *node;
-    mutation.willRemoveChild(child);
-    child.notifyMutationObserversNodeWillDetach();
-    dispatchChildRemovalEvents(child);
-  }
-
-  ChildFrameDisconnector(*this).disconnect(
-      ChildFrameDisconnector::DescendantsOnly);
-}
-
 DEFINE_TRACE(ContainerNode) {
   visitor->trace(m_firstChild);
   visitor->trace(m_lastChild);
@@ -489,60 +452,100 @@ DEFINE_TRACE_WRAPPERS(ContainerNode) {
 
 Node* ContainerNode::removeChild(Node* oldChild,
                                  ExceptionState& exceptionState) {
-  // NotFoundError: Raised if oldChild is not a child of this node.
-  // FIXME: We should never really get PseudoElements in here, but editing will
-  // sometimes attempt to remove them still. We should fix that and enable this
-  // DCHECK.  DCHECK(!oldChild->isPseudoElement())
-  if (!oldChild || oldChild->parentNode() != this ||
-      oldChild->isPseudoElement()) {
+  if (!oldChild || oldChild->parentNode() != this) {
     exceptionState.throwDOMException(
         NotFoundError, "The node to be removed is not a child of this node.");
     return nullptr;
   }
 
-  Node* child = oldChild;
+  // TODO(esprehn): We should never really get PseudoElements in here, but
+  // editing will sometimes attempt to remove them still. We should fix that and
+  // turn this into an assert.
+  if (oldChild->isPseudoElement()) {
+    exceptionState.throwDOMException(NotFoundError,
+                                     "Pseudo elements cannot be removed.");
+    return nullptr;
+  }
+
+  // MutationObserver, no script here.
+  ChildListMutationScope(*this).willRemoveChild(*oldChild);
+  oldChild->notifyMutationObserversNodeWillDetach();
 
-  document().removeFocusedElementOfSubtree(child);
+  // Run script in 'DOMNodeRemoved' and 'DOMNodeRemovedFromDocument' event
+  // handlers.
+  dispatchChildRemovalEvents(*oldChild);
 
-  // Events fired when blurring currently focused node might have moved this
-  // child into a different parent.
-  if (child->parentNode() != this) {
+  if (oldChild->parentNode() != this) {
     exceptionState.throwDOMException(NotFoundError,
                                      "The node to be removed is no longer a "
                                      "child of this node. Perhaps it was moved "
-                                     "in a 'blur' event handler?");
+                                     "in response to a mutation?");
     return nullptr;
   }
 
-  willRemoveChild(*child);
+  // Run script in 'unload' events for <iframe>, <object>, <embed>, etc.
+  ChildFrameDisconnector(*oldChild).disconnect();
 
-  // Mutation events might have moved this child into a different parent.
-  if (child->parentNode() != this) {
+  // Forbid loading frames inside the steps below that run script since we'd
+  // never disconnect them.
+  SubframeLoadingDisabler subframeLoadingDisabler(*oldChild);
+
+  if (oldChild->parentNode() != this) {
     exceptionState.throwDOMException(NotFoundError,
                                      "The node to be removed is no longer a "
                                      "child of this node. Perhaps it was moved "
-                                     "in response to a mutation?");
+                                     "in an 'unload' event handler?");
+    return nullptr;
+  }
+
+  // Run script in blur events.
+  document().removeFocusedElementOfSubtree(oldChild);
+
+  if (oldChild->parentNode() != this) {
+    exceptionState.throwDOMException(NotFoundError,
+                                     "The node to be removed is no longer a "
+                                     "child of this node. Perhaps it was moved "
+                                     "in a 'blur' event handler?");
     return nullptr;
   }
 
   {
+    // Script may run inside the UpdateSuspendScope as a result of plugins.
     HTMLFrameOwnerElement::UpdateSuspendScope suspendWidgetHierarchyUpdates;
     DocumentOrderedMap::RemoveScope treeRemoveScope;
 
-    Node* prev = child->previousSibling();
-    Node* next = child->nextSibling();
-    removeBetween(prev, next, *child);
-    notifyNodeRemoved(*child);
-    childrenChanged(ChildrenChange::forRemoval(*child, prev, next,
+    Node* prev = oldChild->previousSibling();
+    Node* next = oldChild->nextSibling();
+
+    {
+      EventDispatchForbiddenScope forbidEvents;
+      ScriptForbiddenScope forbidScript;
+
+      if (document() == oldChild->document())
+        document().nodeWillBeRemoved(*oldChild);
+
+      // Actually remove the node from the tree and then call removedFrom on it
+      // and all descendants.
+      removeBetween(prev, next, *oldChild);
+      notifyNodeRemoved(*oldChild);
+    }
+
+    // Script could run inside childrenChanged, enable iframe loading again.

[dcheng, 2016/11/07 20:50:57]

Nit: it's not clear to me what "enable iframe loading again" here means, there's
no SubframeLoadDisabler in this scope.

[esprehn, 2016/11/07 22:42:33]

There is one on the stack, see above subframeLoadingDisabler on line 491, I
apparently forgot the release() here though. I'll fix that.

+    // TODO(esprehn): Can script run inside childrenChanged for Removal? It
+    // looks like it can only happen for Insertion.
+    childrenChanged(ChildrenChange::forRemoval(*oldChild, prev, next,
                                                ChildrenChangeSourceAPI));
   }
+
+  // Run script in DOMSubtreeModified event handlers.
   dispatchSubtreeModifiedEvent();
-  return child;
+  return oldChild;
 }
 
 void ContainerNode::removeBetween(Node* previousChild,
                                   Node* nextChild,
                                   Node& oldChild) {
+  ScriptForbiddenScope scriptForbiddenScope;
   EventDispatchForbiddenScope assertNoEventDispatch;
 
   DCHECK_EQ(oldChild.parentNode(), this);
@@ -568,30 +571,59 @@ void ContainerNode::removeBetween(Node* previousChild,
   document().adoptIfNeeded(oldChild);
 }
 
+// TODO(esprehn): This function skips MutationEvents but runs script in lots of
+// other ways, perhaps we should change to dispatching those too and just call
+// removeChild() inside the parser?
 void ContainerNode::parserRemoveChild(Node& oldChild) {
   DCHECK_EQ(oldChild.parentNode(), this);
   DCHECK(!oldChild.isDocumentFragment());
 
-  // This may cause arbitrary Javascript execution via onunload handlers.
-  if (oldChild.connectedSubframeCount())
-    ChildFrameDisconnector(oldChild).disconnect();
+  // MutationObserver, no script here.
+  ChildListMutationScope(*this).willRemoveChild(oldChild);
+  oldChild.notifyMutationObserversNodeWillDetach();
+
+  // Run script in 'unload' events for <iframe>, <object>, <embed>, etc.
+  ChildFrameDisconnector(oldChild).disconnect();
+
+  // Forbid loading frames inside the steps below that run script since we'd
+  // never disconnect them.
+  SubframeLoadingDisabler subframeLoadingDisabler(oldChild);

[dcheng, 2016/11/07 20:50:57]

I'd like to understand why this needs to be scoped at this level.

Lines 592-604 don't run script.
Scripting is forbidden from 607-617.

So it's not clear to me why this SubframeLoadingDisabler helps (since we need to
release it before the next point we can run script anyway).

[esprehn, 2016/11/07 22:42:32]

Anything that finds a ScriptForbiddenScope bypass could insert a frame which
would then not get detached because we're after the ChildFrameDisconnector step.
Also if something in C++, ex. in a user agent shadow root, inserted an iframe it
would also get missed and not detached. So we put a SubframeLoadingDisabler on
the stack to prevent those bugs and the resulting UXSS for defense in depth.

Once we call ChildFrameDisconnector().disconnect(), nothing can insert frames in
that child until after notifyNodeRemoved or bad stuff happens. This enforces
that.

 
   if (oldChild.parentNode() != this)
     return;
 
-  ChildListMutationScope(*this).willRemoveChild(oldChild);
-  oldChild.notifyMutationObserversNodeWillDetach();
+  // TODO(esprehn): Why don't we call removeFocusedElementOfSubtree? This means
+  // focus doesn't get updated when the parser moves children around.
+
+  {
+    // Script may run inside the UpdateSuspendScope as a result of plugins.
+    HTMLFrameOwnerElement::UpdateSuspendScope suspendWidgetHierarchyUpdates;
+    DocumentOrderedMap::RemoveScope treeRemoveScope;
 
-  HTMLFrameOwnerElement::UpdateSuspendScope suspendWidgetHierarchyUpdates;
-  DocumentOrderedMap::RemoveScope treeRemoveScope;
+    Node* prev = oldChild.previousSibling();
+    Node* next = oldChild.nextSibling();
 
-  Node* prev = oldChild.previousSibling();
-  Node* next = oldChild.nextSibling();
-  removeBetween(prev, next, oldChild);
+    {
+      EventDispatchForbiddenScope forbidEvents;
+      ScriptForbiddenScope forbidScript;
 
-  notifyNodeRemoved(oldChild);
-  childrenChanged(ChildrenChange::forRemoval(oldChild, prev, next,
-                                             ChildrenChangeSourceParser));
+      // TODO(esprehn): Why don't we call document().nodeWillBeRemoved(oldChild)
+      // here? This means the parser can corrupt ranges, selection, node
+      // iterators and more.
+
+      // Actually remove the node from the tree and then call removedFrom on it.
+      // No script can run here.
+      removeBetween(prev, next, oldChild);
+      notifyNodeRemoved(oldChild);
+    }
+
+    // Script could run inside childrenChanged, enable iframe loading again.
+    // TODO(esprehn): Can script run inside childrenChanged for Removal? It
+    // looks like it can only happen for Insertion.
+    subframeLoadingDisabler.release();
+    childrenChanged(ChildrenChange::forRemoval(oldChild, prev, next,
+                                               ChildrenChangeSourceParser));
+  }
 }
 
 // This differs from other remove functions because it forcibly removes all the
@@ -600,43 +632,73 @@ void ContainerNode::removeChildren(SubtreeModificationAction action) {
   if (!m_firstChild)
     return;
 
-  // Do any prep work needed before actually starting to detach
-  // and remove... e.g. stop loading frames, fire unload events.
-  willRemoveChildren();
+  // TODO(esprehn): We shouldn't allow removing PseudoElements here.
 
   {
-    // Removing focus can cause frames to load, either via events (focusout,
-    // blur) or widget updates (e.g., for <embed>).
-    SubframeLoadingDisabler disabler(*this);
+    NodeVector children;
+    getChildNodes(*this, children);
+
+    // This must be inside a block scope before the below steps so
+    // MutationRecords are visible in takeRecords() inside those event
+    // handlers.
+    ChildListMutationScope mutation(*this);
+    for (const auto& oldChild : children) {
+      // MutationObserver, no script here.
+      mutation.willRemoveChild(*oldChild);
+      oldChild->notifyMutationObserversNodeWillDetach();
+
+      // TODO(esprehn): removeChild() exposes the MutationRecords to the
+      // child removal event handlers, but this function doesn't because we
+      // batch the records in the ChildListMutationScope above. Should we
+      // make these consistent?
+
+      // Run script in 'DOMNodeRemoved' and 'DOMNodeRemovedFromDocument' event
+      // handlers.
+      dispatchChildRemovalEvents(*oldChild);
+    }
+  }
+
+  // Run script in 'unload' events for <iframe>, <object>, <embed>, etc.
+  ChildFrameDisconnector(*this).disconnect(
+      ChildFrameDisconnector::DescendantsOnly);
 
-    // Exclude this node when looking for removed focusedElement since only
-    // children will be removed.
-    // This must be later than willRemoveChildren, which might change focus
-    // state of a child.
-    document().removeFocusedElementOfSubtree(this, true);
+  // Forbid loading frames inside the steps below that run script since we'd
+  // never disconnect them.
+  SubframeLoadingDisabler subframeLoadingDisabler(*this);
 
-    // Removing a node from a selection can cause widget updates.
-    document().nodeChildrenWillBeRemoved(*this);
-  }
+  // Exclude this node when looking for removed focusedElement since only
+  // children will be removed.
+  document().removeFocusedElementOfSubtree(this, true);

[dcheng, 2016/11/07 20:50:57]

From reading this, perhaps we should just scope SubframeLoadingDisabler inside
removeFocusedElementOfSubtree()? The only time we're actually disabling subframe
loading is inside this statement: in the rest of this function, scripting is
either forbidden (from 679 - 690), or scripting and subframe loading are
enabled.

[esprehn, 2016/11/07 22:42:32]

That wouldn't have prevented the recent UXSS, since they found a way around the
ScriptForbiddenScope and ran script inside of notifyNodeRemoved.

Inserting an iframe in JS or C++ anywhere in here after ChildFrameDisconnector
but before we finish calling notifyNodeRemoved is bad.

[dcheng, 2016/11/08 01:51:12]

Sorry, one more question, because I'd really like to avoid the manual release().
I'm wondering if we can structure the remove methods like this:

  {
    ChildFrameDisconnector(...).disconnect();
    HTMLFrameOwnerElement::UpdateSuspendScope suspendWidgetHierarchyUpdates;
    {
      SubframeLoadingDisabler subframeLoadingDisabler(*this);
      document().removeFocusedElementOfSubtree(tree, this);

      DocumentOrderedMap::removeScope treeRemoveScope;
      {
        EventDispatchForbiddenScope assertNoEventDispatch;
        ScriptForbiddenScope forbidScript;

        while (Node* oldChild = m_firstChild) {
          removeBetween(0, ...);
          notifyNodeRemoved(*oldChild);
        }
      }
    }
    ChildrenChange change = {...}
    childrenChanged(change);
  }

Basically, hoist the UpdateSuspendScope a little higher, so that the scopers
don't have scopes that intersect in an unusual way. It means that hierarchy
updates in focus element changes would be deferred as well (unnecessarily), but
maybe that's OK?

 
   {
+    // Script may run inside the UpdateSuspendScope as a result of plugins.
     HTMLFrameOwnerElement::UpdateSuspendScope suspendWidgetHierarchyUpdates;
     DocumentOrderedMap::RemoveScope treeRemoveScope;
+
     {
       EventDispatchForbiddenScope assertNoEventDispatch;
       ScriptForbiddenScope forbidScript;
 
-      while (Node* child = m_firstChild) {
-        removeBetween(0, child->nextSibling(), *child);
-        notifyNodeRemoved(*child);
+      document().nodeChildrenWillBeRemoved(*this);
+
+      // Remove each node from the tree and then call removedFrom on it and all
+      // descendants.
+      while (Node* oldChild = m_firstChild) {
+        removeBetween(0, oldChild->nextSibling(), *oldChild);
+        notifyNodeRemoved(*oldChild);
       }
     }
 
+    // Script could run inside childrenChanged, enable iframe loading again.
+    // TODO(esprehn): Can script run inside childrenChanged for Removal? It
+    // looks like it can only happen for Insertion.
+    subframeLoadingDisabler.release();
     ChildrenChange change = {AllChildrenRemoved, nullptr, nullptr, nullptr,
                              ChildrenChangeSourceAPI};
     childrenChanged(change);
   }
 
+  // Run script in DOMSubtreeModified event handlers.
   if (action == DispatchSubtreeModifiedEvent)
     dispatchSubtreeModifiedEvent();
 }