Make removeChild, parserRemoveChild and removeChildren more consistent.

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
@@ skipping 420 matching lines @@
                                                       targets, exceptionState))
     return oldChild;
 
   if (next)
     insertNodeVector(targets, next, AdoptAndInsertBefore());
   else
     insertNodeVector(targets, nullptr, AdoptAndAppendChild());
   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);
   Node::trace(visitor);
 }
 
 DEFINE_TRACE_WRAPPERS(ContainerNode) {
   visitor->traceWrappersWithManualWriteBarrier(m_firstChild);
   visitor->traceWrappersWithManualWriteBarrier(m_lastChild);
   Node::traceWrappers(visitor);
 }
 
 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;
+  }
 
-  document().removeFocusedElementOfSubtree(child);
+  // MutationObserver, no script here.
+  ChildListMutationScope(*this).willRemoveChild(*oldChild);
+  oldChild->notifyMutationObserversNodeWillDetach();
 
-  // Events fired when blurring currently focused node might have moved this
-  // child into a different parent.
-  if (child->parentNode() != this) {
+  // Run script in 'DOMNodeRemoved' and 'DOMNodeRemovedFromDocument' event
+  // handlers.
+  dispatchChildRemovalEvents(*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?");
+    return nullptr;
+  }
+
+  // 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);
+
+  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 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;
   }
 
-  willRemoveChild(*child);
-
-  // Mutation events might have moved this child into a different parent.
-  if (child->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?");
-    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);
 
   AttachContext context;
   context.clearInvalidation = true;
   if (!oldChild.needsAttach())
     oldChild.detachLayoutTree(context);
 
   if (nextChild)
     nextChild->setPreviousSibling(previousChild);
   if (previousChild)
     previousChild->setNextSibling(nextChild);
   if (m_firstChild == &oldChild)
     setFirstChild(nextChild);
   if (m_lastChild == &oldChild)
     setLastChild(previousChild);
 
   oldChild.setPreviousSibling(nullptr);
   oldChild.setNextSibling(nullptr);
   oldChild.setParentOrShadowHostNode(nullptr);
 
   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.
 
-  HTMLFrameOwnerElement::UpdateSuspendScope suspendWidgetHierarchyUpdates;
-  DocumentOrderedMap::RemoveScope treeRemoveScope;
+  {
+    // Script may run inside the UpdateSuspendScope as a result of plugins.
+    HTMLFrameOwnerElement::UpdateSuspendScope suspendWidgetHierarchyUpdates;
+    DocumentOrderedMap::RemoveScope treeRemoveScope;
 
-  Node* prev = oldChild.previousSibling();
-  Node* next = oldChild.nextSibling();
-  removeBetween(prev, next, oldChild);
+    Node* prev = oldChild.previousSibling();
+    Node* next = oldChild.nextSibling();
 
-  notifyNodeRemoved(oldChild);
-  childrenChanged(ChildrenChange::forRemoval(oldChild, prev, next,
-                                             ChildrenChangeSourceParser));
+    {
+      EventDispatchForbiddenScope forbidEvents;
+      ScriptForbiddenScope forbidScript;
+
+      // 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
 // children, regardless of read-only status or event exceptions, e.g.
 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);
 
-    // 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);
+    // 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();
 
-    // Removing a node from a selection can cause widget updates.
-    document().nodeChildrenWillBeRemoved(*this);
+      // 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);
+
+  // Forbid loading frames inside the steps below that run script since we'd
+  // never disconnect them.
+  SubframeLoadingDisabler subframeLoadingDisabler(*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();
 }
 
 Node* ContainerNode::appendChild(Node* newChild,
                                  ExceptionState& exceptionState) {
   // Make sure adding the new child is ok
   if (!checkAcceptChild(newChild, 0, exceptionState))
     return newChild;
   DCHECK(newChild);
@@ skipping 818 matching lines @@
     return true;
 
   if (node->isElementNode() && toElement(node)->shadow())
     return true;
 
   return false;
 }
 #endif
 
 }  // namespace blink