Bug 957688 - Remove side-effect-free calls to js::CheckAccess. r=mrbkap
authorBobby Holley <bobbyholley@gmail.com>
Fri, 24 Jan 2014 16:08:24 -0800
changeset 165195 048746b43ad37d5106fe258caf4c118231b9ca8c
parent 165194 08b7f4b5b665a4e4c4c83c7b96d22d0e712efbff
child 165196 4e3c42f192f4cd2b3965e91ec7b970c8e06d9f7f
push id26082
push userphilringnalda@gmail.com
push dateSun, 26 Jan 2014 03:55:40 +0000
treeherdermozilla-central@ce31e8b8cbbd [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmrbkap
bugs957688
milestone29.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 957688 - Remove side-effect-free calls to js::CheckAccess. r=mrbkap js::CheckAccess has all sorts of crazy side-effects on its parameters. Luckily, they mostly happen on dead values. We have to alter a jit-test that previously threw, and doesn't anymore. I have confirmed that the reason for throwing was not the security check itself, but rather the lookupGeneric call that happens inside js::CheckAccess, which ends up throwing 'undefined is not a function'. It seems like this is just an issue of calling lookupGeneric when we shouldn't, and that the correct behavior here is not to throw.
js/src/builtin/Object.cpp
js/src/jit-test/tests/baseline/bug881461.js
js/src/jsobj.cpp
js/src/vm/Interpreter.cpp
js/src/vm/OldDebugAPI.cpp
--- a/js/src/builtin/Object.cpp
+++ b/js/src/builtin/Object.cpp
@@ -567,21 +567,16 @@ obj_watch(JSContext *cx, unsigned argc, 
     RootedObject callable(cx, ValueToCallable(cx, args[1], args.length() - 2));
     if (!callable)
         return false;
 
     RootedId propid(cx);
     if (!ValueToId<CanGC>(cx, args[0], &propid))
         return false;
 
-    RootedValue tmp(cx);
-    unsigned attrs;
-    if (!CheckAccess(cx, obj, propid, JSACC_WATCH, &tmp, &attrs))
-        return false;
-
     if (!JSObject::watch(cx, obj, propid, callable))
         return false;
 
     args.rval().setUndefined();
     return true;
 }
 
 static bool
--- a/js/src/jit-test/tests/baseline/bug881461.js
+++ b/js/src/jit-test/tests/baseline/bug881461.js
@@ -1,3 +1,2 @@
-// |jit-test| error: TypeError
 z = Proxy.create({}, (function(){}));
 ({__proto__: z, set c(a) {}});
--- a/js/src/jsobj.cpp
+++ b/js/src/jsobj.cpp
@@ -578,25 +578,16 @@ DefinePropertyOnObject(JSContext *cx, Ha
             RootedValue v(cx, desc.hasValue() ? desc.value() : UndefinedValue());
             return baseops::DefineGeneric(cx, obj, id, v,
                                           JS_PropertyStub, JS_StrictPropertyStub,
                                           desc.attributes());
         }
 
         JS_ASSERT(desc.isAccessorDescriptor());
 
-        /*
-         * Getters and setters are just like watchpoints from an access
-         * control point of view.
-         */
-        RootedValue dummy(cx);
-        unsigned dummyAttrs;
-        if (!CheckAccess(cx, obj, id, JSACC_WATCH, &dummy, &dummyAttrs))
-            return false;
-
         RootedValue tmp(cx, UndefinedValue());
         return baseops::DefineGeneric(cx, obj, id, tmp,
                                       desc.getter(), desc.setter(), desc.attributes());
     }
 
     /* 8.12.9 steps 5-6 (note 5 is merely a special case of 6). */
     RootedValue v(cx, UndefinedValue());
 
@@ -810,24 +801,16 @@ DefinePropertyOnObject(JSContext *cx, Ha
         if (desc.hasValue())
             v = desc.value();
         attrs = (desc.attributes() & ~unchanged) | (shapeAttributes & unchanged);
         getter = JS_PropertyStub;
         setter = JS_StrictPropertyStub;
     } else {
         JS_ASSERT(desc.isAccessorDescriptor());
 
-        /*
-         * Getters and setters are just like watchpoints from an access
-         * control point of view.
-         */
-        RootedValue dummy(cx);
-        if (!CheckAccess(cx, obj2, id, JSACC_WATCH, &dummy, &attrs))
-             return false;
-
         /* 8.12.9 step 12. */
         unsigned changed = 0;
         if (desc.hasConfigurable())
             changed |= JSPROP_PERMANENT;
         if (desc.hasEnumerable())
             changed |= JSPROP_ENUMERATE;
         if (desc.hasGet())
             changed |= JSPROP_GETTER | JSPROP_SHARED | JSPROP_READONLY;
@@ -5248,25 +5231,16 @@ baseops::DeleteSpecial(JSContext *cx, Ha
     Rooted<jsid> id(cx, SPECIALID_TO_JSID(sid));
     return baseops::DeleteGeneric(cx, obj, id, succeeded);
 }
 
 bool
 js::WatchGuts(JSContext *cx, JS::HandleObject origObj, JS::HandleId id, JS::HandleObject callable)
 {
     RootedObject obj(cx, GetInnerObject(cx, origObj));
-    if (origObj != obj) {
-        // If by unwrapping and innerizing, we changed the object, check again
-        // to make sure that we're allowed to set a watch point.
-        RootedValue v(cx);
-        unsigned attrs;
-        if (!CheckAccess(cx, obj, id, JSACC_WATCH, &v, &attrs))
-            return false;
-    }
-
     if (obj->isNative()) {
         // Use sparse indexes for watched objects, as dense elements can be
         // written to without checking the watchpoint map.
         if (!JSObject::sparsifyDenseElements(cx, obj))
             return false;
 
         types::MarkTypePropertyNonData(cx, obj, id);
     }
--- a/js/src/vm/Interpreter.cpp
+++ b/js/src/vm/Interpreter.cpp
@@ -3912,44 +3912,34 @@ js::RunOnceScriptPrologue(JSContext *cx,
     return true;
 }
 
 bool
 js::InitGetterSetterOperation(JSContext *cx, jsbytecode *pc, HandleObject obj, HandleId id,
                               HandleObject val)
 {
     JS_ASSERT(val->isCallable());
-
-    /*
-     * Getters and setters are just like watchpoints from an access control
-     * point of view.
-     */
-    RootedValue scratch(cx, UndefinedValue());
-    unsigned attrs = 0;
-    if (!CheckAccess(cx, obj, id, JSACC_WATCH, &scratch, &attrs))
-        return false;
-
     PropertyOp getter;
     StrictPropertyOp setter;
-    attrs = JSPROP_ENUMERATE | JSPROP_SHARED;
+    unsigned attrs = JSPROP_ENUMERATE | JSPROP_SHARED;
 
     JSOp op = JSOp(*pc);
 
     if (op == JSOP_INITPROP_GETTER || op == JSOP_INITELEM_GETTER) {
         getter = CastAsPropertyOp(val);
         setter = JS_StrictPropertyStub;
         attrs |= JSPROP_GETTER;
     } else {
         JS_ASSERT(op == JSOP_INITPROP_SETTER || op == JSOP_INITELEM_SETTER);
         getter = JS_PropertyStub;
         setter = CastAsStrictPropertyOp(val);
         attrs |= JSPROP_SETTER;
     }
 
-    scratch.setUndefined();
+    RootedValue scratch(cx);
     return JSObject::defineGeneric(cx, obj, id, scratch, getter, setter, attrs);
 }
 
 bool
 js::InitGetterSetterOperation(JSContext *cx, jsbytecode *pc, HandleObject obj,
                               HandlePropertyName name, HandleObject val)
 {
     RootedId id(cx, NameToId(name));
--- a/js/src/vm/OldDebugAPI.cpp
+++ b/js/src/vm/OldDebugAPI.cpp
@@ -300,39 +300,29 @@ JS_SetWatchPoint(JSContext *cx, JSObject
     assertSameCompartment(cx, obj_);
 
     RootedId id(cx, id_);
     RootedObject origobj(cx, obj_), closure(cx, closure_);
     RootedObject obj(cx, GetInnerObject(cx, origobj));
     if (!obj)
         return false;
 
-    RootedValue v(cx);
-    unsigned attrs;
-
     RootedId propid(cx);
 
     if (JSID_IS_INT(id)) {
         propid = id;
     } else if (JSID_IS_OBJECT(id)) {
         JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_CANT_WATCH_PROP);
         return false;
     } else {
         RootedValue val(cx, IdToValue(id));
         if (!ValueToId<CanGC>(cx, val, &propid))
             return false;
     }
 
-    /*
-     * If, by unwrapping and innerizing, we changed the object, check
-     * again to make sure that we're allowed to set a watch point.
-     */
-    if (origobj != obj && !CheckAccess(cx, obj, propid, JSACC_WATCH, &v, &attrs))
-        return false;
-
     if (!obj->isNative()) {
         JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_CANT_WATCH,
                              obj->getClass()->name);
         return false;
     }
 
     /*
      * Use sparse indexes for watched objects, as dense elements can be written