Bug 596031 - 'this' is wrong in getters and setters when a proxy object is on the prototype chain. r=brendan/jorendorff/gal

--HG--
extra : transplant_source : %9A%16%D6%DF%A3%C4%3F%08%B0-%19%B2%91%0Ee%1B%CC%95%DB%A4

js/src/jsarray.cpp

@@ -688,7 +688,7 @@ js_GetDenseArrayElementValue(JSContext *cx, JSObject *obj, jsid id, Value *vp)
 }
 
 static JSBool
-array_getProperty(JSContext *cx, JSObject *obj, jsid id, Value *vp)
+array_getProperty(JSContext *cx, JSObject *obj, JSObject *receiver, jsid id, Value *vp)
 {
     uint32 i;
 

js/src/jscntxtinlines.h

@@ -738,6 +738,21 @@ CallJSPropertyOpSetter(JSContext *cx, js::PropertyOp op, JSObject *obj, jsid id,
     return op(cx, obj, id, vp);
 }
 
+inline bool
+CallSetter(JSContext *cx, JSObject *obj, jsid id, PropertyOp op, uintN attrs, uintN shortid,
+           js::Value *vp)
+{
+    if (attrs & JSPROP_SETTER)
+        return ExternalGetOrSet(cx, obj, id, CastAsObjectJsval(op), JSACC_WRITE, 1, vp, vp);
+
+    if (attrs & JSPROP_GETTER)
+        return js_ReportGetterOnlyAssignment(cx);
+
+    if (attrs & JSPROP_SHORTID)
+        id = INT_TO_JSID(shortid);
+    return CallJSPropertyOpSetter(cx, op, obj, id, vp);
+}
+
 }  /* namespace js */
 
 #endif /* jscntxtinlines_h___ */

js/src/jsobj.cpp

@@ -3023,7 +3023,7 @@ with_LookupProperty(JSContext *cx, JSObject *obj, jsid id, JSObject **objp,
 }
 
 static JSBool
-with_GetProperty(JSContext *cx, JSObject *obj, jsid id, Value *vp)
+with_GetProperty(JSContext *cx, JSObject *obj, JSObject *receiver, jsid id, Value *vp)
 {
     return obj->getProto()->getProperty(cx, id, vp);
 }
@@ -4933,7 +4933,7 @@ js_NativeSet(JSContext *cx, JSObject *obj, const Shape *shape, bool added, Value
 }
 
 static JS_ALWAYS_INLINE bool
-js_GetPropertyHelperWithShapeInline(JSContext *cx, JSObject *obj, jsid id,
+js_GetPropertyHelperWithShapeInline(JSContext *cx, JSObject *obj, JSObject *receiver, jsid id,
                                     uintN getHow, Value *vp,
                                     const Shape **shapeOut, JSObject **holderOut)
 {
@@ -5019,8 +5019,11 @@ js_GetPropertyHelperWithShapeInline(JSContext *cx, JSObject *obj, jsid id,
         return JS_TRUE;
     }
 
-    if (!obj2->isNative())
-        return obj2->getProperty(cx, id, vp);
+    if (!obj2->isNative()) {
+        return obj2->isProxy()
+               ? JSProxy::get(cx, obj2, receiver, id, vp)
+               : obj2->getProperty(cx, id, vp);
+    }
 
     shape = (Shape *) prop;
     *shapeOut = shape;
@@ -5031,39 +5034,41 @@ js_GetPropertyHelperWithShapeInline(JSContext *cx, JSObject *obj, jsid id,
     }
 
     /* This call site is hot -- use the always-inlined variant of js_NativeGet(). */
-    if (!js_NativeGetInline(cx, obj, obj2, shape, getHow, vp))
+    if (!js_NativeGetInline(cx, receiver, obj2, shape, getHow, vp))
         return JS_FALSE;
 
     return JS_TRUE;
 }
 
-extern bool
-js_GetPropertyHelperWithShape(JSContext *cx, JSObject *obj, jsid id,
+bool
+js_GetPropertyHelperWithShape(JSContext *cx, JSObject *obj, JSObject *receiver, jsid id,
                               uint32 getHow, Value *vp,
                               const Shape **shapeOut, JSObject **holderOut)
 {
-    return js_GetPropertyHelperWithShapeInline(cx, obj, id, getHow, vp, shapeOut, holderOut);
+    return js_GetPropertyHelperWithShapeInline(cx, obj, receiver, id, getHow, vp,
+                                               shapeOut, holderOut);
 }
 
 static JS_ALWAYS_INLINE JSBool
-js_GetPropertyHelperInline(JSContext *cx, JSObject *obj, jsid id, uint32 getHow, Value *vp)
+js_GetPropertyHelperInline(JSContext *cx, JSObject *obj, JSObject *receiver, jsid id,
+                           uint32 getHow, Value *vp)
 {
     const Shape *shape;
     JSObject *holder;
-    return js_GetPropertyHelperWithShapeInline(cx, obj, id, getHow, vp, &shape, &holder);
-}
-
-extern JSBool
-js_GetPropertyHelper(JSContext *cx, JSObject *obj, jsid id, uint32 getHow, Value *vp)
-{
-    return js_GetPropertyHelperInline(cx, obj, id, getHow, vp);
+    return js_GetPropertyHelperWithShapeInline(cx, obj, receiver, id, getHow, vp, &shape, &holder);
 }
 
 JSBool
-js_GetProperty(JSContext *cx, JSObject *obj, jsid id, Value *vp)
+js_GetPropertyHelper(JSContext *cx, JSObject *obj, jsid id, uint32 getHow, Value *vp)
+{
+    return js_GetPropertyHelperInline(cx, obj, obj, id, getHow, vp);
+}
+
+JSBool
+js_GetProperty(JSContext *cx, JSObject *obj, JSObject *receiver, jsid id, Value *vp)
 {
     /* This call site is hot -- use the always-inlined variant of js_GetPropertyHelper(). */
-    return js_GetPropertyHelperInline(cx, obj, id, JSGET_METHOD_BARRIER, vp);
+    return js_GetPropertyHelperInline(cx, obj, receiver, id, JSGET_METHOD_BARRIER, vp);
 }
 
 JSBool
@@ -5099,7 +5104,7 @@ js_GetMethod(JSContext *cx, JSObject *obj, jsid id, uintN getHow, Value *vp)
     if (obj->isXML())
         return js_GetXMLMethod(cx, obj, id, vp);
 #endif
-    return op(cx, obj, id, vp);
+    return op(cx, obj, obj, id, vp);
 }
 
 JS_FRIEND_API(bool)
@@ -5180,8 +5185,26 @@ js_SetPropertyHelper(JSContext *cx, JSObject *obj, jsid id, uintN defineHow,
     if (protoIndex < 0)
         return JS_FALSE;
     if (prop) {
-        if (!pobj->isNative())
+        if (!pobj->isNative()) {
+            if (pobj->isProxy()) {
+                AutoPropertyDescriptorRooter pd(cx);
+                if (!pobj->getProxyHandler()->getPropertyDescriptor(cx, pobj, id, true, &pd))
+                    return false;
+
+                if (pd.attrs & JSPROP_SHARED)
+                    return CallSetter(cx, obj, id, pd.setter, pd.attrs, pd.shortid, vp);
+
+                if (pd.attrs & JSPROP_READONLY) {
+                    if (strict)
+                        return obj->reportReadOnly(cx, id);
+                    if (JS_HAS_STRICT_OPTION(cx))
+                        return obj->reportReadOnly(cx, id, JSREPORT_STRICT | JSREPORT_WARNING);
+                    return true;
+                }
+            }
+
             prop = NULL;
+        }
     } else {
         /* We should never add properties to lexical blocks.  */
         JS_ASSERT(!obj->isBlock());

js/src/jsobj.h

@@ -209,7 +209,13 @@ js_DefineProperty(JSContext *cx, JSObject *obj, jsid id, const js::Value *value,
                   js::PropertyOp getter, js::PropertyOp setter, uintN attrs);
 
 extern JSBool
-js_GetProperty(JSContext *cx, JSObject *obj, jsid id, js::Value *vp);
+js_GetProperty(JSContext *cx, JSObject *obj, JSObject *receiver, jsid id, js::Value *vp);
+
+inline JSBool
+js_GetProperty(JSContext *cx, JSObject *obj, jsid id, js::Value *vp)
+{
+    return js_GetProperty(cx, obj, obj, id, vp);
+}
 
 namespace js {
 
@@ -1064,9 +1070,13 @@ struct JSObject : js::gc::Cell {
         return (op ? op : js_DefineProperty)(cx, this, id, &value, getter, setter, attrs);
     }
 
-    JSBool getProperty(JSContext *cx, jsid id, js::Value *vp) {
+    JSBool getProperty(JSContext *cx, JSObject *receiver, jsid id, js::Value *vp) {
         js::PropertyIdOp op = getOps()->getProperty;
-        return (op ? op : js_GetProperty)(cx, this, id, vp);
+        return (op ? op : (js::PropertyIdOp)js_GetProperty)(cx, this, receiver, id, vp);
+    }
+
+    JSBool getProperty(JSContext *cx, jsid id, js::Value *vp) {
+        return getProperty(cx, this, id, vp);
     }
 
     JSBool setProperty(JSContext *cx, jsid id, js::Value *vp, JSBool strict) {
@@ -1085,7 +1095,7 @@ struct JSObject : js::gc::Cell {
     }
 
     JSBool deleteProperty(JSContext *cx, jsid id, js::Value *rval, JSBool strict) {
-        js::StrictPropertyIdOp op = getOps()->deleteProperty;
+        js::DeleteIdOp op = getOps()->deleteProperty;
         return (op ? op : js_DeleteProperty)(cx, this, id, rval, strict);
     }
 
@@ -1569,8 +1579,9 @@ extern JSBool
 js_GetPropertyHelper(JSContext *cx, JSObject *obj, jsid id, uint32 getHow, js::Value *vp);
 
 extern bool
-js_GetPropertyHelperWithShape(JSContext *cx, JSObject *obj, jsid id, uint32 getHow,
-                              js::Value *vp, const js::Shape **shapeOut, JSObject **holderOut);
+js_GetPropertyHelperWithShape(JSContext *cx, JSObject *obj, JSObject *receiver, jsid id,
+                              uint32 getHow, js::Value *vp,
+                              const js::Shape **shapeOut, JSObject **holderOut);
 
 extern JSBool
 js_GetOwnPropertyDescriptor(JSContext *cx, JSObject *obj, jsid id, js::Value *vp);

js/src/jsproxy.cpp

@@ -127,7 +127,7 @@ JSProxyHandler::get(JSContext *cx, JSObject *proxy, JSObject *receiver, jsid id,
         return true;
     }
     if (desc.attrs & JSPROP_GETTER) {
-        return ExternalGetOrSet(cx, proxy, id, CastAsObjectJsval(desc.getter),
+        return ExternalGetOrSet(cx, receiver, id, CastAsObjectJsval(desc.getter),
                                 JSACC_READ, 0, NULL, vp);
     }
     if (!(desc.attrs & JSPROP_SHARED))
@@ -136,7 +136,7 @@ JSProxyHandler::get(JSContext *cx, JSObject *proxy, JSObject *receiver, jsid id,
         vp->setUndefined();
     if (desc.attrs & JSPROP_SHORTID)
         id = INT_TO_JSID(desc.shortid);
-    return CallJSPropertyOp(cx, desc.getter, proxy, id, vp);
+    return CallJSPropertyOp(cx, desc.getter, receiver, id, vp);
 }
 
 bool
@@ -148,43 +148,29 @@ JSProxyHandler::set(JSContext *cx, JSObject *proxy, JSObject *receiver, jsid id,
         return false;
     /* The control-flow here differs from ::get() because of the fall-through case below. */
     if (desc.obj) {
-        if (desc.setter && ((desc.attrs & JSPROP_SETTER) || desc.setter != PropertyStub)) {
-            if (desc.attrs & JSPROP_SETTER) {
-                return ExternalGetOrSet(cx, proxy, id, CastAsObjectJsval(desc.setter),
-                                        JSACC_WRITE, 1, vp, vp);
-            }
-            if (desc.attrs & JSPROP_SHORTID)
-                id = INT_TO_JSID(desc.shortid);
-            return CallJSPropertyOpSetter(cx, desc.setter, proxy, id, vp);
-        }
+        if (desc.setter && ((desc.attrs & JSPROP_SETTER) || desc.setter != PropertyStub))
+            return CallSetter(cx, receiver, id, desc.setter, desc.attrs, desc.shortid, vp);
         if (desc.attrs & JSPROP_READONLY)
             return true;
         desc.value = *vp;
-        return defineProperty(cx, proxy, id, &desc);
+        return defineProperty(cx, receiver, id, &desc);
     }
     if (!getPropertyDescriptor(cx, proxy, id, true, &desc))
         return false;
     if (desc.obj) {
-        if (desc.setter && ((desc.attrs & JSPROP_SETTER) || desc.setter != PropertyStub)) {
-            if (desc.attrs & JSPROP_SETTER) {
-                return ExternalGetOrSet(cx, proxy, id, CastAsObjectJsval(desc.setter),
-                                        JSACC_WRITE, 1, vp, vp);
-            }
-            if (desc.attrs & JSPROP_SHORTID)
-                id = INT_TO_JSID(desc.shortid);
-            return CallJSPropertyOpSetter(cx, desc.setter, proxy, id, vp);
-        }
+        if (desc.setter && ((desc.attrs & JSPROP_SETTER) || desc.setter != PropertyStub))
+            return CallSetter(cx, receiver, id, desc.setter, desc.attrs, desc.shortid, vp);
         if (desc.attrs & JSPROP_READONLY)
             return true;
         /* fall through */
     }
-    desc.obj = proxy;
+    desc.obj = receiver;
     desc.value = *vp;
     desc.attrs = JSPROP_ENUMERATE;
     desc.getter = NULL;
     desc.setter = NULL;
     desc.shortid = 0;
-    return defineProperty(cx, proxy, id, &desc);
+    return defineProperty(cx, receiver, id, &desc);
 }
 
 bool
@@ -862,15 +848,15 @@ proxy_DefineProperty(JSContext *cx, JSObject *obj, jsid id, const Value *value,
 }
 
 static JSBool
-proxy_GetProperty(JSContext *cx, JSObject *obj, jsid id, Value *vp)
+proxy_GetProperty(JSContext *cx, JSObject *obj, JSObject *receiver, jsid id, Value *vp)
 {
-    return JSProxy::get(cx, obj, obj, id, vp);
+    return JSProxy::get(cx, obj, receiver, id, vp);
 }
 
 static JSBool
 proxy_SetProperty(JSContext *cx, JSObject *obj, jsid id, Value *vp, JSBool strict)
 {
-    // TODO: throwing away strict
+    // FIXME (bug 596351): throwing away strict.
     return JSProxy::set(cx, obj, obj, id, vp);
 }
 

js/src/jstracer.cpp

@@ -12032,7 +12032,7 @@ GetPropertyByName(JSContext* cx, JSObject* obj, JSString** namep, Value* vp, PIC
     /* Delegate to the op, if present. */
     PropertyIdOp op = obj->getOps()->getProperty;
     if (op) {
-        bool result = op(cx, obj, id, vp);
+        bool result = op(cx, obj, obj, id, vp);
         if (!result)
             SetBuiltinError(cx);
         return cx->tracerState->builtinStatus == 0;
@@ -12047,7 +12047,8 @@ GetPropertyByName(JSContext* cx, JSObject* obj, JSString** namep, Value* vp, PIC
 
     const Shape *shape;
     JSObject *holder;
-    if (!js_GetPropertyHelperWithShape(cx, obj, id, JSGET_METHOD_BARRIER, vp, &shape, &holder)) {
+    if (!js_GetPropertyHelperWithShape(cx, obj, obj, id, JSGET_METHOD_BARRIER, vp, &shape,
+                                       &holder)) {
         SetBuiltinError(cx);
         return false;
     }

js/src/jstypedarray.cpp

@@ -493,7 +493,7 @@ class TypedArrayTemplate
     }
 
     static JSBool
-    obj_getProperty(JSContext *cx, JSObject *obj, jsid id, Value *vp)
+    obj_getProperty(JSContext *cx, JSObject *obj, JSObject *receiver, jsid id, Value *vp)
     {
         ThisTypeArray *tarray = ThisTypeArray::fromJSObject(obj);
         JS_ASSERT(tarray);

js/src/jsvalue.h

@@ -898,10 +898,12 @@ typedef JSBool
 (* DefinePropOp)(JSContext *cx, JSObject *obj, jsid id, const Value *value,
                  PropertyOp getter, PropertyOp setter, uintN attrs);
 typedef JSBool
-(* PropertyIdOp)(JSContext *cx, JSObject *obj, jsid id, Value *vp);
+(* PropertyIdOp)(JSContext *cx, JSObject *obj, JSObject *receiver, jsid id, Value *vp);
 typedef JSBool
 (* StrictPropertyIdOp)(JSContext *cx, JSObject *obj, jsid id, Value *vp, JSBool strict);
 typedef JSBool
+(* DeleteIdOp)(JSContext *cx, JSObject *obj, jsid id, Value *vp, JSBool strict);
+typedef JSBool
 (* CallOp)(JSContext *cx, uintN argc, Value *vp);
 typedef JSBool
 (* LookupPropOp)(JSContext *cx, JSObject *obj, jsid id, JSObject **objp,
@@ -999,7 +1001,7 @@ struct ObjectOps {
     js::StrictPropertyIdOp  setProperty;
     js::AttributesOp        getAttributes;
     js::AttributesOp        setAttributes;
-    js::StrictPropertyIdOp  deleteProperty;
+    js::DeleteIdOp          deleteProperty;
     js::NewEnumerateOp      enumerate;
     js::TypeOfOp            typeOf;
     js::TraceOp             trace;

js/src/jswrapper.cpp

@@ -204,13 +204,14 @@ JSWrapper::hasOwn(JSContext *cx, JSObject *wrapper, jsid id, bool *bp)
 bool
 JSWrapper::get(JSContext *cx, JSObject *wrapper, JSObject *receiver, jsid id, Value *vp)
 {
-    GET(JS_GetPropertyById(cx, wrappedObject(wrapper), id, Jsvalify(vp)));
+    GET(wrappedObject(wrapper)->getProperty(cx, receiver, id, vp));
 }
 
 bool
 JSWrapper::set(JSContext *cx, JSObject *wrapper, JSObject *receiver, jsid id, Value *vp)
 {
-    SET(JS_SetPropertyById(cx, wrappedObject(wrapper), id, Jsvalify(vp)));
+    // FIXME (bug 596351): Need deal with strict mode.
+    SET(wrappedObject(wrapper)->setProperty(cx, id, vp, false));
 }
 
 bool

js/src/jsxml.cpp

@@ -4743,7 +4743,7 @@ xml_defineProperty(JSContext *cx, JSObject *obj, jsid id, const Value *v,
 }
 
 static JSBool
-xml_getProperty(JSContext *cx, JSObject *obj, jsid id, Value *vp)
+xml_getProperty(JSContext *cx, JSObject *obj, JSObject *receiver, jsid id, Value *vp)
 {
     if (JSID_IS_DEFAULT_XML_NAMESPACE(id)) {
         vp->setUndefined();

js/src/tests/js1_8_5/extensions/jstests.list

@@ -12,6 +12,7 @@ script scripted-proxies.js
 script array-length-protochange.js
 script parseInt-octal.js
 script proxy-enumerateOwn-duplicates.js
+skip-if(!xulRuntime.shell) script proxy-proto-setter.js
 skip-if(!xulRuntime.shell) script reflect-parse.js
 script destructure-accessor.js
 script censor-strict-caller.js

js/src/tests/js1_8_5/extensions/proxy-proto-setter.js

@@ -0,0 +1,49 @@
+// Any copyright is dedicated to the Public Domain.
+// http://creativecommons.org/licenses/publicdomain/
+// Contributor: Blake Kaplan
+
+function expect(actual, arg) {
+    reportCompare(expect.expected, actual, arg);
+}
+
+var window = { set x(y) { expect(this, y) }, y: 4 };
+expect.expected = window;
+window.x = "setting through a setter directly";
+window.y = 5;
+reportCompare(5, window.y, "setting properties works");
+var easy = { easy: 'yes', __proto__: window }
+expect.expected = easy;
+easy.x = "setting through a setter all-native on prototype";
+easy.y = 6;
+reportCompare(5, window.y, "window.y remains as it was");
+reportCompare(6, easy.y, "shadowing works properly");
+
+var sandbox = evalcx('');
+sandbox.window = window;
+sandbox.print = print;
+sandbox.expect = expect;
+var hard = evalcx('Object.create(window)', sandbox);
+expect.expected = hard;
+hard.x = "a setter through proxy -> native";
+hard.y = 6;
+reportCompare(5, window.y, "window.y remains as it was through a proxy");
+reportCompare(6, hard.y, "shadowing works on proxies");
+
+hard.__proto__ = { 'passed': true }
+reportCompare(true, hard.passed, "can set proxy.__proto__ through a native");
+
+var inverse = evalcx('({ set x(y) { expect(this, y); }, y: 4 })', sandbox);
+expect.expected = inverse;
+inverse.x = "setting through a proxy directly";
+inverse.y = 5;
+reportCompare(5, inverse.y, "setting properties works on proxies");
+
+var inversehard = Object.create(inverse);
+expect.expected = inversehard;
+inversehard.x = "setting through a setter with a proxy on the proto chain";
+inversehard.y = 6;
+reportCompare(5, inverse.y, "inverse.y remains as it was");
+reportCompare(6, inversehard.y, "shadowing works native -> proxy");
+
+inversehard.__proto__ = { 'passed': true }
+reportCompare(true, inversehard.passed, "can set native.__proto__ through a proxy");

js/src/tests/js1_8_5/regress/jstests.list

@@ -12,7 +12,7 @@ script regress-551763-0.js
 script regress-551763-1.js
 script regress-551763-2.js
 script regress-555246-0.js
-fails-if(xulRuntime.shell) script regress-555246-1.js
+script regress-555246-1.js
 script regress-559438.js
 script regress-560101.js
 script regress-560998-1.js
@@ -48,3 +48,4 @@ script regress-598176.js
 script regress-600067.js
 script regress-600137.js
 script regress-602621.js
+fails-if(!xulRuntime.shell) script regress-607863.js

js/src/tests/js1_8_5/regress/regress-607863.js

@@ -0,0 +1,13 @@
+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/*
+ * Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/licenses/publicdomain/
+ */
+
+var sandbox = evalcx('');
+var foreign = evalcx('({ get f() this, set x(v) { result = this } })', sandbox);
+var local = Object.create(foreign);
+
+reportCompare(local, local.f, "this should be set correctly in getters");
+local.x = 42;
+reportCompare(local, sandbox.result, "this should be set correctly in setters");