Xray wrappers don't cache resolved native properties on the holder object (bug 633382, r=mrbkap/jst, a=blocker).
authorAndreas Gal <gal@mozilla.com>
Tue, 15 Feb 2011 19:00:55 -0800
changeset 62643 f9e8182eb1257804a319fc3b7eaa05e602b3ca1a
parent 62642 0d43e33ce1343e713bf7d1ddd31b6a1707bf4667
child 62644 b096f6bfce3015a862470756cd2861ecc80ef80c
push id1
push userroot
push dateTue, 10 Dec 2013 15:46:25 +0000
reviewersmrbkap, jst, blocker
bugs633382
milestone2.0b12pre
Xray wrappers don't cache resolved native properties on the holder object (bug 633382, r=mrbkap/jst, a=blocker).
dom/base/nsGlobalWindow.cpp
dom/base/nsJSEnvironment.cpp
js/src/jsapi.cpp
js/src/xpconnect/wrappers/XrayWrapper.cpp
--- a/dom/base/nsGlobalWindow.cpp
+++ b/dom/base/nsGlobalWindow.cpp
@@ -1915,16 +1915,25 @@ nsGlobalWindow::SetNewDocument(nsIDocume
     // We're reusing the current inner window.
     NS_ASSERTION(!currentInner->IsFrozen(),
                  "We should never be reusing a shared inner window");
     newInnerWindow = currentInner;
 
     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();
       mInnerWindowHolder = wsh->GetInnerWindowHolder();
       
       NS_ASSERTION(newInnerWindow, "Got a state without inner window");
 
       // These assignments addref.
--- a/dom/base/nsJSEnvironment.cpp
+++ b/dom/base/nsJSEnvironment.cpp
@@ -3183,19 +3183,17 @@ nsJSContext::ClearScope(void *aGlobalObj
     if (!JS_GetProperty(mContext, obj, "window", &window)) {
       window = JSVAL_VOID;
 
       JS_ClearPendingException(mContext);
     }
 
     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,
                              JS_PropertyStub, JS_StrictPropertyStub,
                              JSPROP_ENUMERATE | JSPROP_READONLY |
                              JSPROP_PERMANENT)) {
         JS_ClearPendingException(mContext);
       }
--- a/js/src/jsapi.cpp
+++ b/js/src/jsapi.cpp
@@ -1272,54 +1272,57 @@ JS_WrapValue(JSContext *cx, jsval *vp)
 {
     CHECK_REQUEST(cx);
     return cx->compartment->wrap(cx, Valueify(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);
     toTransplant.reserve(vector.length());
 
     for (JSCompartment **p = vector.begin(), **end = vector.end(); p != end; ++p) {
         WrapperMap &pmap = (*p)->crossCompartmentWrappers;
         if (WrapperMap::Ptr wp = pmap.lookup(origv)) {
@@ -1330,35 +1333,35 @@ JS_TransplantObject(JSContext *cx, JSObj
 
     for (Value *begin = toTransplant.begin(), *end = toTransplant.end(); begin != end; ++begin) {
         JSObject *wobj = &begin->toObject();
         JSCompartment *wcompartment = wobj->compartment();
         WrapperMap &pmap = wcompartment->crossCompartmentWrappers;
         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;
         pmap.put(targetv, ObjectValue(*wobj));
     }
 
     // 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))
             return NULL;
         if (!origobj->swap(cx, tobj))
             return NULL;
         origobj->getCompartment()->crossCompartmentWrappers.put(targetv, origv);
     }
--- a/js/src/xpconnect/wrappers/XrayWrapper.cpp
+++ b/js/src/xpconnect/wrappers/XrayWrapper.cpp
@@ -187,17 +187,17 @@ holder_get(JSContext *cx, JSObject *wrap
     JSObject *wnObject = GetWrappedNativeObjectFromHolder(cx, holder);
     XPCWrappedNative *wn = GetWrappedNative(wnObject);
     if (NATIVE_HAS_FLAG(wn, WantGetProperty)) {
         JSAutoEnterCompartment ac;
         if (!ac.enter(cx, holder))
             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;
         }
     }
     return true;
 }
 
@@ -214,17 +214,17 @@ holder_set(JSContext *cx, JSObject *wrap
 
     XPCWrappedNative *wn = GetWrappedNative(wnObject);
     if (NATIVE_HAS_FLAG(wn, WantSetProperty)) {
         JSAutoEnterCompartment ac;
         if (!ac.enter(cx, holder))
             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;
         }
     }
     return true;
 }
 
@@ -434,59 +434,73 @@ XrayWrapper<Base>::resolveOwnProperty(JS
         desc->attrs = JSPROP_ENUMERATE|JSPROP_SHARED;
         desc->getter = wrappedJSObject_getter;
         desc->setter = NULL;
         desc->shortid = 0;
         desc->value = JSVAL_VOID;
         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;
+
+    // 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;
+    }
+
+    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;
         }
 
-        if (desc->obj)
-            return true;
-    }
-
-    JSObject *wnObject = GetWrappedNativeObjectFromHolder(cx, holder);
-    XPCWrappedNative *wn = GetWrappedNative(wnObject);
-
-    // 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)) {
             if (retval)
                 XPCThrower::Throw(rv, cx);
             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;
 }
 
 template <typename Base>
 bool
 XrayWrapper<Base>::getPropertyDescriptor(JSContext *cx, JSObject *wrapper, jsid id,
                                          bool set, PropertyDescriptor *desc_in)
 {