Bug 706301 - Don't cache own properties on XrayProxy. r=mrbkap

js/xpconnect/wrappers/WrapperFactory.cpp

@@ -397,8 +397,6 @@ WrapperFactory::Rewrap(JSContext *cx, JSObject *obj, JSObject *wrappedProto, JSO
     if (!wrapperObj || !xrayHolder)
         return wrapperObj;
 
-    // NB: The fact that the only wrappers to use ProxyExtra are XrayWrappers
-    // is relied on by XPCNativeWrapper.unwrap.
     js::SetProxyExtra(wrapperObj, 0, js::ObjectValue(*xrayHolder));
     return wrapperObj;
 }

js/xpconnect/wrappers/XrayWrapper.cpp

@@ -1083,6 +1083,9 @@ XrayProxy::~XrayProxy()
 {
 }
 
+// The 'holder' here isn't actually of [[Class]] HolderClass like those used by
+// XrayWrapper. Instead, it's a funny hybrid of the 'expando' and 'holder'
+// properties. However, we store it in the same slot. Exercise caution.
 static JSObject *
 GetHolderObject(JSContext *cx, JSObject *wrapper, bool createHolder = true)
 {
@@ -1130,13 +1133,9 @@ XrayProxy::getPropertyDescriptor(JSContext *cx, JSObject *wrapper, jsid id,
         return JS_WrapPropertyDescriptor(cx, desc);
     }
 
-    if (!JS_GetPropertyDescriptorById(cx, holder, id, JSRESOLVE_QUALIFIED, desc))
-        return false;
-    if (desc->obj) {
-        desc->obj = wrapper;
-        return true;
-    }
-
+    // We don't want to cache own properties on our holder. So we first do this
+    // call, and return if we find it (without caching). If we don't find it,
+    // we check the cache and do a full resolve (caching any result).
     if (!js::GetProxyHandler(obj)->getOwnPropertyDescriptor(cx, wrapper, id, set, desc))
         return false;
     if (desc->obj) {
@@ -1144,6 +1143,16 @@ XrayProxy::getPropertyDescriptor(JSContext *cx, JSObject *wrapper, jsid id,
         return true;
     }
 
+    // Now that we're sure this isn't an own property, look up cached non-own
+    // properties before calling all the way through.
+    if (!JS_GetPropertyDescriptorById(cx, holder, id, JSRESOLVE_QUALIFIED, desc))
+        return false;
+    if (desc->obj) {
+        desc->obj = wrapper;
+        return true;
+    }
+
+    // Nothing in the cache. Call through, and cache the result.
     if (!js::GetProxyHandler(obj)->getPropertyDescriptor(cx, wrapper, id, set, desc))
         return false;
 
@@ -1211,10 +1220,12 @@ XrayProxy::getOwnPropertyDescriptor(JSContext *cx, JSObject *wrapper, jsid id,
     if (!js::GetProxyHandler(obj)->getOwnPropertyDescriptor(cx, wrapper, id, set, desc))
         return false;
 
+    // The 'not found' property descriptor has obj == NULL.
+    if (desc->obj)
       desc->obj = wrapper;
 
-    return JS_DefinePropertyById(cx, holder, id, desc->value, desc->getter, desc->setter,
-                                 desc->attrs);
+    // Own properties don't get cached on the holder. Just return.
+    return true;
 }
 
 bool