From 68f04cdd1b24545e1d1db75387f2da8d836d44d9 Mon Sep 17 00:00:00 2001 From: Andreas Gal Date: Tue, 15 Feb 2011 19:00:55 -0800 Subject: [PATCH] Xray wrappers don't cache resolved native properties on the holder object (bug 633382, r=mrbkap/jst, a=blocker). --- dom/base/nsGlobalWindow.cpp | 9 +++ dom/base/nsJSEnvironment.cpp | 4 +- js/src/jsapi.cpp | 67 ++++++++++++----------- js/src/xpconnect/wrappers/XrayWrapper.cpp | 64 +++++++++++++--------- 4 files changed, 84 insertions(+), 60 deletions(-) diff --git a/dom/base/nsGlobalWindow.cpp b/dom/base/nsGlobalWindow.cpp index 8a5545bb487..a62cf688ea6 100644 --- a/dom/base/nsGlobalWindow.cpp +++ b/dom/base/nsGlobalWindow.cpp @@ -1920,6 +1920,15 @@ nsGlobalWindow::SetNewDocument(nsIDocument* aDocument, if (aDocument != oldDoc) { nsWindowSH::InvalidateGlobalScopePolluter(cx, currentInner->mJSObject); } + + // The API we're really looking for here is to go clear all of the + // Xray wrappers associated with our outer window. However, we + // don't expose that API because the implementation would be + // identical to that of JS_TransplantObject, so we just call that + // instead. + if (!JS_TransplantObject(cx, mJSObject, mJSObject)) { + return NS_ERROR_FAILURE; + } } else { if (aState) { newInnerWindow = wsh->GetInnerWindow(); diff --git a/dom/base/nsJSEnvironment.cpp b/dom/base/nsJSEnvironment.cpp index 2b50f8269c7..0eb8109cc3f 100644 --- a/dom/base/nsJSEnvironment.cpp +++ b/dom/base/nsJSEnvironment.cpp @@ -3188,9 +3188,7 @@ nsJSContext::ClearScope(void *aGlobalObj, PRBool aClearFromProtoChain) JS_ClearScope(mContext, obj); - if (xpc::WrapperFactory::IsXrayWrapper(obj)) { - JS_ClearScope(mContext, &obj->getProxyExtra().toObject()); - } + NS_ABORT_IF_FALSE(!xpc::WrapperFactory::IsXrayWrapper(obj), "unexpected wrapper"); if (window != JSVAL_VOID) { if (!JS_DefineProperty(mContext, obj, "window", window, diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp index ff4324d7b0d..3afea1d265a 100644 --- a/js/src/jsapi.cpp +++ b/js/src/jsapi.cpp @@ -1277,44 +1277,47 @@ JS_WrapValue(JSContext *cx, jsval *vp) JS_PUBLIC_API(JSObject *) JS_TransplantObject(JSContext *cx, JSObject *origobj, JSObject *target) { - // This function is called when an object moves between two different - // compartments. In that case, we need to "move" the window from origobj's - // compartment to target's compartment. + // This function is called when an object moves between two + // different compartments. In that case, we need to "move" the + // window from origobj's compartment to target's compartment. JSCompartment *destination = target->getCompartment(); + WrapperMap &map = destination->crossCompartmentWrappers; + Value origv = ObjectValue(*origobj); + JSObject *obj; + if (origobj->getCompartment() == destination) { // If the original object is in the same compartment as the // destination, then we know that we won't find wrapper in the - // destination's cross compartment map and that the same object - // will continue to work. - if (!origobj->swap(cx, target)) + // destination's cross compartment map and that the same + // object will continue to work. Note the rare case where + // |origobj == target|. In that case, we can just treat this + // as a same compartment navigation. The effect is to clear + // all of the wrappers and their holders if they have + // them. This would be cleaner as a separate API. + if (origobj != target && !origobj->swap(cx, target)) return NULL; - return origobj; - } - - JSObject *obj; - WrapperMap &map = destination->crossCompartmentWrappers; - Value origv = ObjectValue(*origobj); - - // There might already be a wrapper for the original object in the new - // compartment. - if (WrapperMap::Ptr p = map.lookup(origv)) { - // If there is, make it the primary outer window proxy around the - // inner (accomplished by swapping target's innards with the old, - // possibly security wrapper, innards). + obj = origobj; + } else if (WrapperMap::Ptr p = map.lookup(origv)) { + // There might already be a wrapper for the original object in + // the new compartment. If there is, make it the primary outer + // window proxy around the inner (accomplished by swapping + // target's innards with the old, possibly security wrapper, + // innards). obj = &p->value.toObject(); map.remove(p); if (!obj->swap(cx, target)) return NULL; } else { - // Otherwise, this is going to be our outer window proxy in the new - // compartment. + // Otherwise, this is going to be our outer window proxy in + // the new compartment. obj = target; } - // Now, iterate through other scopes looking for references to the old - // outer window. They need to be updated to point at the new outer window. - // They also might transition between different types of security wrappers - // based on whether the new compartment is same origin with them. + // Now, iterate through other scopes looking for references to the + // old outer window. They need to be updated to point at the new + // outer window. They also might transition between different + // types of security wrappers based on whether the new compartment + // is same origin with them. Value targetv = ObjectValue(*obj); WrapperVector &vector = cx->runtime->compartments; AutoValueVector toTransplant(cx); @@ -1335,17 +1338,17 @@ JS_TransplantObject(JSContext *cx, JSObject *origobj, JSObject *target) JS_ASSERT(pmap.lookup(origv)); pmap.remove(origv); - // First, we wrap it in the new compartment. This will return a - // new wrapper. + // First, we wrap it in the new compartment. This will return + // a new wrapper. AutoCompartment ac(cx, wobj); JSObject *tobj = obj; if (!ac.enter() || !wcompartment->wrap(cx, &tobj)) return NULL; - // Now, because we need to maintain object identity, we do a brain - // transplant on the old object. At the same time, we update the - // entry in the compartment's wrapper map to point to the old - // wrapper. + // Now, because we need to maintain object identity, we do a + // brain transplant on the old object. At the same time, we + // update the entry in the compartment's wrapper map to point + // to the old wrapper. JS_ASSERT(tobj != wobj); if (!wobj->swap(cx, tobj)) return NULL; @@ -1353,7 +1356,7 @@ JS_TransplantObject(JSContext *cx, JSObject *origobj, JSObject *target) } // Lastly, update the original object to point to the new one. - { + if (origobj->getCompartment() != destination) { AutoCompartment ac(cx, origobj); JSObject *tobj = obj; if (!ac.enter() || !JS_WrapObject(cx, &tobj)) diff --git a/js/src/xpconnect/wrappers/XrayWrapper.cpp b/js/src/xpconnect/wrappers/XrayWrapper.cpp index 304084fa805..4178fc968a9 100644 --- a/js/src/xpconnect/wrappers/XrayWrapper.cpp +++ b/js/src/xpconnect/wrappers/XrayWrapper.cpp @@ -192,7 +192,7 @@ holder_get(JSContext *cx, JSObject *wrapper, jsid id, jsval *vp) return false; JSBool retval = true; nsresult rv = wn->GetScriptableCallback()->GetProperty(wn, cx, wrapper, id, vp, &retval); - if (NS_FAILED(rv)) { + if (NS_FAILED(rv) || !retval) { if (retval) XPCThrower::Throw(rv, cx); return false; @@ -219,7 +219,7 @@ holder_set(JSContext *cx, JSObject *wrapper, jsid id, JSBool strict, jsval *vp) return false; JSBool retval = true; nsresult rv = wn->GetScriptableCallback()->SetProperty(wn, cx, wrapper, id, vp, &retval); - if (NS_FAILED(rv)) { + if (NS_FAILED(rv) || !retval) { if (retval) XPCThrower::Throw(rv, cx); return false; @@ -439,27 +439,38 @@ XrayWrapper::resolveOwnProperty(JSContext *cx, JSObject *wrapper, jsid id, return true; } + desc->obj = NULL; + + uintN flags = (set ? JSRESOLVE_ASSIGNING : 0) | JSRESOLVE_QUALIFIED; JSObject *holder = GetHolder(wrapper); JSObject *expando = GetExpandoObject(holder); - if (expando) { - if (!JS_GetPropertyDescriptorById(cx, expando, id, - (set ? JSRESOLVE_ASSIGNING : 0) | JSRESOLVE_QUALIFIED, - desc)) { - return false; - } - if (desc->obj) - return true; + // Check for expando properties first. + if (expando && !JS_GetPropertyDescriptorById(cx, expando, id, flags, desc)) { + return false; + } + if (desc->obj) { + // Pretend the property lives on the wrapper. + desc->obj = wrapper; + return true; } - JSObject *wnObject = GetWrappedNativeObjectFromHolder(cx, holder); - XPCWrappedNative *wn = GetWrappedNative(wnObject); + JSBool hasProp; + if (!JS_HasPropertyById(cx, holder, id, &hasProp)) { + return false; + } + if (!hasProp) { + JSObject *wnObject = GetWrappedNativeObjectFromHolder(cx, holder); + XPCWrappedNative *wn = GetWrappedNative(wnObject); + + // Run the resolve hook of the wrapped native. + if (!NATIVE_HAS_FLAG(wn, WantNewResolve)) { + desc->obj = nsnull; + return true; + } - // Run the resolve hook of the wrapped native. - if (NATIVE_HAS_FLAG(wn, WantNewResolve)) { JSBool retval = true; JSObject *pobj = NULL; - uintN flags = (set ? JSRESOLVE_ASSIGNING : 0) | JSRESOLVE_QUALIFIED; nsresult rv = wn->GetScriptableInfo()->GetCallback()->NewResolve(wn, cx, wrapper, id, flags, &pobj, &retval); if (NS_FAILED(rv)) { @@ -468,20 +479,23 @@ XrayWrapper::resolveOwnProperty(JSContext *cx, JSObject *wrapper, jsid id, return false; } - if (pobj) { -#ifdef DEBUG - JSBool hasProp; - NS_ASSERTION(JS_HasPropertyById(cx, holder, id, &hasProp) && - hasProp, "id got defined somewhere else?"); -#endif - if (!JS_GetPropertyDescriptorById(cx, holder, id, flags, desc)) - return false; - desc->obj = wrapper; + if (!pobj) { + desc->obj = nsnull; return true; } + +#ifdef DEBUG + NS_ASSERTION(JS_HasPropertyById(cx, holder, id, &hasProp) && + hasProp, "id got defined somewhere else?"); +#endif } - desc->obj = nsnull; + if (!JS_GetPropertyDescriptorById(cx, holder, id, flags, desc)) + return false; + + // Pretend we found the property on the wrapper, not the holder. + desc->obj = wrapper; + return true; }