Bug 836301 - Stop using JSRESOLVE_ASSIGNING to determine GET vs SET. r=mrbkap
authorBobby Holley <bobbyholley@gmail.com>
Fri, 22 Feb 2013 08:14:32 -0800
changeset 122662 81db941b0b769cdbb57580e5f17b1cecda78fb79
parent 122661 e0632e63909741eb1617fc46bce9a67ec4aa6724
child 122663 741efe957058b5f15da8fd649036abab6f065f5e
push id24356
push usergszorc@mozilla.com
push dateSun, 24 Feb 2013 01:00:12 +0000
treeherdermozilla-central@195e706140d1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmrbkap
bugs836301
milestone22.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 836301 - Stop using JSRESOLVE_ASSIGNING to determine GET vs SET. r=mrbkap This is just a heuristic, anyway, and some of the usage is downright broken. There are two cases here: 1 - Deciding what to do for get{Own,}PropertyDescriptor. In these cases, we can just enter with GET and rely on the filtering machinery to filter out dangerous setters for security wrappers. 2 - Custom Xray props. None of these make sense in a |set| context. In fact, they generally have null setters anyway, so we can just assume GET. The policy-entering code in XrayWrapper is super haphazard. We'll get rid of it entirely later in these patches.
js/src/jswrapper.cpp
js/xpconnect/wrappers/XrayWrapper.cpp
--- a/js/src/jswrapper.cpp
+++ b/js/src/jswrapper.cpp
@@ -146,18 +146,17 @@ Wrapper::~Wrapper()
 
 bool
 Wrapper::getPropertyDescriptor(JSContext *cx, JSObject *wrapperArg,
                                jsid id, PropertyDescriptor *desc, unsigned flags)
 {
     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),
-            (flags & JSRESOLVE_ASSIGNING) ? SET : GET);
+    CHECKED(DirectProxyHandler::getPropertyDescriptor(cx, wrapper, id, desc, flags), GET);
 }
 
 bool
 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
--- a/js/xpconnect/wrappers/XrayWrapper.cpp
+++ b/js/xpconnect/wrappers/XrayWrapper.cpp
@@ -721,18 +721,17 @@ XPCWrappedNativeXrayTraits::preserveWrap
 
 bool
 XPCWrappedNativeXrayTraits::resolveNativeProperty(JSContext *cx, JSObject *wrapper,
                                                   JSObject *holder, jsid id,
                                                   JSPropertyDescriptor *desc, unsigned flags)
 {
     MOZ_ASSERT(js::GetObjectJSClass(holder) == &HolderClass);
     XPCJSRuntime* rt = nsXPConnect::GetRuntimeInstance();
-    if (!(flags & JSRESOLVE_ASSIGNING) &&
-        id == rt->GetStringID(XPCJSRuntime::IDX_MOZMATCHESSELECTOR) &&
+    if (id == rt->GetStringID(XPCJSRuntime::IDX_MOZMATCHESSELECTOR) &&
         Is<nsIDOMElement>(wrapper))
     {
         // XPC calling mechanism cannot handle call/bind properly in some cases
         // especially through xray wrappers. This is a temporary work around for
         // this problem for mozMatchesSelector. See: bug 763897.
         desc->obj = wrapper;
         desc->attrs = JSPROP_ENUMERATE;
         JSObject *proto;
@@ -981,19 +980,18 @@ XPCWrappedNativeXrayTraits::resolveOwnPr
     XPCJSRuntime* rt = nsXPConnect::GetRuntimeInstance();
     if (AccessCheck::isChrome(wrapper) &&
         (((id == rt->GetStringID(XPCJSRuntime::IDX_BASEURIOBJECT) ||
            id == rt->GetStringID(XPCJSRuntime::IDX_NODEPRINCIPAL)) &&
           Is<nsINode>(wrapper)) ||
           (id == rt->GetStringID(XPCJSRuntime::IDX_DOCUMENTURIOBJECT) &&
           Is<nsIDocument>(wrapper)))) {
         bool status;
-        Wrapper::Action action = (flags & JSRESOLVE_ASSIGNING) ? Wrapper::SET : Wrapper::GET;
         desc->obj = NULL; // default value
-        if (!jsWrapper.enter(cx, wrapper, id, action, &status))
+        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))
             desc->getter = baseURIObject_getter;
         else if (id == rt->GetStringID(XPCJSRuntime::IDX_DOCUMENTURIOBJECT))
             desc->getter = documentURIObject_getter;
@@ -1405,19 +1403,18 @@ XrayWrapper<Base, Traits>::getPropertyDe
 {
     JSObject *holder = Traits::singleton.ensureHolder(cx, wrapper);
     if (Traits::isResolving(cx, holder, id)) {
         desc->obj = NULL;
         return true;
     }
 
     bool status;
-    Wrapper::Action action = (flags & JSRESOLVE_ASSIGNING) ? Wrapper::SET : Wrapper::GET;
     desc->obj = NULL; // default value
-    if (!this->enter(cx, wrapper, id, action, &status))
+    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.
     if (XrayUtils::IsTransparent(cx, wrapper, id)) {
         JSObject *obj = Traits::getTargetObject(wrapper);
         {
@@ -1436,19 +1433,18 @@ XrayWrapper<Base, Traits>::getPropertyDe
 
     // Only chrome wrappers and same-origin xrays (used by jetpack sandboxes)
     // get .wrappedJSObject. We can check this by determining if the compartment
     // of the wrapper subsumes that of the wrappee.
     XPCJSRuntime* rt = nsXPConnect::GetRuntimeInstance();
     if (AccessCheck::wrapperSubsumes(wrapper) &&
         id == rt->GetStringID(XPCJSRuntime::IDX_WRAPPED_JSOBJECT)) {
         bool status;
-        Wrapper::Action action = (flags & JSRESOLVE_ASSIGNING) ? Wrapper::SET : Wrapper::GET;
         desc->obj = NULL; // default value
-        if (!this->enter(cx, wrapper, id, action, &status))
+        if (!this->enter(cx, wrapper, id, Wrapper::GET, &status))
             return status;
 
         desc->obj = wrapper;
         desc->attrs = JSPROP_ENUMERATE|JSPROP_SHARED;
         desc->getter = wrappedJSObject_getter;
         desc->setter = NULL;
         desc->shortid = 0;
         desc->value = JSVAL_VOID;
@@ -1535,19 +1531,18 @@ XrayWrapper<Base, Traits>::getOwnPropert
 {
     JSObject *holder = Traits::singleton.ensureHolder(cx, wrapper);
     if (Traits::isResolving(cx, holder, id)) {
         desc->obj = NULL;
         return true;
     }
 
     bool status;
-    Wrapper::Action action = (flags & JSRESOLVE_ASSIGNING) ? Wrapper::SET : Wrapper::GET;
     desc->obj = NULL; // default value
-    if (!this->enter(cx, wrapper, id, action, &status))
+    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
     // enter our policy.
     // Redirect access straight to the wrapper if we should be transparent.
     if (XrayUtils::IsTransparent(cx, wrapper, id)) {
@@ -1591,16 +1586,18 @@ XrayWrapper<Base, Traits>::definePropert
         JSAutoCompartment ac(cx, obj);
         if (!JS_WrapPropertyDescriptor(cx, desc))
             return false;
 
         return JS_DefinePropertyById(cx, obj, id, desc->value, desc->getter, desc->setter,
                                      desc->attrs);
     }
 
+    // NB: We still need JSRESOLVE_ASSIGNING here for the time being, because it
+    // tells things like nodelists whether they should create the property or not.
     PropertyDescriptor existing_desc;
     if (!getOwnPropertyDescriptor(cx, wrapper, id, &existing_desc, JSRESOLVE_ASSIGNING))
         return false;
 
     if (existing_desc.obj && (existing_desc.attrs & JSPROP_PERMANENT))
         return true; // silently ignore attempt to overwrite native property
 
     bool defined = false;