Issue #2135 - Bug 1393806/Part 1: Change nsNodeUtils cloning/adopting stuff to use an ErrorResult for errors

This will allow us to propagate out more informative errors in some cases.

dom/base/nsDocument.cpp

@@ -5823,14 +5823,9 @@ nsIDocument::ImportNode(nsINode& aNode, bool aDeep, ErrorResult& rv) const
     case nsIDOMNode::COMMENT_NODE:
     case nsIDOMNode::DOCUMENT_TYPE_NODE:
     {
-      nsCOMPtr<nsINode> newNode;
       nsCOMArray<nsINode> nodesWithProperties;
-      rv = nsNodeUtils::Clone(imported, aDeep, mNodeInfoManager,
+      return nsNodeUtils::Clone(imported, aDeep, mNodeInfoManager,
-                              nodesWithProperties, getter_AddRefs(newNode));
+                                &nodesWithProperties, rv);
-      if (rv.Failed()) {
-        return nullptr;
-      }
-      return newNode.forget();
     }
     default:
     {
@@ -6996,8 +6991,8 @@ nsIDocument::AdoptNode(nsINode& aAdoptedNode, ErrorResult& rv)
   }
 
   nsCOMArray<nsINode> nodesWithProperties;
-  rv = nsNodeUtils::Adopt(adoptedNode, sameDocument ? nullptr : mNodeInfoManager,
+  nsNodeUtils::Adopt(adoptedNode, sameDocument ? nullptr : mNodeInfoManager,
-                          newScope, nodesWithProperties);
+                     newScope, nodesWithProperties, rv);
   if (rv.Failed()) {
     // Disconnect all nodes from their parents, since some have the old document
     // as their ownerDocument and some have this as their ownerDocument.

dom/base/nsINode.cpp

@@ -3084,9 +3084,7 @@ nsINode::WrapObject(JSContext *aCx, JS::Handle<JSObject*> aGivenProto)
 already_AddRefed<nsINode>
 nsINode::CloneNode(bool aDeep, ErrorResult& aError)
 {
-  nsCOMPtr<nsINode> result;
+  return nsNodeUtils::CloneNodeImpl(this, aDeep, aError);
-  aError = nsNodeUtils::CloneNodeImpl(this, aDeep, getter_AddRefs(result));
-  return result.forget();
 }
 
 nsDOMAttributeMap*

dom/base/nsNodeUtils.cpp

@@ -404,28 +404,20 @@ nsNodeUtils::TraverseUserData(nsINode* aNode,
 }
 
 /* static */
-nsresult
+already_AddRefed<nsINode>
-nsNodeUtils::CloneNodeImpl(nsINode *aNode, bool aDeep, nsINode **aResult)
+nsNodeUtils::CloneNodeImpl(nsINode *aNode, bool aDeep, ErrorResult& aError)
 {
-  *aResult = nullptr;
-
-  nsCOMPtr<nsINode> newNode;
   nsCOMArray<nsINode> nodesWithProperties;
-  nsresult rv = Clone(aNode, aDeep, nullptr, nodesWithProperties,
+  return Clone(aNode, aDeep, nullptr, &nodesWithProperties, aError);
-                      getter_AddRefs(newNode));
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  newNode.forget(aResult);
-  return NS_OK;
 }
 
 /* static */
-nsresult
+already_AddRefed<nsINode>
 nsNodeUtils::CloneAndAdopt(nsINode *aNode, bool aClone, bool aDeep,
                            nsNodeInfoManager *aNewNodeInfoManager,
                            JS::Handle<JSObject*> aReparentScope,
-                           nsCOMArray<nsINode> &aNodesWithProperties,
+                           nsCOMArray<nsINode> *aNodesWithProperties,
-                           nsINode *aParent, nsINode **aResult)
+                           nsINode *aParent, ErrorResult& aError)
 {
   NS_PRECONDITION((!aClone && aNewNodeInfoManager) || !aReparentScope,
                   "If cloning or not getting a new nodeinfo we shouldn't "
@@ -433,14 +425,11 @@ nsNodeUtils::CloneAndAdopt(nsINode *aNode, bool aClone, bool aDeep,
   NS_PRECONDITION(!aParent || aNode->IsNodeOfType(nsINode::eCONTENT),
                   "Can't insert document or attribute nodes into a parent");
 
-  *aResult = nullptr;
-
   // First deal with aNode and walk its attributes (and their children). Then,
   // if aDeep is true, deal with aNode's children (and recurse into their
   // attributes and children).
 
   nsAutoScriptBlocker scriptBlocker;
-  nsresult rv;
 
   nsNodeInfoManager *nodeInfoManager = aNewNodeInfoManager;
 
@@ -452,14 +441,20 @@ nsNodeUtils::CloneAndAdopt(nsINode *aNode, bool aClone, bool aDeep,
     // Don't allow importing/adopting nodes from non-privileged "scriptable"
     // documents to "non-scriptable" documents.
     nsIDocument* newDoc = nodeInfoManager->GetDocument();
-    NS_ENSURE_STATE(newDoc);
+    if (NS_WARN_IF(!newDoc)) {
+      aError.Throw(NS_ERROR_UNEXPECTED);
+      return nullptr;
+    }
     bool hasHadScriptHandlingObject = false;
     if (!newDoc->GetScriptHandlingObject(hasHadScriptHandlingObject) &&
         !hasHadScriptHandlingObject) {
       nsIDocument* currentDoc = aNode->OwnerDoc();
-      NS_ENSURE_STATE((nsContentUtils::IsChromeDoc(currentDoc) ||
+      if (NS_WARN_IF(!nsContentUtils::IsChromeDoc(currentDoc) &&
-                       (!currentDoc->GetScriptHandlingObject(hasHadScriptHandlingObject) &&
+                     (currentDoc->GetScriptHandlingObject(hasHadScriptHandlingObject) ||
-                        !hasHadScriptHandlingObject)));
+                      hasHadScriptHandlingObject))) {
+        aError.Throw(NS_ERROR_UNEXPECTED);
+        return nullptr;
+      }
     }
 
     newNodeInfo = nodeInfoManager->GetNodeInfo(nodeInfo->NameAtom(),
@@ -475,8 +470,11 @@ nsNodeUtils::CloneAndAdopt(nsINode *aNode, bool aClone, bool aDeep,
 
   nsCOMPtr<nsINode> clone;
   if (aClone) {
-    rv = aNode->Clone(nodeInfo, getter_AddRefs(clone));
+    nsresult rv = aNode->Clone(nodeInfo, getter_AddRefs(clone));
-    NS_ENSURE_SUCCESS(rv, rv);
+    if (NS_WARN_IF(NS_FAILED(rv))) {
+      aError.Throw(rv);
+      return nullptr;
+    }
 
     if (CustomElementRegistry::IsCustomElementEnabled() &&
         clone->IsHTMLElement()) {
@@ -514,7 +512,10 @@ nsNodeUtils::CloneAndAdopt(nsINode *aNode, bool aClone, bool aDeep,
       // parent.
       rv = aParent->AppendChildTo(static_cast<nsIContent*>(clone.get()),
                                   false);
-      NS_ENSURE_SUCCESS(rv, rv);
+      if (NS_WARN_IF(NS_FAILED(rv))) {
+        aError.Throw(rv);
+        return nullptr;
+      }
     }
     else if (aDeep && clone->IsNodeOfType(nsINode::eDOCUMENT)) {
       // After cloning the document itself, we want to clone the children into
@@ -613,7 +614,7 @@ nsNodeUtils::CloneAndAdopt(nsINode *aNode, bool aClone, bool aDeep,
       if ((wrapper = aNode->GetWrapper())) {
         MOZ_ASSERT(IsDOMObject(wrapper));
         JSAutoCompartment ac(cx, wrapper);
-        rv = ReparentWrapper(cx, wrapper);
+        nsresult rv = ReparentWrapper(cx, wrapper);
         if (NS_FAILED(rv)) {
           if (wasRegistered) {
             aNode->OwnerDoc()->UnregisterActivityObserver(aNode->AsElement());
@@ -622,17 +623,18 @@ nsNodeUtils::CloneAndAdopt(nsINode *aNode, bool aClone, bool aDeep,
           if (wasRegistered) {
             aNode->OwnerDoc()->RegisterActivityObserver(aNode->AsElement());
           }
-          return rv;
+          aError.Throw(rv);
+          return nullptr;
         }
       }
     }
   }
 
   if (aNode->HasProperties()) {
-    bool ok = aNodesWithProperties.AppendObject(aNode);
+    bool ok = aNodesWithProperties->AppendObject(aNode);
     MOZ_RELEASE_ASSERT(ok, "Out of memory");
     if (aClone) {
-      ok = aNodesWithProperties.AppendObject(clone);
+      ok = aNodesWithProperties->AppendObject(clone);
       MOZ_RELEASE_ASSERT(ok, "Out of memory");
     }
   }
@@ -642,22 +644,24 @@ nsNodeUtils::CloneAndAdopt(nsINode *aNode, bool aClone, bool aDeep,
     for (nsIContent* cloneChild = aNode->GetFirstChild();
          cloneChild;
          cloneChild = cloneChild->GetNextSibling()) {
-      nsCOMPtr<nsINode> child;
+      nsCOMPtr<nsINode> child =
-      rv = CloneAndAdopt(cloneChild, aClone, true, nodeInfoManager,
+        CloneAndAdopt(cloneChild, aClone, true, nodeInfoManager,
                       aReparentScope, aNodesWithProperties, clone,
-                         getter_AddRefs(child));
+                      aError);
-      NS_ENSURE_SUCCESS(rv, rv);
+      if (NS_WARN_IF(aError.Failed())) {
+        return nullptr;
+      }
     }
   }
 
-  // TODO: update this if we choose to land bug 1393806.
   if (aDeep && !aClone && aNode->IsElement()) {
     if (ShadowRoot* shadowRoot = aNode->AsElement()->GetShadowRoot()) {
-      nsCOMPtr<nsINode> child;
+      nsCOMPtr<nsINode> child = CloneAndAdopt(shadowRoot, aClone, aDeep, nodeInfoManager,
-      rv = CloneAndAdopt(shadowRoot, aClone, aDeep, nodeInfoManager,
                                               aReparentScope, aNodesWithProperties, clone,
-                         getter_AddRefs(child));
+                                              aError);
-      NS_ENSURE_SUCCESS(rv, rv);
+      if (NS_WARN_IF(aError.Failed())) {
+        return nullptr;
+      }
     }
   }
 
@@ -676,11 +680,13 @@ nsNodeUtils::CloneAndAdopt(nsINode *aNode, bool aClone, bool aDeep,
     for (nsIContent* cloneChild = origContent->GetFirstChild();
          cloneChild;
          cloneChild = cloneChild->GetNextSibling()) {
-      nsCOMPtr<nsINode> child;
+      nsCOMPtr<nsINode> child =
-      rv = CloneAndAdopt(cloneChild, aClone, aDeep, ownerNodeInfoManager,
+        CloneAndAdopt(cloneChild, aClone, aDeep, ownerNodeInfoManager,
                       aReparentScope, aNodesWithProperties, cloneContent,
-                         getter_AddRefs(child));
+                      aError);
-      NS_ENSURE_SUCCESS(rv, rv);
+      if (NS_WARN_IF(aError.Failed())) {
+        return nullptr;
+      }
     }
   }
 
@@ -703,9 +709,7 @@ nsNodeUtils::CloneAndAdopt(nsINode *aNode, bool aClone, bool aDeep,
   }
 #endif
 
-  clone.forget(aResult);
+  return clone.forget();
-
-  return NS_OK;
 }
 
 

dom/base/nsNodeUtils.h

@@ -17,6 +17,7 @@ template<class E> class nsCOMArray;
 class nsCycleCollectionTraversalCallback;
 namespace mozilla {
 struct NonOwningAnimationTarget;
+class ErrorResult;
 namespace dom {
 class Animation;
 } // namespace dom
@@ -181,25 +182,17 @@ public:
    * @param aNodesWithProperties All nodes (from amongst aNode and its
    *                             descendants) with properties. Every node will
    *                             be followed by its clone.
-   * @param aResult *aResult will contain the cloned node.
+   * @param aError The error, if any.
+   *
+   * @return The newly created node.  Null in error conditions.
    */
-  static nsresult Clone(nsINode *aNode, bool aDeep,
+  static already_AddRefed<nsINode> Clone(nsINode *aNode, bool aDeep,
                                          nsNodeInfoManager *aNewNodeInfoManager,
-                        nsCOMArray<nsINode> &aNodesWithProperties,
+                                         nsCOMArray<nsINode> *aNodesWithProperties,
-                        nsINode **aResult)
+                                         mozilla::ErrorResult& aError)
   {
     return CloneAndAdopt(aNode, true, aDeep, aNewNodeInfoManager,
-                         nullptr, aNodesWithProperties, nullptr, aResult);
+                         nullptr, aNodesWithProperties, nullptr, aError);
-  }
-
-  /**
-   * Clones aNode, its attributes and, if aDeep is true, its descendant nodes
-   */
-  static nsresult Clone(nsINode *aNode, bool aDeep, nsINode **aResult)
-  {
-    nsCOMArray<nsINode> dummyNodeWithProperties;
-    return CloneAndAdopt(aNode, true, aDeep, nullptr, nullptr,
-                         dummyNodeWithProperties, aNode->GetParent(), aResult);
   }
 
   /**
@@ -218,19 +211,20 @@ public:
    *                       should be done.
    * @param aNodesWithProperties All nodes (from amongst aNode and its
    *                             descendants) with properties.
+   * @param aError The error, if any.
    */
-  static nsresult Adopt(nsINode *aNode, nsNodeInfoManager *aNewNodeInfoManager,
+  static void Adopt(nsINode *aNode, nsNodeInfoManager *aNewNodeInfoManager,
                     JS::Handle<JSObject*> aReparentScope,
-                        nsCOMArray<nsINode> &aNodesWithProperties)
+                    nsCOMArray<nsINode>& aNodesWithProperties,
+                    mozilla::ErrorResult& aError)
   {
-    nsCOMPtr<nsINode> node;
+    // Just need to store the return value of CloneAndAdopt in a
-    nsresult rv = CloneAndAdopt(aNode, false, true, aNewNodeInfoManager,
+    // temporary nsCOMPtr to make sure we release it.
-                                aReparentScope, aNodesWithProperties,
+    nsCOMPtr<nsINode> node = CloneAndAdopt(aNode, false, true, aNewNodeInfoManager,
-                                nullptr, getter_AddRefs(node));
+                                           aReparentScope, &aNodesWithProperties,
+                                           nullptr, aError);
 
     nsMutationGuard::DidMutate();
-
-    return rv;
   }
 
   /**
@@ -248,9 +242,12 @@ public:
    *
    * @param aNode the node to clone
    * @param aDeep if true all descendants will be cloned too
-   * @param aResult the clone
+   * @param aError the error, if any.
+   *
+   * @return the clone, or null if an error occurs.
    */
-  static nsresult CloneNodeImpl(nsINode *aNode, bool aDeep, nsINode **aResult);
+  static already_AddRefed<nsINode> CloneNodeImpl(nsINode *aNode, bool aDeep,
+                                                 mozilla::ErrorResult& aError);
 
   /**
    * Release the UserData for aNode.
@@ -286,7 +283,7 @@ private:
    *
    * @param aNode Node to adopt/clone.
    * @param aClone If true the node will be cloned and the cloned node will
-   *               be in aResult.
+   *               be returned.
    * @param aDeep If true the function will be called recursively on
    *              descendants of the node
    * @param aNewNodeInfoManager The nodeinfo manager to use to create new
@@ -302,14 +299,18 @@ private:
    * @param aParent If aClone is true the cloned node will be appended to
    *                aParent's children. May be null. If not null then aNode
    *                must be an nsIContent.
-   * @param aResult If aClone is true then *aResult will contain the cloned
+   * @param aError The error, if any.
-   *                node.
+   *
+   * @return If aClone is true then the cloned node will be returned,
+   *          unless an error occurred.  In error conditions, null
+   *          will be returned.
    */
-  static nsresult CloneAndAdopt(nsINode *aNode, bool aClone, bool aDeep,
+  static already_AddRefed<nsINode>
-                                nsNodeInfoManager *aNewNodeInfoManager,
+    CloneAndAdopt(nsINode *aNode, bool aClone, bool aDeep,
+                  nsNodeInfoManager* aNewNodeInfoManager,
                   JS::Handle<JSObject*> aReparentScope,
-                                nsCOMArray<nsINode> &aNodesWithProperties,
+                  nsCOMArray<nsINode> *aNodesWithProperties,
-                                nsINode *aParent, nsINode **aResult);
+                  nsINode *aParent, mozilla::ErrorResult& aError);
 
   enum class AnimationMutationType
   {

dom/svg/SVGUseElement.cpp

@@ -259,14 +259,12 @@ SVGUseElement::CreateAnonymousContent()
     }
   }
 
-  nsCOMPtr<nsINode> newnode;
-  nsCOMArray<nsINode> unused;
   nsNodeInfoManager* nodeInfoManager =
     targetContent->OwnerDoc() == OwnerDoc() ?
       nullptr : OwnerDoc()->NodeInfoManager();
-  nsNodeUtils::Clone(targetContent, true, nodeInfoManager, unused,
+  IgnoredErrorResult rv;
-                     getter_AddRefs(newnode));
+  nsCOMPtr<nsINode> newnode =
-
+    nsNodeUtils::Clone(targetContent, true, nodeInfoManager, nullptr, rv);
   nsCOMPtr<nsIContent> newcontent = do_QueryInterface(newnode);
 
   if (!newcontent)

dom/xbl/nsXBLBinding.cpp

@@ -321,10 +321,11 @@ nsXBLBinding::GenerateAnonymousContent()
   if (hasContent) {
     nsIDocument* doc = mBoundElement->OwnerDoc();
 
-    nsCOMPtr<nsINode> clonedNode;
+    IgnoredErrorResult rv;
-    nsCOMArray<nsINode> nodesWithProperties;
+    nsCOMPtr<nsINode> clonedNode =
-    nsNodeUtils::Clone(content, true, doc->NodeInfoManager(),
+      nsNodeUtils::Clone(content, true, doc->NodeInfoManager(), nullptr, rv);
-                       nodesWithProperties, getter_AddRefs(clonedNode));
+    // FIXME: Bug 1399558, Why is this code OK assuming that nsNodeUtils::Clone
+    // never fails?
     mContent = clonedNode->AsElement();
 
     // Search for <xbl:children> elements in the XBL content. In the presence