From 59d3c6c36d41b1ca1a3723ea80c060067458628d Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Mon, 25 Feb 2013 13:54:18 -0800 Subject: [PATCH] Bug 836301 - Hoist enter() calls from {Xray,}Wrapper::foo into Proxy::foo. r=mrbkap --- js/src/jsproxy.cpp | 141 +++++++++++++++--- js/src/jswrapper.cpp | 80 ++-------- js/xpconnect/tests/chrome/test_bug760109.xul | 13 +- js/xpconnect/tests/unit/test_bug780370.js | 5 +- js/xpconnect/wrappers/ChromeObjectWrapper.cpp | 79 +++++++--- js/xpconnect/wrappers/ChromeObjectWrapper.h | 3 + js/xpconnect/wrappers/XrayWrapper.cpp | 23 +-- 7 files changed, 216 insertions(+), 128 deletions(-) diff --git a/js/src/jsproxy.cpp b/js/src/jsproxy.cpp index ebdc57ba827c..6998383c7a42 100644 --- a/js/src/jsproxy.cpp +++ b/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()) - return GetProxyHandler(proxy)->iterate(cx, proxy, flags, vp.address()); + vp.setUndefined(); // default result if we refuse to perform this action + 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 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 diff --git a/js/src/jswrapper.cpp b/js/src/jswrapper.cpp index 0d4899e8d3f8..819b64a8b608 100644 --- a/js/src/jswrapper.cpp +++ b/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 - CHECKED(DirectProxyHandler::getPropertyDescriptor(cx, wrapper, id, desc, flags), GET); + return DirectProxyHandler::getPropertyDescriptor(cx, wrapper, id, desc, flags); } 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 - CHECKED(DirectProxyHandler::getOwnPropertyDescriptor(cx, wrapper, id, desc, flags), GET); + return DirectProxyHandler::getOwnPropertyDescriptor(cx, wrapper, id, desc, flags); } 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 - jsid id = JSID_VOID; - GET(DirectProxyHandler::getOwnPropertyNames(cx, wrapper, props)); + return 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 - SET(DirectProxyHandler::delete_(cx, wrapper, id, bp)); + return 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 - static jsid id = JSID_VOID; - GET(DirectProxyHandler::enumerate(cx, wrapper, props)); + return 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 - GET(DirectProxyHandler::has(cx, wrapper, id, bp)); + return 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 - GET(DirectProxyHandler::hasOwn(cx, wrapper, id, bp)); + return 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 - GET(DirectProxyHandler::get(cx, wrapper, receiver, id, vp)); + return 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 - const jsid id = JSID_VOID; - GET(DirectProxyHandler::keys(cx, wrapper, props)); + return 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 - const jsid id = JSID_VOID; - GET(DirectProxyHandler::iterate(cx, wrapper, flags, vp)); + return 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 - const jsid id = JSID_VOID; - CHECKED(DirectProxyHandler::call(cx, wrapper, argc, vp), CALL); + return DirectProxyHandler::call(cx, wrapper, argc, vp); } 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 - const jsid id = JSID_VOID; - CHECKED(DirectProxyHandler::construct(cx, wrapper, argc, argv, vp), CALL); + return DirectProxyHandler::construct(cx, wrapper, argc, argv, vp); } 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 - const jsid id = JSID_VOID; - GET(DirectProxyHandler::hasInstance(cx, wrapper, v, bp)); + return 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; } diff --git a/js/xpconnect/tests/chrome/test_bug760109.xul b/js/xpconnect/tests/chrome/test_bug760109.xul index 311ccb1e6ed0..aa26158d5101 100644 --- a/js/xpconnect/tests/chrome/test_bug760109.xul +++ b/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 - // object with a custom chrome-implemented prototype, because we'll end up - // bouncing up the prototype chain to Object, even though that's not fully - // accurate. It's unlikely to be a problem though, so we just verify that - // it does what we expect. - ok(chromeObject.constructor === Object, "Object constructor does what we expect"); + // Check |constructor|. + // Note that the 'constructor' property of the underlying chrome object + // will be resolved on SomeConstructor.prototype, which has an empty + // __exposedProps__. This means that we shouldn't remap the property, even + // though we'd also be able to find it on Object.prototype. Some recent + // 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. diff --git a/js/xpconnect/tests/unit/test_bug780370.js b/js/xpconnect/tests/unit/test_bug780370.js index 1e335d51a452..027f13418898 100644 --- a/js/xpconnect/tests/unit/test_bug780370.js +++ b/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); } diff --git a/js/xpconnect/wrappers/ChromeObjectWrapper.cpp b/js/xpconnect/wrappers/ChromeObjectWrapper.cpp index f2f6c79e6aff..5868089d5d72 100644 --- a/js/xpconnect/wrappers/ChromeObjectWrapper.cpp +++ b/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 - // 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. + // First, try a lookup on the base wrapper if permitted. 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. - if (!ChromeObjectWrapperBase::has(cx, wrapper, id, bp)) + // Try the lookup on the base wrapper if permitted. + 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); +} + } diff --git a/js/xpconnect/wrappers/ChromeObjectWrapper.h b/js/xpconnect/wrappers/ChromeObjectWrapper.h index b5e0b7e9708e..1a98f3a9026f 100644 --- a/js/xpconnect/wrappers/ChromeObjectWrapper.h +++ b/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 diff --git a/js/xpconnect/wrappers/XrayWrapper.cpp b/js/xpconnect/wrappers/XrayWrapper.cpp index e7b442873db5..84dcab856fce 100644 --- a/js/xpconnect/wrappers/XrayWrapper.cpp +++ b/js/xpconnect/wrappers/XrayWrapper.cpp @@ -983,12 +983,8 @@ XPCWrappedNativeXrayTraits::resolveOwnProperty(JSContext *cx, js::Wrapper &jsWra id == rt->GetStringID(XPCJSRuntime::IDX_NODEPRINCIPAL)) && Is(wrapper)) || (id == rt->GetStringID(XPCJSRuntime::IDX_DOCUMENTURIOBJECT) && - Is(wrapper)))) { - bool status; - desc->obj = NULL; // default value - if (!jsWrapper.enter(cx, wrapper, id, Wrapper::GET, &status)) - return status; - + Is(wrapper)))) + { desc->obj = wrapper; desc->attrs = JSPROP_ENUMERATE|JSPROP_SHARED; if (id == rt->GetStringID(XPCJSRuntime::IDX_BASEURIOBJECT)) @@ -1420,11 +1416,6 @@ XrayWrapper::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::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::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