Bug 1132522, part 2 - Treat false return from proxyHandler.set() as strict mode failure. r=efaust.

--HG--
extra : rebase_source : 438b6070a1770bc17cc455a457df2b033bbc972d

browser/devtools/shared/observable-object.js

@@ -91,6 +91,7 @@ Handler.prototype = {
     let [wrapped, path] = this.unwrap(target, key, value);
     target[key] = value;
     this._emitter.emit("set", path, value);
+    return true;
   },
   getOwnPropertyDescriptor: function(target, key) {
     let desc = Object.getOwnPropertyDescriptor(target, key);

js/src/jit-test/tests/proxy/testDirectProxySetFailure.js

@@ -0,0 +1,40 @@
+// Test handling of false return from a handler.set() hook.
+
+load(libdir + "asserts.js");
+
+var obj = {x: 1};
+var p = new Proxy(obj, {
+    set(target, key, value, receiver) { return false; }
+});
+
+// Failing to set a property is a no-op in non-strict code.
+assertEq(p.x = 2, 2);
+assertEq(obj.x, 1);
+
+// It's a TypeError in strict mode code.
+assertThrowsInstanceOf(() => { "use strict"; p.x = 2; }, TypeError);
+assertEq(obj.x, 1);
+
+// Even if the value doesn't change.
+assertThrowsInstanceOf(() => { "use strict"; p.x = 1; }, TypeError);
+assertEq(obj.x, 1);
+
+// Even if the target property doesn't already exist.
+assertThrowsInstanceOf(() => { "use strict"; p.z = 1; }, TypeError);
+assertEq("z" in obj, false);
+
+// [].sort() mutates its operand only by doing strict [[Set]] calls.
+var arr = ["not", "already", "in", "order"];
+var p2 = new Proxy(arr, {
+    get(target, key, receiver) {
+        if (key === "length" && new Proxy([1], {}).length === 1) {
+            throw new Error("Congratulations! You have fixed bug 895223 at last! " +
+                            "Please delete this whole get() method which is a " +
+                            "workaround for that bug.");
+        }
+        return target[key];
+    },
+    set(target, key, value, receiver) { return false; }
+});
+assertThrowsInstanceOf(() => p2.sort(), TypeError);
+assertDeepEq(arr, ["not", "already", "in", "order"]);

js/src/js.msg

@@ -345,6 +345,7 @@ MSG_DEF(JSMSG_CANT_DEFINE_NE_AS_NC,    0, JSEXN_TYPEERR, "proxy can't define a n
 MSG_DEF(JSMSG_PROXY_DEFINE_RETURNED_FALSE, 1, JSEXN_TYPEERR, "proxy defineProperty handler returned false for property '{0}'")
 MSG_DEF(JSMSG_PROXY_DELETE_RETURNED_FALSE, 1, JSEXN_TYPEERR, "can't delete property '{0}': proxy deleteProperty handler returned false")
 MSG_DEF(JSMSG_PROXY_PREVENTEXTENSIONS_RETURNED_FALSE, 0, JSEXN_TYPEERR, "proxy preventExtensions handler returned false")
+MSG_DEF(JSMSG_PROXY_SET_RETURNED_FALSE, 1, JSEXN_TYPEERR, "proxy set handler returned false for property '{0}'")
 MSG_DEF(JSMSG_CANT_REPORT_AS_NON_EXTENSIBLE, 0, JSEXN_TYPEERR, "proxy can't report an extensible object as non-extensible")
 MSG_DEF(JSMSG_CANT_REPORT_C_AS_NC,     0, JSEXN_TYPEERR, "proxy can't report existing configurable property as non-configurable")
 MSG_DEF(JSMSG_CANT_REPORT_E_AS_NE,     0, JSEXN_TYPEERR, "proxy can't report an existing own property as non-existent on a non-extensible object")

js/src/proxy/ScriptedDirectProxyHandler.cpp

@@ -925,33 +925,29 @@ ScriptedDirectProxyHandler::get(JSContext *cx, HandleObject proxy, HandleObject
     return true;
 }
 
-// ES6 (22 May, 2014) 9.5.9 Proxy.[[SetP]](P, V, Receiver)
+// ES6 draft rev 32 (2015 Feb 2) 9.5.9 Proxy.[[Set]](P, V, Receiver)
 bool
 ScriptedDirectProxyHandler::set(JSContext *cx, HandleObject proxy, HandleObject receiver,
                                 HandleId id, MutableHandleValue vp, ObjectOpResult &result) const
 {
-    // step 2
+    // step 2-3 (Steps 1 and 4 are irrelevant assertions.)
     RootedObject handler(cx, GetDirectProxyHandlerObject(proxy));
-
-    // step 3
     if (!handler) {
         JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_PROXY_REVOKED);
         return false;
     }
 
-    // step 4
+    // step 5-7
     RootedObject target(cx, proxy->as<ProxyObject>().target());
-
-    // step 5-6
     RootedValue trap(cx);
     if (!GetProperty(cx, handler, handler, cx->names().set, &trap))
         return false;
 
-    // step 7
+    // step 8
     if (trap.isUndefined())
         return DirectProxyHandler::set(cx, proxy, receiver, id, vp, result);
 
-    // step 8,10
+    // step 9-10
     RootedValue value(cx);
     if (!IdToStringOrSymbol(cx, id, &value))
         return false;
@@ -965,43 +961,37 @@ ScriptedDirectProxyHandler::set(JSContext *cx, HandleObject proxy, HandleObject
     if (!Invoke(cx, ObjectValue(*handler), trap, ArrayLength(argv), argv, &trapResult))
         return false;
 
-    // FIXME - bug 1132522: Step 9 is not implemented yet.
-    // if (!ToBoolean(trapResult))
-    //     return result.fail(JSMSG_PROXY_SET_RETURNED_FALSE);
-    bool success = ToBoolean(trapResult);
+    // step 11
+    if (!ToBoolean(trapResult))
+        return result.fail(JSMSG_PROXY_SET_RETURNED_FALSE);
 
-    if (success) {
-        // step 12-13
-        Rooted<PropertyDescriptor> desc(cx);
-        if (!GetOwnPropertyDescriptor(cx, target, id, &desc))
-            return false;
+    // step 12-13
+    Rooted<PropertyDescriptor> desc(cx);
+    if (!GetOwnPropertyDescriptor(cx, target, id, &desc))
+        return false;
 
-        // step 14
-        if (desc.object()) {
-            if (IsDataDescriptor(desc) && desc.isPermanent() && desc.isReadonly()) {
-                bool same;
-                if (!SameValue(cx, vp, desc.value(), &same))
-                    return false;
-                if (!same) {
-                    JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_CANT_SET_NW_NC);
-                    return false;
-                }
-            }
-
-            if (IsAccessorDescriptor(desc) && desc.isPermanent() && !desc.hasSetterObject()) {
-                JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_CANT_SET_WO_SETTER);
+    // step 14
+    if (desc.object()) {
+        if (IsDataDescriptor(desc) && desc.isPermanent() && desc.isReadonly()) {
+            bool same;
+            if (!SameValue(cx, vp, desc.value(), &same))
+                return false;
+            if (!same) {
+                JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_CANT_SET_NW_NC);
                 return false;
             }
         }
+
+        if (IsAccessorDescriptor(desc) && desc.isPermanent() && !desc.hasSetterObject()) {
+            JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_CANT_SET_WO_SETTER);
+            return false;
+        }
     }
 
-    // step 11, 15
-    // XXX FIXME - This use of vp is wrong. Bug 1132522 can clean it up.
-    vp.setBoolean(success);
+    // step 15
     return result.succeed();
 }
 
-
 // ES6 (22 May, 2014) 9.5.13 Proxy.[[Call]]
 bool
 ScriptedDirectProxyHandler::call(JSContext *cx, HandleObject proxy, const CallArgs &args) const

js/src/tests/ecma_6/Array/fill.js

@@ -59,7 +59,7 @@ var objWithSetter = {set "0"(val) { setterCalled = true}, length: 1};
 assertEq(setterCalled, true);
 
 var setHandlerCallCount = 0;
-var proxy = new Proxy({length: 3}, {set: function(value) {setHandlerCallCount++;}});
+var proxy = new Proxy({length: 3}, {set(t, i, v, r) { setHandlerCallCount++; return true; }});
 [].fill.call(proxy, 2);
 assertEq(setHandlerCallCount, 3);
 

js/src/tests/ecma_6/Array/from_proxy.js

@@ -20,6 +20,7 @@ function LoggingProxy(target) {
         set: function (t, id, v) {
             log.push("set", id);
             t[id] = v;
+            return true;
         }
     };
     return new Proxy(target || [], h);

js/src/tests/ecma_6/TypedArray/from_proxy.js

@@ -31,6 +31,7 @@ for (var constructor of constructors) {
             set: function (t, id, v) {
                 log.push("set", id);
                 t[id] = v;
+                return true;
             }
         };
         return new Proxy(Object(target), h);

js/src/vm/Xdr.h

@@ -29,11 +29,11 @@ namespace js {
  *
  *  https://developer.mozilla.org/en-US/docs/SpiderMonkey/Internals/Bytecode
  */
-static const uint32_t XDR_BYTECODE_VERSION_SUBTRAHEND = 246;
+static const uint32_t XDR_BYTECODE_VERSION_SUBTRAHEND = 247;
 static const uint32_t XDR_BYTECODE_VERSION =
     uint32_t(0xb973c0de - XDR_BYTECODE_VERSION_SUBTRAHEND);
 
-static_assert(JSErr_Limit == 385,
+static_assert(JSErr_Limit == 386,
               "GREETINGS, POTENTIAL SUBTRAHEND INCREMENTER! If you added or "
               "removed MSG_DEFs from js.msg, you should increment "
               "XDR_BYTECODE_VERSION_SUBTRAHEND and update this assertion's "