From 9f0fb9c16e20d8d6ff192a89cecf7971b05d6996 Mon Sep 17 00:00:00 2001 From: FranklinDM Date: Sat, 4 Mar 2023 17:41:08 +0800 Subject: [PATCH] Issue #2135 - Bug 1393806/Part 3: Change dom::ReparentWrapper to take an ErrorResult * Bug 1393806 (Part 4) landed as part of Issue #1118. * Account for ReparentWrappersInSubtree, introduced in 1466991 --- dom/base/crashtests/1393806.html | 17 ++++++++++ dom/base/crashtests/crashtests.list | 1 + dom/base/nsINode.cpp | 51 ++++++++++++++++------------- dom/base/nsNodeUtils.cpp | 5 ++- dom/bindings/BindingUtils.cpp | 31 ++++++++++-------- dom/bindings/BindingUtils.h | 4 +-- dom/xml/XMLDocument.cpp | 4 +++ 7 files changed, 72 insertions(+), 41 deletions(-) create mode 100644 dom/base/crashtests/1393806.html diff --git a/dom/base/crashtests/1393806.html b/dom/base/crashtests/1393806.html new file mode 100644 index 0000000000..4e859bf602 --- /dev/null +++ b/dom/base/crashtests/1393806.html @@ -0,0 +1,17 @@ + + + + + diff --git a/dom/base/crashtests/crashtests.list b/dom/base/crashtests/crashtests.list index 40d358b38a..ea64f4762b 100644 --- a/dom/base/crashtests/crashtests.list +++ b/dom/base/crashtests/crashtests.list @@ -209,6 +209,7 @@ load 1230422.html load 1251361.html load 1304437.html pref(clipboard.autocopy,true) load 1385272-1.html +load 1393806.html pref(dom.webcomponents.enabled,true) load 1341693.html pref(dom.webcomponents.enabled,true) load 1419799.html pref(dom.webcomponents.enabled,false) load 1422931.html diff --git a/dom/base/nsINode.cpp b/dom/base/nsINode.cpp index c7297698c2..735dfb16e5 100644 --- a/dom/base/nsINode.cpp +++ b/dom/base/nsINode.cpp @@ -1555,28 +1555,25 @@ nsINode::SetExplicitBaseURI(nsIURI* aURI) return rv; } -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(); - } + DebugOnly adoptedNode = doc->AdoptNode(*aNode, aError); - NS_ASSERTION(aParent->OwnerDoc() == doc, - "ownerDoc chainged while adopting"); - NS_ASSERTION(adoptedNode == aNode, "Uh, adopt node changed nodes?"); - NS_ASSERTION(aParent->OwnerDoc() == aNode->OwnerDoc(), +#ifdef DEBUG + if (!aError.Failed()) { + MOZ_ASSERT(aParent->OwnerDoc() == doc, + "ownerDoc changed while adopting"); + 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 @@ -1601,19 +1598,20 @@ ReparentWrappersInSubtree(nsIContent* aRoot) rootedGlobal = xpc::GetXBLScope(cx, rootedGlobal); - nsresult rv; + ErrorResult rv; JS::Rooted reflector(cx); for (nsIContent* cur = aRoot; cur; cur = cur->GetNextNode(aRoot)) { if ((reflector = cur->GetWrapper())) { JSAutoCompartment ac(cx, reflector); - rv = ReparentWrapper(cx, reflector); - if NS_FAILED(rv) { + ReparentWrapper(cx, reflector, rv); + rv.WouldReportJSException(); + if (rv.Failed()) { // We _could_ consider BlastSubtreeToPieces here, but it's not really // needed. Having some nodes in here accessible to content while others // are not is probably OK. We just need to fail out of the actual // insertion, so they're not in the DOM. Returning a failure here will // do that. - return rv; + return rv.StealNSResult(); } } } @@ -1627,7 +1625,6 @@ nsINode::doInsertChildAt(nsIContent* aKid, uint32_t aIndex, { NS_PRECONDITION(!aKid->GetParentNode(), "Inserting node that already has parent"); - nsresult rv; // The id-handling code, and in the future possibly other code, need to // react to unexpected attribute changes. @@ -1638,15 +1635,23 @@ nsINode::doInsertChildAt(nsIContent* aKid, uint32_t aIndex, 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(); + } } 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; @@ -2483,7 +2488,7 @@ nsINode::ReplaceOrInsertBefore(bool aReplace, nsINode* aNewChild, // 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; } diff --git a/dom/base/nsNodeUtils.cpp b/dom/base/nsNodeUtils.cpp index 3eefc544f8..f1be693d2e 100644 --- a/dom/base/nsNodeUtils.cpp +++ b/dom/base/nsNodeUtils.cpp @@ -614,8 +614,8 @@ nsNodeUtils::CloneAndAdopt(nsINode *aNode, bool aClone, bool aDeep, 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()); } @@ -623,7 +623,6 @@ nsNodeUtils::CloneAndAdopt(nsINode *aNode, bool aClone, bool aDeep, if (wasRegistered) { aNode->OwnerDoc()->RegisterActivityObserver(aNode->AsElement()); } - aError.Throw(rv); return nullptr; } } diff --git a/dom/bindings/BindingUtils.cpp b/dom/bindings/BindingUtils.cpp index 879db27a49..080705174e 100644 --- a/dom/bindings/BindingUtils.cpp +++ b/dom/bindings/BindingUtils.cpp @@ -2106,16 +2106,19 @@ DictionaryBase::AppendJSONToString(const char16_t* aJSONData, return true; } -nsresult -ReparentWrapper(JSContext* aCx, JS::Handle aObjArg) +void +ReparentWrapper(JSContext* aCx, JS::Handle 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. - JS_CHECK_RECURSION_CONSERVATIVE(aCx, return NS_ERROR_FAILURE); + // TODO: Make sure to retain 'onerror' if bug 1342439 lands. + JS_CHECK_RECURSION_CONSERVATIVE(aCx, aError.StealExceptionFromJSContext(aCx); return); JS::Rooted aObj(aCx, aObjArg); const DOMJSClass* domClass = GetDOMClass(aObj); @@ -2135,12 +2138,12 @@ ReparentWrapper(JSContext* aCx, JS::Handle aObjArg) 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); @@ -2156,12 +2159,14 @@ ReparentWrapper(JSContext* aCx, JS::Handle aObjArg) JS::Handle proto = (domClass->mGetProto)(aCx); if (!proto) { - return NS_ERROR_FAILURE; + aError.StealExceptionFromJSContext(aCx); + return; } JS::Rooted newobj(aCx, JS_CloneObject(aCx, aObj, proto)); if (!newobj) { - return NS_ERROR_FAILURE; + aError.StealExceptionFromJSContext(aCx); + return; } JS::Rooted propertyHolder(aCx); @@ -2169,11 +2174,13 @@ ReparentWrapper(JSContext* aCx, JS::Handle aObjArg) 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; @@ -2189,7 +2196,8 @@ ReparentWrapper(JSContext* aCx, JS::Handle aObjArg) // 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 @@ -2244,9 +2252,6 @@ ReparentWrapper(JSContext* aCx, JS::Handle aObjArg) if (htmlobject) { htmlobject->SetupProtoChain(aCx, aObj); } - - // Now we can just return the wrapper - return NS_OK; } GlobalObject::GlobalObject(JSContext* aCx, JSObject* aObject) diff --git a/dom/bindings/BindingUtils.h b/dom/bindings/BindingUtils.h index 9ae5ed1f73..d55be9eb89 100644 --- a/dom/bindings/BindingUtils.h +++ b/dom/bindings/BindingUtils.h @@ -2758,8 +2758,8 @@ const nsAString& NonNullHelper(const binding_detail::FakeString& aArg) // Reparent the wrapper of aObj to whatever its native now thinks its // parent should be. -nsresult -ReparentWrapper(JSContext* aCx, JS::Handle aObj); +void +ReparentWrapper(JSContext* aCx, JS::Handle aObj, ErrorResult& aError); /** * Used to implement the Symbol.hasInstance property of an interface object. diff --git a/dom/xml/XMLDocument.cpp b/dom/xml/XMLDocument.cpp index ad11f5de83..cbcfdb2e6e 100644 --- a/dom/xml/XMLDocument.cpp +++ b/dom/xml/XMLDocument.cpp @@ -155,6 +155,8 @@ NS_NewDOMDocument(nsIDOMDocument** aInstancePtrResult, if (aDoctype) { nsCOMPtr tmpNode; rv = doc->AppendChild(aDoctype, getter_AddRefs(tmpNode)); + // TODO: if we choose to land bug 1318479, make sure to call + // result.WouldReportJSException() before stealing the NSResult. NS_ENSURE_SUCCESS(rv, rv); } @@ -167,6 +169,8 @@ NS_NewDOMDocument(nsIDOMDocument** aInstancePtrResult, nsCOMPtr tmpNode; rv = doc->AppendChild(root, getter_AddRefs(tmpNode)); + // TODO: if we choose to land bug 1318479, make sure to call + // result.WouldReportJSException() before stealing the NSResult. NS_ENSURE_SUCCESS(rv, rv); }