Bug 836301 - Hoist enter() calls from {Xray,}Wrapper::foo into Proxy::foo. r=mrbkap

js/src/jsproxy.cpp

@@ -2200,7 +2200,6 @@ ScriptedDirectProxyHandler ScriptedDirectProxyHandler::singleton;
         return protoCall;                                                    \
     JS_END_MACRO                                                             \
 
-
 bool
 Proxy::getPropertyDescriptor(JSContext *cx, JSObject *proxy_, jsid id_, PropertyDescriptor *desc,
                              unsigned flags)
@@ -2209,6 +2208,10 @@ Proxy::getPropertyDescriptor(JSContext *cx, JSObject *proxy_, jsid id_, Property
     RootedObject proxy(cx, proxy_);
     RootedId id(cx, id_);
     BaseProxyHandler *handler = GetProxyHandler(proxy);
+    desc->obj = NULL; // default result if we refuse to perform this action
+    AutoEnterPolicy policy(cx, handler, proxy, id, BaseProxyHandler::GET, true);
+    if (!policy.allowed())
+        return policy.returnValue();
     if (!handler->hasPrototype())
         return handler->getPropertyDescriptor(cx, proxy, id, desc, flags);
     if (!handler->getOwnPropertyDescriptor(cx, proxy, id, desc, flags))
@@ -2241,7 +2244,12 @@ Proxy::getOwnPropertyDescriptor(JSContext *cx, JSObject *proxy_, jsid id, Proper
 {
     JS_CHECK_RECURSION(cx, return false);
     RootedObject proxy(cx, proxy_);
-    return GetProxyHandler(proxy)->getOwnPropertyDescriptor(cx, proxy, id, desc, flags);
+    BaseProxyHandler *handler = GetProxyHandler(proxy);
+    desc->obj = NULL; // default result if we refuse to perform this action
+    AutoEnterPolicy policy(cx, handler, proxy, id, BaseProxyHandler::GET, true);
+    if (!policy.allowed())
+        return policy.returnValue();
+    return handler->getOwnPropertyDescriptor(cx, proxy, id, desc, flags);
 }
 
 bool
@@ -2265,7 +2273,11 @@ bool
 Proxy::defineProperty(JSContext *cx, JSObject *proxy_, jsid id, PropertyDescriptor *desc)
 {
     JS_CHECK_RECURSION(cx, return false);
+    BaseProxyHandler *handler = GetProxyHandler(proxy_);
     RootedObject proxy(cx, proxy_);
+    AutoEnterPolicy policy(cx, handler, proxy, id, BaseProxyHandler::SET, true);
+    if (!policy.allowed())
+        return policy.returnValue();
     return GetProxyHandler(proxy)->defineProperty(cx, proxy, id, desc);
 }
 
@@ -2284,7 +2296,11 @@ bool
 Proxy::getOwnPropertyNames(JSContext *cx, JSObject *proxy_, AutoIdVector &props)
 {
     JS_CHECK_RECURSION(cx, return false);
+    BaseProxyHandler *handler = GetProxyHandler(proxy_);
     RootedObject proxy(cx, proxy_);
+    AutoEnterPolicy policy(cx, handler, proxy, JSID_VOID, BaseProxyHandler::GET, true);
+    if (!policy.allowed())
+        return policy.returnValue();
     return GetProxyHandler(proxy)->getOwnPropertyNames(cx, proxy, props);
 }
 
@@ -2292,7 +2308,12 @@ bool
 Proxy::delete_(JSContext *cx, JSObject *proxy_, jsid id, bool *bp)
 {
     JS_CHECK_RECURSION(cx, return false);
+    BaseProxyHandler *handler = GetProxyHandler(proxy_);
     RootedObject proxy(cx, proxy_);
+    *bp = true; // default result if we refuse to perform this action
+    AutoEnterPolicy policy(cx, handler, proxy, id, BaseProxyHandler::SET, true);
+    if (!policy.allowed())
+        return policy.returnValue();
     return GetProxyHandler(proxy)->delete_(cx, proxy, id, bp);
 }
 
@@ -2322,6 +2343,9 @@ Proxy::enumerate(JSContext *cx, JSObject *proxy_, AutoIdVector &props)
     JS_CHECK_RECURSION(cx, return false);
     RootedObject proxy(cx, proxy_);
     BaseProxyHandler *handler = GetProxyHandler(proxy);
+    AutoEnterPolicy policy(cx, handler, proxy, JSID_VOID, BaseProxyHandler::GET, true);
+    if (!policy.allowed())
+        return policy.returnValue();
     if (!handler->hasPrototype())
         return GetProxyHandler(proxy)->enumerate(cx, proxy, props);
     if (!handler->keys(cx, proxy, props))
@@ -2339,6 +2363,10 @@ Proxy::has(JSContext *cx, JSObject *proxy_, jsid id_, bool *bp)
     RootedObject proxy(cx, proxy_);
     RootedId id(cx, id_);
     BaseProxyHandler *handler = GetProxyHandler(proxy);
+    *bp = false; // default result if we refuse to perform this action
+    AutoEnterPolicy policy(cx, handler, proxy, id, BaseProxyHandler::GET, true);
+    if (!policy.allowed())
+        return policy.returnValue();
     if (!handler->hasPrototype())
         return handler->has(cx, proxy, id, bp);
     if (!handler->hasOwn(cx, proxy, id, bp))
@@ -2356,7 +2384,12 @@ Proxy::hasOwn(JSContext *cx, JSObject *proxy_, jsid id, bool *bp)
 {
     JS_CHECK_RECURSION(cx, return false);
     RootedObject proxy(cx, proxy_);
-    return GetProxyHandler(proxy)->hasOwn(cx, proxy, id, bp);
+    BaseProxyHandler *handler = GetProxyHandler(proxy);
+    *bp = false; // default result if we refuse to perform this action
+    AutoEnterPolicy policy(cx, handler, proxy, id, BaseProxyHandler::GET, true);
+    if (!policy.allowed())
+        return policy.returnValue();
+    return handler->hasOwn(cx, proxy, id, bp);
 }
 
 bool
@@ -2365,6 +2398,10 @@ Proxy::get(JSContext *cx, HandleObject proxy, HandleObject receiver, HandleId id
 {
     JS_CHECK_RECURSION(cx, return false);
     BaseProxyHandler *handler = GetProxyHandler(proxy);
+    vp.setUndefined(); // default result if we refuse to perform this action
+    AutoEnterPolicy policy(cx, handler, proxy, id, BaseProxyHandler::GET, true);
+    if (!policy.allowed())
+        return policy.returnValue();
     bool own;
     if (!handler->hasPrototype()) {
         own = true;
@@ -2383,17 +2420,20 @@ Proxy::getElementIfPresent(JSContext *cx, HandleObject proxy, HandleObject recei
 {
     JS_CHECK_RECURSION(cx, return false);
 
-    BaseProxyHandler *handler = GetProxyHandler(proxy);
-
-    if (!handler->hasPrototype()) {
-        return GetProxyHandler(proxy)->getElementIfPresent(cx, proxy, receiver, index,
-                                                           vp.address(), present);
-    }
-
     RootedId id(cx);
     if (!IndexToId(cx, index, &id))
         return false;
 
+    BaseProxyHandler *handler = GetProxyHandler(proxy);
+    AutoEnterPolicy policy(cx, handler, proxy, id, BaseProxyHandler::GET, true);
+    if (!policy.allowed())
+        return policy.returnValue();
+
+    if (!handler->hasPrototype()) {
+        return handler->getElementIfPresent(cx, proxy, receiver, index,
+                                            vp.address(), present);
+    }
+
     bool hasOwn;
     if (!handler->hasOwn(cx, proxy, id, &hasOwn))
         return false;
@@ -2413,6 +2453,9 @@ Proxy::set(JSContext *cx, HandleObject proxy, HandleObject receiver, HandleId id
 {
     JS_CHECK_RECURSION(cx, return false);
     BaseProxyHandler *handler = GetProxyHandler(proxy);
+    AutoEnterPolicy policy(cx, handler, proxy, id, BaseProxyHandler::SET, true);
+    if (!policy.allowed())
+        return policy.returnValue();
     if (handler->hasPrototype()) {
         // If we're using a prototype, we still want to use the proxy trap unless
         // we have a non-own property with a setter.
@@ -2440,7 +2483,11 @@ Proxy::keys(JSContext *cx, JSObject *proxy_, AutoIdVector &props)
 {
     JS_CHECK_RECURSION(cx, return false);
     RootedObject proxy(cx, proxy_);
-    return GetProxyHandler(proxy)->keys(cx, proxy, props);
+    BaseProxyHandler *handler = GetProxyHandler(proxy);
+    AutoEnterPolicy policy(cx, handler, proxy, JSID_VOID, BaseProxyHandler::GET, true);
+    if (!policy.allowed())
+        return policy.returnValue();
+    return handler->keys(cx, proxy, props);
 }
 
 bool
@@ -2448,8 +2495,19 @@ Proxy::iterate(JSContext *cx, HandleObject proxy, unsigned flags, MutableHandleV
 {
     JS_CHECK_RECURSION(cx, return false);
     BaseProxyHandler *handler = GetProxyHandler(proxy);
-    if (!handler->hasPrototype())
+    vp.setUndefined(); // default result if we refuse to perform this action
-        return GetProxyHandler(proxy)->iterate(cx, proxy, flags, vp.address());
+    if (!handler->hasPrototype()) {
+        AutoEnterPolicy policy(cx, handler, proxy, JSID_VOID,
+                               BaseProxyHandler::GET, /* mayThrow = */ false);
+        // If the policy denies access but wants us to return true, we need
+        // to hand a valid (empty) iterator object to the caller.
+        if (!policy.allowed()) {
+            AutoIdVector props(cx);
+            return policy.returnValue() &&
+                   EnumeratedIdVectorToIterator(cx, proxy, flags, props, vp);
+        }
+        return handler->iterate(cx, proxy, flags, vp.address());
+    }
     AutoIdVector props(cx);
     // The other Proxy::foo methods do the prototype-aware work for us here.
     if ((flags & JSITER_OWNONLY)
@@ -2465,7 +2523,19 @@ Proxy::call(JSContext *cx, JSObject *proxy_, unsigned argc, Value *vp)
 {
     JS_CHECK_RECURSION(cx, return false);
     RootedObject proxy(cx, proxy_);
-    return GetProxyHandler(proxy)->call(cx, proxy, argc, vp);
+    BaseProxyHandler *handler = GetProxyHandler(proxy);
+
+    // Because vp[0] is JS_CALLEE on the way in and JS_RVAL on the way out, we
+    // can only set our default value once we're sure that we're not calling the
+    // trap.
+    AutoEnterPolicy policy(cx, handler, proxy, JSID_VOID,
+                           BaseProxyHandler::CALL, true);
+    if (!policy.allowed()) {
+        vp->setUndefined();
+        return policy.returnValue();
+    }
+
+    return handler->call(cx, proxy, argc, vp);
 }
 
 bool
@@ -2473,7 +2543,19 @@ Proxy::construct(JSContext *cx, JSObject *proxy_, unsigned argc, Value *argv, Va
 {
     JS_CHECK_RECURSION(cx, return false);
     RootedObject proxy(cx, proxy_);
-    return GetProxyHandler(proxy)->construct(cx, proxy, argc, argv, rval);
+    BaseProxyHandler *handler = GetProxyHandler(proxy);
+
+    // Because vp[0] is JS_CALLEE on the way in and JS_RVAL on the way out, we
+    // can only set our default value once we're sure that we're not calling the
+    // trap.
+    AutoEnterPolicy policy(cx, handler, proxy, JSID_VOID,
+                           BaseProxyHandler::CALL, true);
+    if (!policy.allowed()) {
+        rval->setUndefined();
+        return policy.returnValue();
+    }
+
+    return handler->construct(cx, proxy, argc, argv, rval);
 }
 
 bool
@@ -2481,6 +2563,9 @@ Proxy::nativeCall(JSContext *cx, IsAcceptableThis test, NativeImpl impl, CallArg
 {
     JS_CHECK_RECURSION(cx, return false);
     Rooted<JSObject*> proxy(cx, &args.thisv().toObject());
+    // Note - we don't enter a policy here because our security architecture
+    // guards against nativeCall by overriding the trap itself in the right
+    // circumstances.
     return GetProxyHandler(proxy)->nativeCall(cx, test, impl, args);
 }
 
@@ -2488,6 +2573,11 @@ bool
 Proxy::hasInstance(JSContext *cx, HandleObject proxy, MutableHandleValue v, bool *bp)
 {
     JS_CHECK_RECURSION(cx, return false);
+    BaseProxyHandler *handler = GetProxyHandler(proxy);
+    *bp = false; // default result if we refuse to perform this action
+    AutoEnterPolicy policy(cx, handler, proxy, JSID_VOID, BaseProxyHandler::GET, true);
+    if (!policy.allowed())
+        return policy.returnValue();
     return GetProxyHandler(proxy)->hasInstance(cx, proxy, v, bp);
 }
 
@@ -2503,7 +2593,14 @@ Proxy::obj_toString(JSContext *cx, JSObject *proxy_)
 {
     JS_CHECK_RECURSION(cx, return NULL);
     RootedObject proxy(cx, proxy_);
-    return GetProxyHandler(proxy)->obj_toString(cx, proxy);
+    BaseProxyHandler *handler = GetProxyHandler(proxy);
+    AutoEnterPolicy policy(cx, handler, proxy, JSID_VOID,
+                           BaseProxyHandler::GET, /* mayThrow = */ false);
+    // Do the safe thing if the policy rejects.
+    if (!policy.allowed()) {
+        return handler->BaseProxyHandler::obj_toString(cx, proxy);
+    }
+    return handler->obj_toString(cx, proxy);
 }
 
 JSString *
@@ -2511,7 +2608,17 @@ Proxy::fun_toString(JSContext *cx, JSObject *proxy_, unsigned indent)
 {
     JS_CHECK_RECURSION(cx, return NULL);
     RootedObject proxy(cx, proxy_);
-    return GetProxyHandler(proxy)->fun_toString(cx, proxy, indent);
+    BaseProxyHandler *handler = GetProxyHandler(proxy);
+    AutoEnterPolicy policy(cx, handler, proxy, JSID_VOID,
+                           BaseProxyHandler::GET, /* mayThrow = */ false);
+    // Do the safe thing if the policy rejects.
+    if (!policy.allowed()) {
+        if (proxy->isCallable())
+            return JS_NewStringCopyZ(cx, "function () {\n    [native code]\n}");
+        ReportIsNotFunction(cx, ObjectValue(*proxy));
+        return NULL;
+    }
+    return handler->fun_toString(cx, proxy, indent);
 }
 
 bool

js/src/jswrapper.cpp

@@ -115,17 +115,6 @@ js::IsCrossCompartmentWrapper(RawObject wrapper)
            !!(Wrapper::wrapperHandler(wrapper)->flags() & Wrapper::CROSS_COMPARTMENT);
 }
 
-#define CHECKED(op, act)                                                     \
-    JS_BEGIN_MACRO                                                           \
-        AutoEnterPolicy policy(cx, this, wrapper, id, act, true);            \
-        if (!policy.allowed())                                               \
-            return policy.returnValue();                                     \
-        return (op);                                                         \
-    JS_END_MACRO
-
-#define SET(action) CHECKED(action, SET)
-#define GET(action) CHECKED(action, GET)
-
 Wrapper::Wrapper(unsigned flags, bool hasPrototype) : DirectProxyHandler(&sWrapperFamily)
                                                     , mFlags(flags)
                                                     , mSafeToUnwrap(true)
@@ -143,8 +132,7 @@ Wrapper::getPropertyDescriptor(JSContext *cx, JSObject *wrapperArg,
 {
     RootedObject wrapper(cx, wrapperArg);
     JS_ASSERT(!hasPrototype()); // Should never be called when there's a prototype.
-    desc->obj = NULL; // default result if we refuse to perform this action
+    return DirectProxyHandler::getPropertyDescriptor(cx, wrapper, id, desc, flags);
-    CHECKED(DirectProxyHandler::getPropertyDescriptor(cx, wrapper, id, desc, flags), GET);
 }
 
 bool
@@ -152,8 +140,7 @@ Wrapper::getOwnPropertyDescriptor(JSContext *cx, JSObject *wrapperArg,
                                   jsid id, PropertyDescriptor *desc, unsigned flags)
 {
     RootedObject wrapper(cx, wrapperArg);
-    desc->obj = NULL; // default result if we refuse to perform this action
+    return DirectProxyHandler::getOwnPropertyDescriptor(cx, wrapper, id, desc, flags);
-    CHECKED(DirectProxyHandler::getOwnPropertyDescriptor(cx, wrapper, id, desc, flags), GET);
 }
 
 bool
@@ -161,7 +148,7 @@ Wrapper::defineProperty(JSContext *cx, JSObject *wrapperArg, jsid id,
                         PropertyDescriptor *desc)
 {
     RootedObject wrapper(cx, wrapperArg);
-    SET(DirectProxyHandler::defineProperty(cx, wrapper, id, desc));
+    return DirectProxyHandler::defineProperty(cx, wrapper, id, desc);
 }
 
 bool
@@ -169,17 +156,14 @@ Wrapper::getOwnPropertyNames(JSContext *cx, JSObject *wrapperArg,
                              AutoIdVector &props)
 {
     RootedObject wrapper(cx, wrapperArg);
-    // if we refuse to perform this action, props remains empty
+    return DirectProxyHandler::getOwnPropertyNames(cx, wrapper, props);
-    jsid id = JSID_VOID;
-    GET(DirectProxyHandler::getOwnPropertyNames(cx, wrapper, props));
 }
 
 bool
 Wrapper::delete_(JSContext *cx, JSObject *wrapperArg, jsid id, bool *bp)
 {
     RootedObject wrapper(cx, wrapperArg);
-    *bp = true; // default result if we refuse to perform this action
+    return DirectProxyHandler::delete_(cx, wrapper, id, bp);
-    SET(DirectProxyHandler::delete_(cx, wrapper, id, bp));
 }
 
 bool
@@ -187,9 +171,7 @@ Wrapper::enumerate(JSContext *cx, JSObject *wrapperArg, AutoIdVector &props)
 {
     RootedObject wrapper(cx, wrapperArg);
     JS_ASSERT(!hasPrototype()); // Should never be called when there's a prototype.
-    // if we refuse to perform this action, props remains empty
+    return DirectProxyHandler::enumerate(cx, wrapper, props);
-    static jsid id = JSID_VOID;
-    GET(DirectProxyHandler::enumerate(cx, wrapper, props));
 }
 
 /*
@@ -231,24 +213,21 @@ Wrapper::has(JSContext *cx, JSObject *wrapperArg, jsid id, bool *bp)
 {
     RootedObject wrapper(cx, wrapperArg);
     JS_ASSERT(!hasPrototype()); // Should never be called when there's a prototype.
-    *bp = false; // default result if we refuse to perform this action
+    return DirectProxyHandler::has(cx, wrapper, id, bp);
-    GET(DirectProxyHandler::has(cx, wrapper, id, bp));
 }
 
 bool
 Wrapper::hasOwn(JSContext *cx, JSObject *wrapperArg, jsid id, bool *bp)
 {
     RootedObject wrapper(cx, wrapperArg);
-    *bp = false; // default result if we refuse to perform this action
+    return DirectProxyHandler::hasOwn(cx, wrapper, id, bp);
-    GET(DirectProxyHandler::hasOwn(cx, wrapper, id, bp));
 }
 
 bool
 Wrapper::get(JSContext *cx, JSObject *wrapperArg, JSObject *receiver, jsid id, Value *vp)
 {
     RootedObject wrapper(cx, wrapperArg);
-    vp->setUndefined(); // default result if we refuse to perform this action
+    return DirectProxyHandler::get(cx, wrapper, receiver, id, vp);
-    GET(DirectProxyHandler::get(cx, wrapper, receiver, id, vp));
 }
 
 bool
@@ -256,16 +235,14 @@ Wrapper::set(JSContext *cx, JSObject *wrapperArg, JSObject *receiver, jsid id, b
              Value *vp)
 {
     RootedObject wrapper(cx, wrapperArg);
-    SET(DirectProxyHandler::set(cx, wrapper, receiver, id, strict, vp));
+    return DirectProxyHandler::set(cx, wrapper, receiver, id, strict, vp);
 }
 
 bool
 Wrapper::keys(JSContext *cx, JSObject *wrapperArg, AutoIdVector &props)
 {
     RootedObject wrapper(cx, wrapperArg);
-    // if we refuse to perform this action, props remains empty
+    return DirectProxyHandler::keys(cx, wrapper, props);
-    const jsid id = JSID_VOID;
-    GET(DirectProxyHandler::keys(cx, wrapper, props));
 }
 
 bool
@@ -273,27 +250,21 @@ Wrapper::iterate(JSContext *cx, JSObject *wrapperArg, unsigned flags, Value *vp)
 {
     RootedObject wrapper(cx, wrapperArg);
     JS_ASSERT(!hasPrototype()); // Should never be called when there's a prototype.
-    vp->setUndefined(); // default result if we refuse to perform this action
+    return DirectProxyHandler::iterate(cx, wrapper, flags, vp);
-    const jsid id = JSID_VOID;
-    GET(DirectProxyHandler::iterate(cx, wrapper, flags, vp));
 }
 
 bool
 Wrapper::call(JSContext *cx, JSObject *wrapperArg, unsigned argc, Value *vp)
 {
     RootedObject wrapper(cx, wrapperArg);
-    vp->setUndefined(); // default result if we refuse to perform this action
+    return DirectProxyHandler::call(cx, wrapper, argc, vp);
-    const jsid id = JSID_VOID;
-    CHECKED(DirectProxyHandler::call(cx, wrapper, argc, vp), CALL);
 }
 
 bool
 Wrapper::construct(JSContext *cx, JSObject *wrapperArg, unsigned argc, Value *argv, Value *vp)
 {
     RootedObject wrapper(cx, wrapperArg);
-    vp->setUndefined(); // default result if we refuse to perform this action
+    return DirectProxyHandler::construct(cx, wrapper, argc, argv, vp);
-    const jsid id = JSID_VOID;
-    CHECKED(DirectProxyHandler::construct(cx, wrapper, argc, argv, vp), CALL);
 }
 
 bool
@@ -308,23 +279,13 @@ Wrapper::nativeCall(JSContext *cx, IsAcceptableThis test, NativeImpl impl, CallA
 bool
 Wrapper::hasInstance(JSContext *cx, HandleObject wrapper, MutableHandleValue v, bool *bp)
 {
-    *bp = false; // default result if we refuse to perform this action
+    return DirectProxyHandler::hasInstance(cx, wrapper, v, bp);
-    const jsid id = JSID_VOID;
-    GET(DirectProxyHandler::hasInstance(cx, wrapper, v, bp));
 }
 
 JSString *
 Wrapper::obj_toString(JSContext *cx, JSObject *wrapperArg)
 {
     RootedObject wrapper(cx, wrapperArg);
-    bool status;
-    if (!enter(cx, wrapper, JSID_VOID, GET, &status)) {
-        if (status) {
-            // Perform some default behavior that doesn't leak any information.
-            return JS_NewStringCopyZ(cx, "[object Object]");
-        }
-        return NULL;
-    }
     JSString *str = DirectProxyHandler::obj_toString(cx, wrapper);
     return str;
 }
@@ -332,17 +293,6 @@ Wrapper::obj_toString(JSContext *cx, JSObject *wrapperArg)
 JSString *
 Wrapper::fun_toString(JSContext *cx, JSObject *wrapper, unsigned indent)
 {
-    bool status;
-    if (!enter(cx, wrapper, JSID_VOID, GET, &status)) {
-        if (status) {
-            // Perform some default behavior that doesn't leak any information.
-            if (wrapper->isCallable())
-                return JS_NewStringCopyZ(cx, "function () {\n    [native code]\n}");
-            ReportIsNotFunction(cx, ObjectValue(*wrapper));
-            return NULL;
-        }
-        return NULL;
-    }
     JSString *str = DirectProxyHandler::fun_toString(cx, wrapper, indent);
     return str;
 }

js/xpconnect/tests/chrome/test_bug760109.xul

@@ -31,12 +31,13 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=760109
     ok(protoProto === Object.prototype,
        "Object prototype remapped properly");
 
-    // Check |constructor|. The semantics of this weird for the case of an
+    // Check |constructor|.
-    // object with a custom chrome-implemented prototype, because we'll end up
+    // Note that the 'constructor' property of the underlying chrome object
-    // bouncing up the prototype chain to Object, even though that's not fully
+    // will be resolved on SomeConstructor.prototype, which has an empty
-    // accurate. It's unlikely to be a problem though, so we just verify that
+    // __exposedProps__. This means that we shouldn't remap the property, even
-    // it does what we expect.
+    // though we'd also be able to find it on Object.prototype. Some recent
-    ok(chromeObject.constructor === Object, "Object constructor does what we expect");
+    // refactoring has made it possible to do the right thing here.
+    is(typeof chromeObject.constructor, "undefined", "Object constructor does what we expect");
     ok(chromeArray.constructor === Array, "Array constructor remapped properly");
 
     // We should be able to .forEach on the Array.

js/xpconnect/tests/unit/test_bug780370.js

@@ -16,5 +16,8 @@ function run_test()
   do_check_eq(Cu.evalInSandbox('typeof obj.foo', sb), 'undefined', "COW works as expected");
   do_check_true(Cu.evalInSandbox('obj.hasOwnProperty === Object.prototype.hasOwnProperty', sb),
                 "Remapping happens even when the property is explicitly exposed");
-  do_check_eq(Cu.evalInSandbox('Object.prototype.bar = 10; obj.bar', sb), 10);
+  // NB: We used to test for the following, but such behavior became very
+  // difficult to implement in a recent refactor. We're moving away from this
+  // API anyway, so we decided to explicitly drop support for this.
+  // do_check_eq(Cu.evalInSandbox('Object.prototype.bar = 10; obj.bar', sb), 10);
 }

js/xpconnect/wrappers/ChromeObjectWrapper.cpp

@@ -14,6 +14,16 @@ namespace xpc {
 // their prototype, we have to instrument the traps to do this manually.
 ChromeObjectWrapper ChromeObjectWrapper::singleton;
 
+static bool
+AllowedByBase(JSContext *cx, JSObject *wrapper, jsid id, js::Wrapper::Action act)
+{
+    MOZ_ASSERT(js::Wrapper::wrapperHandler(wrapper) ==
+               &ChromeObjectWrapper::singleton);
+    bool bp;
+    ChromeObjectWrapper *handler = &ChromeObjectWrapper::singleton;
+    return handler->ChromeObjectWrapperBase::enter(cx, wrapper, id, act, &bp);
+}
+
 static bool
 PropIsFromStandardPrototype(JSContext *cx, JSPropertyDescriptor *desc)
 {
@@ -23,16 +33,35 @@ PropIsFromStandardPrototype(JSContext *cx, JSPropertyDescriptor *desc)
     return JS_IdentifyClassPrototype(cx, unwrapped) != JSProto_Null;
 }
 
+// Note that we're past the policy enforcement stage, here, so we can query
+// ChromeObjectWrapperBase and get an unfiltered view of the underlying object.
+// This lets us determine whether the property we would have found (given a
+// transparent wrapper) would have come off a standard prototype.
+static bool
+PropIsFromStandardPrototype(JSContext *cx, JSObject *wrapper, jsid id)
+{
+    MOZ_ASSERT(js::Wrapper::wrapperHandler(wrapper) ==
+               &ChromeObjectWrapper::singleton);
+    JSPropertyDescriptor desc;
+    ChromeObjectWrapper *handler = &ChromeObjectWrapper::singleton;
+    if (!handler->ChromeObjectWrapperBase::getPropertyDescriptor(cx, wrapper, id,
+                                                                 &desc, 0) ||
+        !desc.obj)
+    {
+        return false;
+    }
+    return PropIsFromStandardPrototype(cx, &desc);
+}
+
 bool
 ChromeObjectWrapper::getPropertyDescriptor(JSContext *cx, JSObject *wrapper,
                                            jsid id, js::PropertyDescriptor *desc,
                                            unsigned flags)
 {
-    // First, try the lookup on the base wrapper. This can throw for various
+    // First, try a lookup on the base wrapper if permitted.
-    // reasons, including sets (gets fail silently). There's nothing we can really
-    // do for sets, so we can conveniently propagate any exception we hit here.
     desc->obj = NULL;
-    if (!ChromeObjectWrapperBase::getPropertyDescriptor(cx, wrapper, id,
+    if (AllowedByBase(cx, wrapper, id, Wrapper::GET) &&
+        !ChromeObjectWrapperBase::getPropertyDescriptor(cx, wrapper, id,
                                                         desc, flags)) {
         return false;
     }
@@ -58,9 +87,12 @@ ChromeObjectWrapper::getPropertyDescriptor(JSContext *cx, JSObject *wrapper,
 bool
 ChromeObjectWrapper::has(JSContext *cx, JSObject *wrapper, jsid id, bool *bp)
 {
-    // Try the lookup on the base wrapper.
+    // Try the lookup on the base wrapper if permitted.
-    if (!ChromeObjectWrapperBase::has(cx, wrapper, id, bp))
+    if (AllowedByBase(cx, wrapper, id, js::Wrapper::GET) &&
+        !ChromeObjectWrapperBase::has(cx, wrapper, id, bp))
+    {
         return false;
+    }
 
     // If we found something or have no prototype, we're done.
     JSObject *wrapperProto;
@@ -82,19 +114,14 @@ bool
 ChromeObjectWrapper::get(JSContext *cx, JSObject *wrapper, JSObject *receiver,
                          jsid id, js::Value *vp)
 {
-    // Start with a call to getPropertyDescriptor. We unfortunately need to do
-    // this because the call signature of ::get doesn't give us any way to
-    // determine the object upon which the property was found.
-    JSPropertyDescriptor desc;
-    if (!ChromeObjectWrapperBase::getPropertyDescriptor(cx, wrapper, id, &desc,
-                                                        0)) {
-        return false;
-    }
-
-    // Only call through to the get trap on the underlying object if we'll find
-    // something, and if what we'll find is not on a standard prototype.
     vp->setUndefined();
-    if (desc.obj && !PropIsFromStandardPrototype(cx, &desc)) {
+    JSPropertyDescriptor desc;
+    // Only call through to the get trap on the underlying object if we're
+    // allowed to see the property, and if what we'll find is not on a standard
+    // prototype.
+    if (AllowedByBase(cx, wrapper, id, js::Wrapper::GET) &&
+        !PropIsFromStandardPrototype(cx, wrapper, id))
+    {
         // Call the get trap.
         if (!ChromeObjectWrapperBase::get(cx, wrapper, receiver, id, vp))
             return false;
@@ -115,4 +142,20 @@ ChromeObjectWrapper::get(JSContext *cx, JSObject *wrapper, JSObject *receiver,
     return js::GetGeneric(cx, wrapperProto, receiver, id, vp);
 }
 
+// This mechanism isn't ideal because we end up calling enter() on the base class
+// twice (once during enter() here and once during the trap itself), and policy
+// enforcement or COWs isn't cheap. But it results in the cleanest code, and this
+// whole proto remapping thing for COWs is going to be phased out anyway.
+bool
+ChromeObjectWrapper::enter(JSContext *cx, JSObject *wrapper, jsid id,
+                           js::Wrapper::Action act, bool *bp)
+{
+    if (AllowedByBase(cx, wrapper, id, act))
+        return true;
+    // COWs fail silently for GETs, and that also happens to be the only case
+    // where we might want to redirect the lookup to the home prototype chain.
+    *bp = (act == Wrapper::GET);
+    return *bp && PropIsFromStandardPrototype(cx, wrapper, id);
+}
+
 }

js/xpconnect/wrappers/ChromeObjectWrapper.h

@@ -37,6 +37,9 @@ class ChromeObjectWrapper : public ChromeObjectWrapperBase
     virtual bool get(JSContext *cx, JSObject *wrapper, JSObject *receiver,
                      jsid id, js::Value *vp) MOZ_OVERRIDE;
 
+    virtual bool enter(JSContext *cx, JSObject *wrapper, jsid id,
+                       js::Wrapper::Action act, bool *bp) MOZ_OVERRIDE;
+
     // NB: One might think we'd need to implement enumerate(), keys(), iterate(),
     // and getPropertyNames() here. However, ES5 built-in properties aren't
     // enumerable (and SpiderMonkey's implementation seems to match the spec

js/xpconnect/wrappers/XrayWrapper.cpp

@@ -983,12 +983,8 @@ XPCWrappedNativeXrayTraits::resolveOwnProperty(JSContext *cx, js::Wrapper &jsWra
            id == rt->GetStringID(XPCJSRuntime::IDX_NODEPRINCIPAL)) &&
           Is<nsINode>(wrapper)) ||
           (id == rt->GetStringID(XPCJSRuntime::IDX_DOCUMENTURIOBJECT) &&
-          Is<nsIDocument>(wrapper)))) {
+          Is<nsIDocument>(wrapper))))
-        bool status;
+    {
-        desc->obj = NULL; // default value
-        if (!jsWrapper.enter(cx, wrapper, id, Wrapper::GET, &status))
-            return status;
-
         desc->obj = wrapper;
         desc->attrs = JSPROP_ENUMERATE|JSPROP_SHARED;
         if (id == rt->GetStringID(XPCJSRuntime::IDX_BASEURIOBJECT))
@@ -1420,11 +1416,6 @@ XrayWrapper<Base, Traits>::getPropertyDescriptor(JSContext *cx, JSObject *wrappe
         return true;
     }
 
-    bool status;
-    desc->obj = NULL; // default value
-    if (!this->enter(cx, wrapper, id, Wrapper::GET, &status))
-        return status;
-
     typename Traits::ResolvingIdImpl resolving(wrapper, id);
 
     // Redirect access straight to the wrapper if we should be transparent.
@@ -1450,11 +1441,6 @@ XrayWrapper<Base, Traits>::getPropertyDescriptor(JSContext *cx, JSObject *wrappe
     XPCJSRuntime* rt = nsXPConnect::GetRuntimeInstance();
     if (AccessCheck::wrapperSubsumes(wrapper) &&
         id == rt->GetStringID(XPCJSRuntime::IDX_WRAPPED_JSOBJECT)) {
-        bool status;
-        desc->obj = NULL; // default value
-        if (!this->enter(cx, wrapper, id, Wrapper::GET, &status))
-            return status;
-
         desc->obj = wrapper;
         desc->attrs = JSPROP_ENUMERATE|JSPROP_SHARED;
         desc->getter = wrappedJSObject_getter;
@@ -1548,11 +1534,6 @@ XrayWrapper<Base, Traits>::getOwnPropertyDescriptor(JSContext *cx, JSObject *wra
         return true;
     }
 
-    bool status;
-    desc->obj = NULL; // default value
-    if (!this->enter(cx, wrapper, id, Wrapper::GET, &status))
-        return status;
-
     typename Traits::ResolvingIdImpl resolving(wrapper, id);
 
     // NB: Nothing we do here acts on the wrapped native itself, so we don't