From a735d8f183cd0b718add41ac26defcfb62e537d3 Mon Sep 17 00:00:00 2001 From: Jason Orendorff Date: Fri, 13 Feb 2015 09:49:31 -0600 Subject: [PATCH] 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 | 1 + .../tests/proxy/testDirectProxySetFailure.js | 40 ++++++++++++ js/src/js.msg | 1 + js/src/proxy/ScriptedDirectProxyHandler.cpp | 62 ++++++++----------- js/src/tests/ecma_6/Array/fill.js | 2 +- js/src/tests/ecma_6/Array/from_proxy.js | 1 + js/src/tests/ecma_6/TypedArray/from_proxy.js | 1 + js/src/vm/Xdr.h | 4 +- 8 files changed, 73 insertions(+), 39 deletions(-) create mode 100644 js/src/jit-test/tests/proxy/testDirectProxySetFailure.js diff --git a/browser/devtools/shared/observable-object.js b/browser/devtools/shared/observable-object.js index 099f1f235f92..9c8d768f841d 100644 --- a/browser/devtools/shared/observable-object.js +++ b/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); diff --git a/js/src/jit-test/tests/proxy/testDirectProxySetFailure.js b/js/src/jit-test/tests/proxy/testDirectProxySetFailure.js new file mode 100644 index 000000000000..c4715f9c35a7 --- /dev/null +++ b/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"]); diff --git a/js/src/js.msg b/js/src/js.msg index 0749ca8d26c7..14a89e2da0fb 100644 --- a/js/src/js.msg +++ b/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") diff --git a/js/src/proxy/ScriptedDirectProxyHandler.cpp b/js/src/proxy/ScriptedDirectProxyHandler.cpp index 8bea8085b61f..919803a32219 100644 --- a/js/src/proxy/ScriptedDirectProxyHandler.cpp +++ b/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().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 desc(cx); - if (!GetOwnPropertyDescriptor(cx, target, id, &desc)) - return false; + // step 12-13 + Rooted 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 diff --git a/js/src/tests/ecma_6/Array/fill.js b/js/src/tests/ecma_6/Array/fill.js index ad94e6632983..70f1e0b52228 100644 --- a/js/src/tests/ecma_6/Array/fill.js +++ b/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); diff --git a/js/src/tests/ecma_6/Array/from_proxy.js b/js/src/tests/ecma_6/Array/from_proxy.js index 124052703770..c3c82d0b7e48 100644 --- a/js/src/tests/ecma_6/Array/from_proxy.js +++ b/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); diff --git a/js/src/tests/ecma_6/TypedArray/from_proxy.js b/js/src/tests/ecma_6/TypedArray/from_proxy.js index cebd0f209b81..c08c1ee4a223 100644 --- a/js/src/tests/ecma_6/TypedArray/from_proxy.js +++ b/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); diff --git a/js/src/vm/Xdr.h b/js/src/vm/Xdr.h index 2a2e3528f1d7..742e6319fa6c 100644 --- a/js/src/vm/Xdr.h +++ b/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 "