Bug 1132522, part 1 - Treat false return from proxyHandler.defineProperty() as strict mode failure. r=efaust.
authorJason Orendorff <jorendorff@mozilla.com>
Thu, 12 Feb 2015 11:29:32 -0600
changeset 261954 f2a7b760ad5c0fc6bdf8d06f7ed97410f9d977c7
parent 261953 18c84fcee487097abe547d7a5cccf641ee489270
child 261955 911612d952e61b18a04c22032c89ff9ef1923107
push id4718
push userraliiev@mozilla.com
push dateMon, 11 May 2015 18:39:53 +0000
treeherdermozilla-beta@c20c4ef55f08 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersefaust
bugs1132522
milestone39.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 1132522, part 1 - Treat false return from proxyHandler.defineProperty() as strict mode failure. r=efaust.
browser/devtools/shared/observable-object.js
js/src/jit-test/tests/collections/Array-of-ordering.js
js/src/jit-test/tests/proxy/testDirectProxyDefineProperty2.js
js/src/jit-test/tests/proxy/testDirectProxyDefinePropertyFailure.js
js/src/jit-test/tests/proxy/testDirectProxySet7.js
js/src/jit-test/tests/proxy/testDirectProxySet8.js
js/src/jit-test/tests/proxy/testDirectProxySet9.js
js/src/jit-test/tests/proxy/testDirectProxySetArray1.js
js/src/jit-test/tests/proxy/testDirectProxySetArray3.js
js/src/jit-test/tests/proxy/testDirectProxySetArray4.js
js/src/jit-test/tests/proxy/testIndirectProxySet.js
js/src/js.msg
js/src/proxy/ScriptedDirectProxyHandler.cpp
js/src/tests/ecma_6/Array/from_proxy.js
js/src/tests/ecma_6/TypedArray/from_proxy.js
js/src/vm/Xdr.h
--- a/browser/devtools/shared/observable-object.js
+++ b/browser/devtools/shared/observable-object.js
@@ -120,10 +120,11 @@ Handler.prototype = {
       if ("get" in desc) {
         [desc.get] = this.unwrap(target, "get "+key, desc.get);
       }
       if ("set" in desc) {
         [desc.set] = this.unwrap(target, "set "+key, desc.set);
       }
       Object.defineProperty(target, key, desc);
     }
+    return true;
   }
 };
--- a/js/src/jit-test/tests/collections/Array-of-ordering.js
+++ b/js/src/jit-test/tests/collections/Array-of-ordering.js
@@ -3,19 +3,21 @@
 load(libdir + "asserts.js");
 
 var log;
 
 var dstdata = [];
 var dst = new Proxy(dstdata, {
     defineProperty: function (t, name, desc) {
         log.push(["def", name, desc.value]);
+        return true;
     },
     set: function (t, name, value) {
         log.push(["set", name, value]);
+        return true;
     }
 });
 
 function Troop() {
     return dst;
 }
 Troop.of = Array.of;
 
--- a/js/src/jit-test/tests/proxy/testDirectProxyDefineProperty2.js
+++ b/js/src/jit-test/tests/proxy/testDirectProxyDefineProperty2.js
@@ -9,16 +9,17 @@ var handler = {
         assertEq(this, handler);
         assertEq(target1, target);
         log.push(key);
         assertEq(desc1 == desc, false);
         assertEq(desc1.value, 'bar');
         assertEq(desc1.writable, true);
         assertEq(desc1.enumerable, false);
         assertEq(desc1.configurable, true);
+        return true;
     }
 };
 var desc = {
     value: 'bar',
     writable: true,
     enumerable: false,
     configurable: true
 };
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/proxy/testDirectProxyDefinePropertyFailure.js
@@ -0,0 +1,26 @@
+// Test handling of false return from a handler.defineProperty() hook.
+
+load(libdir + "asserts.js");
+
+var obj = {x: 1, y: 2};
+var nope = false;
+var p = new Proxy(obj, {
+    defineProperty(target, key, desc) { return nope; }
+});
+
+// Object.defineProperty throws on failure.
+print(1);
+assertThrowsInstanceOf(() => Object.defineProperty(p, "z", {value: 3}), TypeError);
+assertEq("z" in obj, false);
+assertThrowsInstanceOf(() => Object.defineProperty(p, "x", {value: 0}), TypeError);
+
+// Setting a property ultimately causes [[DefineOwnProperty]] to be called.
+// In strict mode code only, this is a TypeError.
+print(2);
+assertEq(p.z = 3, 3);
+assertThrowsInstanceOf(() => { "use strict"; p.z = 3; }, TypeError);
+
+// Other falsy values also trigger failure.
+print(3);
+for (nope of [0, -0, NaN, ""])
+    assertThrowsInstanceOf(() => { "use strict"; p.z = 3; }, TypeError);
--- a/js/src/jit-test/tests/proxy/testDirectProxySet7.js
+++ b/js/src/jit-test/tests/proxy/testDirectProxySet7.js
@@ -1,15 +1,15 @@
 // Assigning to a proxy with no set handler calls the defineProperty handler
 // when no such property already exists.
 
 var hits = 0;
 var t = {};
 var p = new Proxy(t, {
-    defineProperty(t, id, desc) { hits++; }
+    defineProperty(t, id, desc) { hits++; return true; }
 });
 p.x = 1;
 assertEq(hits, 1);
 assertEq("x" in t, false);
 
 // Same thing, but the receiver is a plain object inheriting from p:
 var receiver = Object.create(p);
 hits = 0;
--- a/js/src/jit-test/tests/proxy/testDirectProxySet8.js
+++ b/js/src/jit-test/tests/proxy/testDirectProxySet8.js
@@ -1,16 +1,16 @@
 // Assigning to a proxy with no set handler calls the defineProperty handler
 // when an existing inherited data property already exists.
 
 var hits = 0;
 var proto = {x: 1};
 var t = Object.create(proto);
 var p = new Proxy(t, {
-    defineProperty(t, id, desc) { hits++; }
+    defineProperty(t, id, desc) { hits++; return true; }
 });
 p.x = 2;
 assertEq(hits, 1);
 assertEq(proto.x, 1);
 assertEq(t.hasOwnProperty("x"), false);
 
 // Same thing, but the receiver is a plain object inheriting from p:
 var receiver = Object.create(p);
--- a/js/src/jit-test/tests/proxy/testDirectProxySet9.js
+++ b/js/src/jit-test/tests/proxy/testDirectProxySet9.js
@@ -6,14 +6,15 @@ var p = new Proxy(t, {
     defineProperty(t, id, desc) {
         hits++;
 
         // ES6 draft rev 28 (2014 Oct 14) 9.1.9 step 5.e.i.
         // Since the property already exists, the system only changes
         // the value. desc is otherwise empty.
         assertEq(Object.getOwnPropertyNames(desc).join(","), "value");
         assertEq(desc.value, 42);
+        return true;
     }
 });
 var hits = 0;
 p.x = 42;
 assertEq(hits, 1);
 assertEq(t.x, 1);
--- a/js/src/jit-test/tests/proxy/testDirectProxySetArray1.js
+++ b/js/src/jit-test/tests/proxy/testDirectProxySetArray1.js
@@ -5,16 +5,17 @@ function test(id) {
     var arr = [, 1, 2, 3];
     var p = new Proxy(arr, {
         defineProperty(t, id, desc) {
             hits++;
             assertEq(desc.value, "ponies");
             assertEq(desc.enumerable, true);
             assertEq(desc.configurable, true);
             assertEq(desc.writable, true);
+            return true;
         }
     });
     var hits = 0;
     p[id] = "ponies";
     assertEq(hits, 1);
     assertEq(id in arr, false);
     assertEq(arr.length, 4);
 }
--- a/js/src/jit-test/tests/proxy/testDirectProxySetArray3.js
+++ b/js/src/jit-test/tests/proxy/testDirectProxySetArray3.js
@@ -6,15 +6,16 @@ var p = new Proxy(a, {
     defineProperty(t, id, desc) {
         hits++;
 
         // ES6 draft rev 28 (2014 Oct 14) 9.1.9 step 5.e.i.
         // Since the property already exists, the system only changes
         // the value. desc is otherwise empty.
         assertEq(Object.getOwnPropertyNames(desc).join(","), "value");
         assertEq(desc.value, 2);
+        return true;
     }
 });
 var hits = 0;
 p.length = 2;
 assertEq(hits, 1);
 assertEq(a.length, 4);
 assertEq(a[2], 2);
--- a/js/src/jit-test/tests/proxy/testDirectProxySetArray4.js
+++ b/js/src/jit-test/tests/proxy/testDirectProxySetArray4.js
@@ -6,16 +6,17 @@ function test(arr) {
         defineProperty(t, id, desc) {
             hits++;
 
             // ES6 draft rev 28 (2014 Oct 14) 9.1.9 step 5.e.i.
             // Since the property already exists, the system only changes
             // the value. desc is otherwise empty.
             assertEq(Object.getOwnPropertyNames(desc).join(","), "value");
             assertEq(desc.value, "ponies");
+            return true;
         }
     });
     var hits = 0;
     p[0] = "ponies";
     assertEq(hits, 1);
     assertEq(arr[0], 123);
 }
 
--- a/js/src/jit-test/tests/proxy/testIndirectProxySet.js
+++ b/js/src/jit-test/tests/proxy/testIndirectProxySet.js
@@ -10,16 +10,17 @@ var q = new Proxy(p, {
     defineProperty(t, id, desc) {
         assertEq(t, p);
         assertEq(id, "x");
         assertEq(desc.configurable, true);
         assertEq(desc.enumerable, true);
         assertEq(desc.writable, true);
         assertEq(desc.value, 3);
         hits++;
+        return true;
     }
 });
 var hits = 0;
 
 // This assignment eventually reaches ScriptedIndirectProxyHandler::set
 // with arguments (proxy=p, receiver=q). Test that it finishes by defining
 // a property on receiver, not on proxy.
 q.x = 3;
--- a/js/src/js.msg
+++ b/js/src/js.msg
@@ -338,16 +338,17 @@ MSG_DEF(JSMSG_USE_ASM_LINK_FAIL,       1
 MSG_DEF(JSMSG_USE_ASM_TYPE_OK,         1, JSEXN_NONE,    "Successfully compiled asm.js code ({0})")
 
 // Proxy
 MSG_DEF(JSMSG_BAD_TRAP_RETURN_VALUE,   2, JSEXN_TYPEERR,"trap {1} for {0} returned a primitive value")
 MSG_DEF(JSMSG_CANT_CHANGE_EXTENSIBILITY, 0, JSEXN_TYPEERR, "can't change object's extensibility")
 MSG_DEF(JSMSG_CANT_DEFINE_INVALID,     0, JSEXN_TYPEERR, "proxy can't define an incompatible property descriptor")
 MSG_DEF(JSMSG_CANT_DEFINE_NEW,         0, JSEXN_TYPEERR, "proxy can't define a new property on a non-extensible object")
 MSG_DEF(JSMSG_CANT_DEFINE_NE_AS_NC,    0, JSEXN_TYPEERR, "proxy can't define a non-existent property as non-configurable")
+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_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")
 MSG_DEF(JSMSG_CANT_REPORT_INVALID,     0, JSEXN_TYPEERR, "proxy can't report an incompatible property descriptor")
 MSG_DEF(JSMSG_CANT_REPORT_NC_AS_NE,    0, JSEXN_TYPEERR, "proxy can't report a non-configurable own property as non-existent")
 MSG_DEF(JSMSG_CANT_REPORT_NEW,         0, JSEXN_TYPEERR, "proxy can't report a new property on a non-extensible object")
--- a/js/src/proxy/ScriptedDirectProxyHandler.cpp
+++ b/js/src/proxy/ScriptedDirectProxyHandler.cpp
@@ -587,19 +587,19 @@ ScriptedDirectProxyHandler::defineProper
         ObjectValue(*target),
         propKey,
         descObj
     };
     RootedValue trapResult(cx);
     if (!Invoke(cx, ObjectValue(*handler), trap, ArrayLength(argv), argv, &trapResult))
         return false;
 
-    // FIXME - bug 1132522: Step 12 is not implemented yet.
-    // if (!ToBoolean(trapResult))
-    //     return result.fail(JSMSG_PROXY_DEFINE_RETURNED_FALSE);
+    // step 12
+    if (!ToBoolean(trapResult))
+        return result.fail(JSMSG_PROXY_DEFINE_RETURNED_FALSE);
 
     // step 13-14
     Rooted<PropertyDescriptor> targetDesc(cx);
     if (!GetOwnPropertyDescriptor(cx, target, id, &targetDesc))
         return false;
 
     // step 15-16
     bool extensibleTarget;
--- a/js/src/tests/ecma_6/Array/from_proxy.js
+++ b/js/src/tests/ecma_6/Array/from_proxy.js
@@ -2,17 +2,17 @@
  * http://creativecommons.org/licenses/publicdomain/ */
 
 // Two tests involving Array.from and a Proxy.
 var log = [];
 function LoggingProxy(target) {
     var h = {
         defineProperty: function (t, id) {
             log.push("define", id);
-            return undefined;
+            return true;
         },
         has: function (t, id) {
             log.push("has", id);
             return id in t;
         },
         get: function (t, id) {
             log.push("get", id);
             return t[id];
--- a/js/src/tests/ecma_6/TypedArray/from_proxy.js
+++ b/js/src/tests/ecma_6/TypedArray/from_proxy.js
@@ -13,17 +13,17 @@ const constructors = [
 for (var constructor of constructors) {
     // Two tests involving %TypedArray%.from and a Proxy.
     var log = [];
     function LoggingProxy(target) {
         log.push("target", target);
         var h = {
             defineProperty: function (t, id) {
                 log.push("define", id);
-                return undefined;
+                return true;
             },
             has: function (t, id) {
                 log.push("has", id);
                 return id in t;
             },
             get: function (t, id) {
                 log.push("get", id);
                 return t[id];
--- a/js/src/vm/Xdr.h
+++ b/js/src/vm/Xdr.h
@@ -24,21 +24,21 @@ namespace js {
  * versions.  If deserialization fails, the data should be invalidated if
  * possible.
  *
  * When you change this, run make_opcode_doc.py and copy the new output into
  * this wiki page:
  *
  *  https://developer.mozilla.org/en-US/docs/SpiderMonkey/Internals/Bytecode
  */
-static const uint32_t XDR_BYTECODE_VERSION_SUBTRAHEND = 255;
+static const uint32_t XDR_BYTECODE_VERSION_SUBTRAHEND = 256;
 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 "
               "expected JSErr_Limit value.");
 
 class XDRBuffer {
   public:
     explicit XDRBuffer(JSContext *cx)