Bug 1393806 part 3. Change dom::ReparentWrapper to take an ErrorResult. r=peterv
authorBoris Zbarsky <bzbarsky@mit.edu>
Wed, 13 Sep 2017 13:34:55 -0400
changeset 430297 1ca7ea3ba0fe32afd748eac1f73dfec54f4d80ae
parent 430296 25ba920b0c5ae53d58961601114d0c60b914d377
child 430298 867fe67b16a1227b2f4758ff69a705e43b39af10
push id7761
push userjlund@mozilla.com
push dateFri, 15 Sep 2017 00:19:52 +0000
treeherdermozilla-beta@c38455951db4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspeterv
bugs1393806
milestone57.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1393806 part 3. Change dom::ReparentWrapper to take an ErrorResult. r=peterv This makes it easier for its consumers to avoid leaving a dangling exception on the JSContext. MozReview-Commit-ID: Xep7IkYxSx
dom/base/crashtests/1393806.html
dom/base/crashtests/crashtests.list
dom/base/nsFrameLoader.cpp
dom/base/nsINode.cpp
dom/base/nsNodeUtils.cpp
dom/bindings/BindingUtils.cpp
dom/bindings/BindingUtils.h
dom/html/nsHTMLDocument.cpp
dom/xml/XMLDocument.cpp
new file mode 100644
--- /dev/null
+++ b/dom/base/crashtests/1393806.html
@@ -0,0 +1,17 @@
+<html>
+  <head>
+    <script>
+      function fun_0() {
+        document.implementation.createDocument('', '', null).adoptNode(o2);
+      }
+
+      o1 = document.createElement('map');
+      o2 = document.createElement('iframe');
+      document.documentElement.appendChild(o1);
+      document.documentElement.appendChild(o2);
+      o1.textContent = 'x';
+      document.addEventListener('DOMNodeRemoved', fun_0, false);
+      o1.innerText = 'x';
+    </script>
+  </head>
+</html>
--- a/dom/base/crashtests/crashtests.list
+++ b/dom/base/crashtests/crashtests.list
@@ -218,8 +218,9 @@ pref(dom.IntersectionObserver.enabled,tr
 load 1377826.html
 skip-if(stylo&&isDebugBuild&&winWidget) load structured_clone_container_throws.html # Bug 1383845
 HTTP(..) load xhr_abortinprogress.html
 load xhr_empty_datauri.html
 load xhr_html_nullresponse.html
 load 1383478.html
 load 1383780.html
 pref(clipboard.autocopy,true) load 1385272-1.html
+load 1393806.html
--- a/dom/base/nsFrameLoader.cpp
+++ b/dom/base/nsFrameLoader.cpp
@@ -2358,18 +2358,19 @@ nsFrameLoader::SetOwnerContent(Element* 
   mOwnerContent = aContent;
 
   AutoJSAPI jsapi;
   jsapi.Init();
 
   JS::RootedObject wrapper(jsapi.cx(), GetWrapper());
   if (wrapper) {
     JSAutoCompartment ac(jsapi.cx(), wrapper);
-    nsresult rv = ReparentWrapper(jsapi.cx(), wrapper);
-    Unused << NS_WARN_IF(NS_FAILED(rv));
+    IgnoredErrorResult rv;
+    ReparentWrapper(jsapi.cx(), wrapper, rv);
+    Unused << NS_WARN_IF(rv.Failed());
   }
 
   if (RenderFrameParent* rfp = GetCurrentRenderFrame()) {
     rfp->OwnerContentChanged(aContent);
   }
 }
 
 bool
--- a/dom/base/nsINode.cpp
+++ b/dom/base/nsINode.cpp
@@ -1508,91 +1508,99 @@ nsINode::Unlink(nsINode* tmp)
   }
 
   if (tmp->HasProperties()) {
     nsNodeUtils::UnlinkUserData(tmp);
     tmp->DeleteProperty(nsGkAtoms::keepobjectsalive);
   }
 }
 
-static nsresult
-AdoptNodeIntoOwnerDoc(nsINode *aParent, nsINode *aNode)
+static void
+AdoptNodeIntoOwnerDoc(nsINode *aParent, nsINode *aNode, ErrorResult& aError)
 {
   NS_ASSERTION(!aNode->GetParentNode(),
                "Should have removed from parent already");
 
   nsIDocument *doc = aParent->OwnerDoc();
 
-  ErrorResult rv;
-  nsINode* adoptedNode = doc->AdoptNode(*aNode, rv);
-  rv.WouldReportJSException();
-  if (NS_WARN_IF(rv.Failed())) {
-    return rv.StealNSResult();
-  }
-
-  NS_ASSERTION(aParent->OwnerDoc() == doc,
+  DebugOnly<nsINode*> adoptedNode = doc->AdoptNode(*aNode, aError);
+
+#ifdef DEBUG
+  if (!aError.Failed()) {
+    MOZ_ASSERT(aParent->OwnerDoc() == doc,
                "ownerDoc chainged while adopting");
-  NS_ASSERTION(adoptedNode == aNode, "Uh, adopt node changed nodes?");
-  NS_ASSERTION(aParent->OwnerDoc() == aNode->OwnerDoc(),
+    MOZ_ASSERT(adoptedNode == aNode, "Uh, adopt node changed nodes?");
+    MOZ_ASSERT(aParent->OwnerDoc() == aNode->OwnerDoc(),
                "ownerDocument changed again after adopting!");
-
-  return NS_OK;
+  }
+#endif // DEBUG
 }
 
-static nsresult
-CheckForOutdatedParent(nsINode* aParent, nsINode* aNode)
+static void
+CheckForOutdatedParent(nsINode* aParent, nsINode* aNode, ErrorResult& aError)
 {
   if (JSObject* existingObjUnrooted = aNode->GetWrapper()) {
     JS::Rooted<JSObject*> existingObj(RootingCx(), existingObjUnrooted);
 
     AutoJSContext cx;
     nsIGlobalObject* global = aParent->OwnerDoc()->GetScopeObject();
     MOZ_ASSERT(global);
 
     if (js::GetGlobalForObjectCrossCompartment(existingObj) !=
         global->GetGlobalJSObject()) {
       JSAutoCompartment ac(cx, existingObj);
-      nsresult rv = ReparentWrapper(cx, existingObj);
-      NS_ENSURE_SUCCESS(rv, rv);
+      ReparentWrapper(cx, existingObj, aError);
     }
   }
-
-  return NS_OK;
 }
 
 nsresult
 nsINode::doInsertChildAt(nsIContent* aKid, uint32_t aIndex,
                          bool aNotify, nsAttrAndChildArray& aChildArray)
 {
   MOZ_ASSERT(!aKid->GetParentNode(), "Inserting node that already has parent");
   MOZ_ASSERT(!IsNodeOfType(nsINode::eATTRIBUTE));
 
-  nsresult rv;
-
   // The id-handling code, and in the future possibly other code, need to
   // react to unexpected attribute changes.
   nsMutationGuard::DidMutate();
 
   // Do this before checking the child-count since this could cause mutations
   nsIDocument* doc = GetUncomposedDoc();
   mozAutoDocUpdate updateBatch(GetComposedDoc(), UPDATE_CONTENT_MODEL, aNotify);
 
   if (OwnerDoc() != aKid->OwnerDoc()) {
-    rv = AdoptNodeIntoOwnerDoc(this, aKid);
-    NS_ENSURE_SUCCESS(rv, rv);
+    ErrorResult error;
+    AdoptNodeIntoOwnerDoc(this, aKid, error);
+
+    // Need to WouldReportJSException() if our callee can throw a JS
+    // exception (which it can) and we're neither propagating the
+    // error out nor unconditionally suppressing it.
+    error.WouldReportJSException();
+    if (NS_WARN_IF(error.Failed())) {
+      return error.StealNSResult();
+    }
   } else if (OwnerDoc()->DidDocumentOpen()) {
-    rv = CheckForOutdatedParent(this, aKid);
-    NS_ENSURE_SUCCESS(rv, rv);
+    ErrorResult error;
+    CheckForOutdatedParent(this, aKid, error);
+
+    // Need to WouldReportJSException() if our callee can throw a JS
+    // exception (which it can) and we're neither propagating the
+    // error out nor unconditionally suppressing it.
+    error.WouldReportJSException();
+    if (NS_WARN_IF(error.Failed())) {
+      return error.StealNSResult();
+    }
   }
 
   uint32_t childCount = aChildArray.ChildCount();
   NS_ENSURE_TRUE(aIndex <= childCount, NS_ERROR_ILLEGAL_VALUE);
   bool isAppend = (aIndex == childCount);
 
-  rv = aChildArray.InsertChildAt(aKid, aIndex);
+  nsresult rv = aChildArray.InsertChildAt(aKid, aIndex);
   NS_ENSURE_SUCCESS(rv, rv);
   if (aIndex == 0) {
     mFirstChild = aKid;
   }
 
   // Invalidate cached array of child nodes
   nsSlots* slots = GetExistingSlots();
   if (slots && slots->mChildNodes) {
@@ -2427,22 +2435,22 @@ nsINode::ReplaceOrInsertBefore(bool aRep
 
   // Move new child over to our document if needed. Do this after removing
   // it from its parent so that AdoptNode doesn't fire DOMNodeRemoved
   // DocumentType nodes are the only nodes that can have a null
   // ownerDocument according to the DOM spec, and we need to allow
   // inserting them w/o calling AdoptNode().
   nsIDocument* doc = OwnerDoc();
   if (doc != newContent->OwnerDoc()) {
-    aError = AdoptNodeIntoOwnerDoc(this, aNewChild);
+    AdoptNodeIntoOwnerDoc(this, aNewChild, aError);
     if (aError.Failed()) {
       return nullptr;
     }
   } else if (doc->DidDocumentOpen()) {
-    aError = CheckForOutdatedParent(this, aNewChild);
+    CheckForOutdatedParent(this, aNewChild, aError);
     if (aError.Failed()) {
       return nullptr;
     }
   }
 
   /*
    * Check if we're inserting a document fragment. If we are, we need
    * to actually add its children individually (i.e. we don't add the
--- a/dom/base/nsNodeUtils.cpp
+++ b/dom/base/nsNodeUtils.cpp
@@ -582,26 +582,25 @@ nsNodeUtils::CloneAndAdopt(nsINode *aNod
     }
 
     if (aReparentScope) {
       AutoJSContext cx;
       JS::Rooted<JSObject*> wrapper(cx);
       if ((wrapper = aNode->GetWrapper())) {
         MOZ_ASSERT(IsDOMObject(wrapper));
         JSAutoCompartment ac(cx, wrapper);
-        nsresult rv = ReparentWrapper(cx, wrapper);
-        if (NS_FAILED(rv)) {
+        ReparentWrapper(cx, wrapper, aError);
+        if (aError.Failed()) {
           if (wasRegistered) {
             aNode->OwnerDoc()->UnregisterActivityObserver(aNode->AsElement());
           }
           aNode->mNodeInfo.swap(newNodeInfo);
           if (wasRegistered) {
             aNode->OwnerDoc()->RegisterActivityObserver(aNode->AsElement());
           }
-          aError.Throw(rv);
           return nullptr;
         }
       }
     }
   }
 
   if (aDeep && (!aClone || !aNode->IsNodeOfType(nsINode::eATTRIBUTE))) {
     // aNode's children.
--- a/dom/bindings/BindingUtils.cpp
+++ b/dom/bindings/BindingUtils.cpp
@@ -2174,27 +2174,30 @@ DictionaryBase::AppendJSONToString(const
                                    uint32_t aDataLength,
                                    void* aString)
 {
   nsAString* string = static_cast<nsAString*>(aString);
   string->Append(aJSONData, aDataLength);
   return true;
 }
 
-nsresult
-ReparentWrapper(JSContext* aCx, JS::Handle<JSObject*> aObjArg)
+void
+ReparentWrapper(JSContext* aCx, JS::Handle<JSObject*> aObjArg, ErrorResult& aError)
 {
   js::AssertSameCompartment(aCx, aObjArg);
 
+  aError.MightThrowJSException();
+
   // Check if we're anywhere near the stack limit before we reach the
   // transplanting code, since it has no good way to handle errors. This uses
   // the untrusted script limit, which is not strictly necessary since no
   // actual script should run.
   if (!js::CheckRecursionLimitConservative(aCx)) {
-    return NS_ERROR_FAILURE;
+    aError.StealExceptionFromJSContext(aCx);
+    return;
   }
 
   JS::Rooted<JSObject*> aObj(aCx, aObjArg);
   const DOMJSClass* domClass = GetDOMClass(aObj);
 
   // DOM things are always parented to globals.
   JS::Rooted<JSObject*> oldParent(aCx,
                                   js::GetGlobalForObjectCrossCompartment(aObj));
@@ -2205,71 +2208,76 @@ ReparentWrapper(JSContext* aCx, JS::Hand
   MOZ_ASSERT(JS_IsGlobalObject(newParent));
 
   JSAutoCompartment oldAc(aCx, oldParent);
 
   JSCompartment* oldCompartment = js::GetObjectCompartment(oldParent);
   JSCompartment* newCompartment = js::GetObjectCompartment(newParent);
   if (oldCompartment == newCompartment) {
     MOZ_ASSERT(oldParent == newParent);
-    return NS_OK;
+    return;
   }
 
   nsISupports* native = UnwrapDOMObjectToISupports(aObj);
   if (!native) {
-    return NS_OK;
+    return;
   }
 
   bool isProxy = js::IsProxy(aObj);
   JS::Rooted<JSObject*> expandoObject(aCx);
   if (isProxy) {
     expandoObject = DOMProxyHandler::GetAndClearExpandoObject(aObj);
   }
 
   JSAutoCompartment newAc(aCx, newParent);
 
   // First we clone the reflector. We get a copy of its properties and clone its
   // expando chain.
 
   JS::Handle<JSObject*> proto = (domClass->mGetProto)(aCx);
   if (!proto) {
-    return NS_ERROR_FAILURE;
+    aError.StealExceptionFromJSContext(aCx);
+    return;
   }
 
   JS::Rooted<JSObject*> newobj(aCx, JS_CloneObject(aCx, aObj, proto));
   if (!newobj) {
-    return NS_ERROR_FAILURE;
+    aError.StealExceptionFromJSContext(aCx);
+    return;
   }
 
   JS::Rooted<JSObject*> propertyHolder(aCx);
   JS::Rooted<JSObject*> copyFrom(aCx, isProxy ? expandoObject : aObj);
   if (copyFrom) {
     propertyHolder = JS_NewObjectWithGivenProto(aCx, nullptr, nullptr);
     if (!propertyHolder) {
-      return NS_ERROR_OUT_OF_MEMORY;
+      aError.StealExceptionFromJSContext(aCx);
+      return;
     }
 
     if (!JS_CopyPropertiesFrom(aCx, propertyHolder, copyFrom)) {
-      return NS_ERROR_FAILURE;
+      aError.StealExceptionFromJSContext(aCx);
+      return;
     }
   } else {
     propertyHolder = nullptr;
   }
 
   // Expandos from other compartments are attached to the target JS object.
   // Copy them over, and let the old ones die a natural death.
 
   // Note that at this point the DOM_OBJECT_SLOT for |newobj| has not been set.
   // CloneExpandoChain() will use this property of |newobj| when it calls
   // preserveWrapper() via attachExpandoObject() if |aObj| has expandos set, and
   // preserveWrapper() will not do anything in this case.  This is safe because
   // if expandos are present then the wrapper will already have been preserved
   // for this native.
   if (!xpc::XrayUtils::CloneExpandoChain(aCx, newobj, aObj)) {
-    return NS_ERROR_FAILURE;
+    aError.StealExceptionFromJSContext(aCx);
+    return;
   }
 
   // We've set up |newobj|, so we make it own the native by setting its reserved
   // slot and nulling out the reserved slot of |obj|.
   //
   // NB: It's important to do this _after_ copying the properties to
   // propertyHolder. Otherwise, an object with |foo.x === foo| will
   // crash when JS_CopyPropertiesFrom tries to call wrap() on foo.x.
@@ -2309,19 +2317,16 @@ ReparentWrapper(JSContext* aCx, JS::Hand
     rv = UNWRAP_OBJECT(HTMLEmbedElement, &maybeObjLC, htmlobject);
     if (NS_FAILED(rv)) {
       htmlobject = nullptr;
     }
   }
   if (htmlobject) {
     htmlobject->SetupProtoChain(aCx, aObj);
   }
-
-  // Now we can just return the wrapper
-  return NS_OK;
 }
 
 GlobalObject::GlobalObject(JSContext* aCx, JSObject* aObject)
   : mGlobalJSObject(aCx),
     mCx(aCx),
     mGlobalObject(nullptr)
 {
   MOZ_ASSERT(mCx);
--- a/dom/bindings/BindingUtils.h
+++ b/dom/bindings/BindingUtils.h
@@ -2692,18 +2692,18 @@ void NonNullHelper(binding_detail::FakeS
 MOZ_ALWAYS_INLINE
 const nsAString& NonNullHelper(const binding_detail::FakeString& aArg)
 {
   return aArg;
 }
 
 // Reparent the wrapper of aObj to whatever its native now thinks its
 // parent should be.
-nsresult
-ReparentWrapper(JSContext* aCx, JS::Handle<JSObject*> aObj);
+void
+ReparentWrapper(JSContext* aCx, JS::Handle<JSObject*> aObj, ErrorResult& aError);
 
 /**
  * Used to implement the Symbol.hasInstance property of an interface object.
  */
 bool
 InterfaceHasInstance(JSContext* cx, unsigned argc, JS::Value* vp);
 
 bool
--- a/dom/html/nsHTMLDocument.cpp
+++ b/dom/html/nsHTMLDocument.cpp
@@ -1719,28 +1719,28 @@ nsHTMLDocument::Open(JSContext* cx,
     // that we've had stuff done to us.  From now on, if anyone tries to
     // document.open() us, they get a new inner window.
     SetIsInitialDocument(false);
 
     nsCOMPtr<nsIScriptGlobalObject> newScope(do_QueryReferent(mScopeObject));
     JS::Rooted<JSObject*> wrapper(cx, GetWrapper());
     if (oldScope && newScope != oldScope && wrapper) {
       JSAutoCompartment ac(cx, wrapper);
-      rv = mozilla::dom::ReparentWrapper(cx, wrapper);
+      mozilla::dom::ReparentWrapper(cx, wrapper, rv);
       if (rv.Failed()) {
         return nullptr;
       }
 
       // Also reparent the template contents owner document
       // because its global is set to the same as this document.
       if (mTemplateContentsOwner) {
         JS::Rooted<JSObject*> contentsOwnerWrapper(cx,
           mTemplateContentsOwner->GetWrapper());
         if (contentsOwnerWrapper) {
-          rv = mozilla::dom::ReparentWrapper(cx, contentsOwnerWrapper);
+          mozilla::dom::ReparentWrapper(cx, contentsOwnerWrapper, rv);
           if (rv.Failed()) {
             return nullptr;
           }
         }
       }
     }
   }
 
--- a/dom/xml/XMLDocument.cpp
+++ b/dom/xml/XMLDocument.cpp
@@ -159,16 +159,20 @@ NS_NewDOMDocument(nsIDOMDocument** aInst
   // XMLDocuments and documents "created in memory" get to be UTF-8 by default,
   // unlike the legacy HTML mess
   doc->SetDocumentCharacterSet(UTF_8_ENCODING);
 
   if (aDoctype) {
     nsCOMPtr<nsINode> doctypeAsNode = do_QueryInterface(aDoctype);
     ErrorResult result;
     d->AppendChild(*doctypeAsNode, result);
+    // Need to WouldReportJSException() if our callee can throw a JS
+    // exception (which it can) and we're neither propagating the
+    // error out nor unconditionally suppressing it.
+    result.WouldReportJSException();
     if (NS_WARN_IF(result.Failed())) {
       return result.StealNSResult();
     }
   }
 
   if (!aQualifiedName.IsEmpty()) {
     ErrorResult result;
     ElementCreationOptionsOrString options;
@@ -176,16 +180,20 @@ NS_NewDOMDocument(nsIDOMDocument** aInst
 
     nsCOMPtr<Element> root =
       doc->CreateElementNS(aNamespaceURI, aQualifiedName, options, result);
     if (NS_WARN_IF(result.Failed())) {
       return result.StealNSResult();
     }
 
     d->AppendChild(*root, result);
+    // Need to WouldReportJSException() if our callee can throw a JS
+    // exception (which it can) and we're neither propagating the
+    // error out nor unconditionally suppressing it.
+    result.WouldReportJSException();
     if (NS_WARN_IF(result.Failed())) {
       return result.StealNSResult();
     }
   }
 
   *aInstancePtrResult = doc;
   NS_ADDREF(*aInstancePtrResult);