From 370639ca37015938a4bedee21645f048bb048756 Mon Sep 17 00:00:00 2001 From: Jon Coppeard Date: Thu, 5 Mar 2015 13:39:53 +0000 Subject: [PATCH] Bug 1138874 - Change ReparentWrapper() to avoid multiple JS objects pointing to the same native r=bholley --- dom/bindings/BindingUtils.cpp | 95 +++++++++------------------ js/xpconnect/wrappers/XrayWrapper.cpp | 21 ++++++ 2 files changed, 53 insertions(+), 63 deletions(-) diff --git a/dom/bindings/BindingUtils.cpp b/dom/bindings/BindingUtils.cpp index 4b77b63e2d9..30c50b5ab27 100644 --- a/dom/bindings/BindingUtils.cpp +++ b/dom/bindings/BindingUtils.cpp @@ -1735,31 +1735,6 @@ DictionaryBase::AppendJSONToString(const char16_t* aJSONData, return true; } - -// Dynamically ensure that two objects don't end up with the same reserved slot. -class MOZ_STACK_CLASS AutoCloneDOMObjectSlotGuard -{ -public: - AutoCloneDOMObjectSlotGuard(JSContext* aCx, JSObject* aOld, JSObject* aNew) - : mOldReflector(aCx, aOld), mNewReflector(aCx, aNew) - { - MOZ_ASSERT(js::GetReservedOrProxyPrivateSlot(aOld, DOM_OBJECT_SLOT) == - js::GetReservedOrProxyPrivateSlot(aNew, DOM_OBJECT_SLOT)); - } - - ~AutoCloneDOMObjectSlotGuard() - { - if (js::GetReservedOrProxyPrivateSlot(mOldReflector, DOM_OBJECT_SLOT).toPrivate()) { - js::SetReservedOrProxyPrivateSlot(mNewReflector, DOM_OBJECT_SLOT, - JS::PrivateValue(nullptr)); - } - } - -private: - JS::Rooted mOldReflector; - JS::Rooted mNewReflector; -}; - nsresult ReparentWrapper(JSContext* aCx, JS::Handle aObjArg) { @@ -1804,9 +1779,7 @@ ReparentWrapper(JSContext* aCx, JS::Handle aObjArg) JSAutoCompartment newAc(aCx, newParent); // First we clone the reflector. We get a copy of its properties and clone its - // expando chain. The only part that is dangerous here is that if we have to - // return early we must avoid ending up with two reflectors pointing to the - // same native. Other than that, the objects we create will just go away. + // expando chain. JS::Handle proto = (domClass->mGetProto)(aCx, newParent); if (!proto) { @@ -1818,48 +1791,44 @@ ReparentWrapper(JSContext* aCx, JS::Handle aObjArg) return NS_ERROR_FAILURE; } - js::SetReservedOrProxyPrivateSlot(newobj, DOM_OBJECT_SLOT, - js::GetReservedOrProxyPrivateSlot(aObj, DOM_OBJECT_SLOT)); - - // At this point, both |aObj| and |newobj| point to the same native - // which is bad, because one of them will end up being finalized with a - // native it does not own. |cloneGuard| ensures that if we exit before - // clearing |aObj|'s reserved slot the reserved slot of |newobj| will be - // set to null. |aObj| will go away soon, because we swap it with - // another object during the transplant and let that object die. JS::Rooted propertyHolder(aCx); - { - AutoCloneDOMObjectSlotGuard cloneGuard(aCx, aObj, newobj); - - JS::Rooted copyFrom(aCx, isProxy ? expandoObject : aObj); - if (copyFrom) { - propertyHolder = JS_NewObjectWithGivenProto(aCx, nullptr, JS::NullPtr()); - if (!propertyHolder) { - return NS_ERROR_OUT_OF_MEMORY; - } - - if (!JS_CopyPropertiesFrom(aCx, propertyHolder, copyFrom)) { - return NS_ERROR_FAILURE; - } - } else { - propertyHolder = nullptr; + JS::Rooted copyFrom(aCx, isProxy ? expandoObject : aObj); + if (copyFrom) { + propertyHolder = JS_NewObjectWithGivenProto(aCx, nullptr, JS::NullPtr()); + if (!propertyHolder) { + return NS_ERROR_OUT_OF_MEMORY; } - // Expandos from other compartments are attached to the target JS object. - // Copy them over, and let the old ones die a natural death. - if (!xpc::XrayUtils::CloneExpandoChain(aCx, newobj, aObj)) { + if (!JS_CopyPropertiesFrom(aCx, propertyHolder, copyFrom)) { return NS_ERROR_FAILURE; } - - // We've set up |newobj|, so we make it own the native by 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. - js::SetReservedOrProxyPrivateSlot(aObj, DOM_OBJECT_SLOT, JS::PrivateValue(nullptr)); + } 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 already have been + // preserved called for this native. + if (!xpc::XrayUtils::CloneExpandoChain(aCx, newobj, aObj)) { + return NS_ERROR_FAILURE; + } + + // 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. + js::SetReservedOrProxyPrivateSlot(newobj, DOM_OBJECT_SLOT, + js::GetReservedOrProxyPrivateSlot(aObj, DOM_OBJECT_SLOT)); + js::SetReservedOrProxyPrivateSlot(aObj, DOM_OBJECT_SLOT, JS::PrivateValue(nullptr)); + aObj = xpc::TransplantObject(aCx, aObj, newobj); if (!aObj) { MOZ_CRASH(); diff --git a/js/xpconnect/wrappers/XrayWrapper.cpp b/js/xpconnect/wrappers/XrayWrapper.cpp index f184e5248a7..ce533da31a6 100644 --- a/js/xpconnect/wrappers/XrayWrapper.cpp +++ b/js/xpconnect/wrappers/XrayWrapper.cpp @@ -1002,6 +1002,27 @@ XrayTraits::cloneExpandoChain(JSContext *cx, HandleObject dst, HandleObject src) MOZ_ASSERT(getExpandoChain(dst) == nullptr); RootedObject oldHead(cx, getExpandoChain(src)); + +#ifdef DEBUG + // When this is called from dom::ReparentWrapper() there will be no native + // set for |dst|. Eventually it will be set to that of |src|. This will + // prevent attachExpandoObject() from preserving the wrapper, but this is + // not a problem because in this case the wrapper will already have been + // preserved when expandos were originally added to |src|. In this case + // assert the wrapper for |src| has been preserved. + if (oldHead) { + nsISupports *dstId = mozilla::dom::UnwrapDOMObjectToISupports(dst); + if (!dstId) { + nsISupports *srcId = mozilla::dom::UnwrapDOMObjectToISupports(src); + if (srcId) { + nsWrapperCache* cache = nullptr; + CallQueryInterface(srcId, &cache); + MOZ_ASSERT_IF(cache, cache->PreservingWrapper()); + } + } + } +#endif + while (oldHead) { RootedObject exclusive(cx, JS_GetReservedSlot(oldHead, JSSLOT_EXPANDO_EXCLUSIVE_GLOBAL)