From f59cbf7cd5ff326451d1ec537c5f63de6cd4b0d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Bargull?= Date: Wed, 23 Aug 2017 23:56:40 +0200 Subject: [PATCH] Bug 1389752 - Throw TypeError if [[OwnPropertyKeys]] of scripted proxies contains duplicates. r=till, r=qdot --HG-- extra : rebase_source : 7c31e7c3443d7a1885d89a0688022f68f5034bf0 --- .../testDirectProxyGetOwnPropertyNames4.js | 4 +- .../tests/proxy/testDirectProxyKeys4.js | 4 +- js/src/js.msg | 1 + js/src/proxy/ScriptedProxyHandler.cpp | 65 ++++++++++--------- .../ecma_6/Proxy/ownkeys-trap-duplicates.js | 7 +- js/src/tests/jstests.list | 3 + .../fetch/api/headers/headers-record.html | 48 ++------------ 7 files changed, 53 insertions(+), 79 deletions(-) diff --git a/js/src/jit-test/tests/proxy/testDirectProxyGetOwnPropertyNames4.js b/js/src/jit-test/tests/proxy/testDirectProxyGetOwnPropertyNames4.js index 090a3d741064..d6ebb15b5ed2 100644 --- a/js/src/jit-test/tests/proxy/testDirectProxyGetOwnPropertyNames4.js +++ b/js/src/jit-test/tests/proxy/testDirectProxyGetOwnPropertyNames4.js @@ -1,5 +1,7 @@ load(libdir + "asserts.js"); +// Throw TypeError if trap returns a property key multiple times. + var handler = { ownKeys : () => [ 'foo', 'foo' ] }; for (let p of [new Proxy({}, handler), Proxy.revocable({}, handler).proxy]) - assertDeepEq(Object.getOwnPropertyNames(p), ['foo', 'foo']); + assertThrowsInstanceOf(() => Object.getOwnPropertyNames(p), TypeError); diff --git a/js/src/jit-test/tests/proxy/testDirectProxyKeys4.js b/js/src/jit-test/tests/proxy/testDirectProxyKeys4.js index f20d5368deb3..16ce41a02965 100644 --- a/js/src/jit-test/tests/proxy/testDirectProxyKeys4.js +++ b/js/src/jit-test/tests/proxy/testDirectProxyKeys4.js @@ -1,5 +1,7 @@ load(libdir + "asserts.js"); +// Throw TypeError if trap returns a property key multiple times. + var handler = { ownKeys: () => [ 'foo', 'foo' ] }; for (let p of [new Proxy({}, handler), Proxy.revocable({}, handler).proxy]) - assertDeepEq(Object.keys(p), []); // Properties are not enumerable. + assertThrowsInstanceOf(() => Object.keys(p), TypeError); diff --git a/js/src/js.msg b/js/src/js.msg index 9702fa21605e..4b8f1c39173b 100644 --- a/js/src/js.msg +++ b/js/src/js.msg @@ -420,6 +420,7 @@ MSG_DEF(JSMSG_CANT_SET_NW_NC, 1, JSEXN_TYPEERR, "proxy can't successful MSG_DEF(JSMSG_CANT_SET_WO_SETTER, 1, JSEXN_TYPEERR, "proxy can't succesfully set an accessor property '{0}' without a setter") MSG_DEF(JSMSG_CANT_SKIP_NC, 1, JSEXN_TYPEERR, "proxy can't skip a non-configurable property '{0}'") MSG_DEF(JSMSG_OWNKEYS_STR_SYM, 0, JSEXN_TYPEERR, "proxy [[OwnPropertyKeys]] must return an array with only string and symbol elements") +MSG_DEF(JSMSG_OWNKEYS_DUPLICATE, 1, JSEXN_TYPEERR, "proxy [[OwnPropertyKeys]] can't report property '{0}' more than once") MSG_DEF(JSMSG_MUST_REPORT_SAME_VALUE, 1, JSEXN_TYPEERR, "proxy must report the same value for the non-writable, non-configurable property '{0}'") MSG_DEF(JSMSG_MUST_REPORT_UNDEFINED, 1, JSEXN_TYPEERR, "proxy must report undefined for a non-configurable accessor property '{0}' without a getter") MSG_DEF(JSMSG_PROXY_CONSTRUCT_OBJECT, 0, JSEXN_TYPEERR, "proxy [[Construct]] must return an object") diff --git a/js/src/proxy/ScriptedProxyHandler.cpp b/js/src/proxy/ScriptedProxyHandler.cpp index e643528eabdb..354b900741ca 100644 --- a/js/src/proxy/ScriptedProxyHandler.cpp +++ b/js/src/proxy/ScriptedProxyHandler.cpp @@ -713,7 +713,8 @@ CreateFilteredListFromArrayLike(JSContext* cx, HandleValue v, AutoIdVector& prop } -// ES8 rev 0c1bd3004329336774cbc90de727cd0cf5f11e93 9.5.11 Proxy.[[OwnPropertyKeys]]() +// ES2018 draft rev aab1ea3bd4d03c85d6f4a91503b4169346ab7271 +// 9.5.11 Proxy.[[OwnPropertyKeys]]() bool ScriptedProxyHandler::ownPropertyKeys(JSContext* cx, HandleObject proxy, AutoIdVector& props) const { @@ -748,28 +749,44 @@ ScriptedProxyHandler::ownPropertyKeys(JSContext* cx, HandleObject proxy, AutoIdV if (!CreateFilteredListFromArrayLike(cx, trapResultArray, trapResult)) return false; - // Step 9. + // Steps 9, 18. + Rooted> uncheckedResultKeys(cx, GCHashSet(cx)); + if (!uncheckedResultKeys.init(trapResult.length())) + return false; + + for (size_t i = 0, len = trapResult.length(); i < len; i++) { + MOZ_ASSERT(!JSID_IS_VOID(trapResult[i])); + + auto ptr = uncheckedResultKeys.lookupForAdd(trapResult[i]); + if (ptr) + return js::Throw(cx, trapResult[i], JSMSG_OWNKEYS_DUPLICATE); + + if (!uncheckedResultKeys.add(ptr, trapResult[i])) + return false; + } + + // Step 10. bool extensibleTarget; if (!IsExtensible(cx, target, &extensibleTarget)) return false; - // Steps 10-11. + // Steps 11-13. AutoIdVector targetKeys(cx); if (!GetPropertyKeys(cx, target, JSITER_OWNONLY | JSITER_HIDDEN | JSITER_SYMBOLS, &targetKeys)) return false; - // Steps 12-13. + // Steps 14-15. AutoIdVector targetConfigurableKeys(cx); AutoIdVector targetNonconfigurableKeys(cx); - // Step 14. + // Step 16. Rooted desc(cx); for (size_t i = 0; i < targetKeys.length(); ++i) { - // Step 14a. + // Step 16.a. if (!GetOwnPropertyDescriptor(cx, target, targetKeys[i], &desc)) return false; - // Steps 14b-c. + // Steps 16.b-c. if (desc.object() && !desc.configurable()) { if (!targetNonconfigurableKeys.append(targetKeys[i])) return false; @@ -779,61 +796,47 @@ ScriptedProxyHandler::ownPropertyKeys(JSContext* cx, HandleObject proxy, AutoIdV } } - // Step 15. + // Step 17. if (extensibleTarget && targetNonconfigurableKeys.empty()) return props.appendAll(trapResult); - // Step 16. - // The algorithm below always removes all occurences of the same key - // at once, so we can use a set here. - Rooted> uncheckedResultKeys(cx, GCHashSet(cx)); - if (!uncheckedResultKeys.init(trapResult.length())) - return false; - - for (size_t i = 0, len = trapResult.length(); i < len; i++) { - MOZ_ASSERT(!JSID_IS_VOID(trapResult[i])); - - if (!uncheckedResultKeys.put(trapResult[i])) - return false; - } - - // Step 17. + // Step 19. for (size_t i = 0; i < targetNonconfigurableKeys.length(); ++i) { MOZ_ASSERT(!JSID_IS_VOID(targetNonconfigurableKeys[i])); auto ptr = uncheckedResultKeys.lookup(targetNonconfigurableKeys[i]); - // Step 17a. + // Step 19.a. if (!ptr) return js::Throw(cx, targetNonconfigurableKeys[i], JSMSG_CANT_SKIP_NC); - // Step 17b. + // Step 19.b. uncheckedResultKeys.remove(ptr); } - // Step 18. + // Step 20. if (extensibleTarget) return props.appendAll(trapResult); - // Step 19. + // Step 21. for (size_t i = 0; i < targetConfigurableKeys.length(); ++i) { MOZ_ASSERT(!JSID_IS_VOID(targetConfigurableKeys[i])); auto ptr = uncheckedResultKeys.lookup(targetConfigurableKeys[i]); - // Step 19a. + // Step 21.a. if (!ptr) return js::Throw(cx, targetConfigurableKeys[i], JSMSG_CANT_REPORT_E_AS_NE); - // Step 19b. + // Step 21.b. uncheckedResultKeys.remove(ptr); } - // Step 20. + // Step 22. if (!uncheckedResultKeys.empty()) return js::Throw(cx, uncheckedResultKeys.all().front(), JSMSG_CANT_REPORT_NEW); - // Step 21. + // Step 23. return props.appendAll(trapResult); } diff --git a/js/src/tests/ecma_6/Proxy/ownkeys-trap-duplicates.js b/js/src/tests/ecma_6/Proxy/ownkeys-trap-duplicates.js index eef84d5713d4..d52566265793 100644 --- a/js/src/tests/ecma_6/Proxy/ownkeys-trap-duplicates.js +++ b/js/src/tests/ecma_6/Proxy/ownkeys-trap-duplicates.js @@ -8,7 +8,8 @@ var BUGNUMBER = 1293995; var summary = "Scripted proxies' [[OwnPropertyKeys]] should not throw if the trap " + "implementation returns duplicate properties and the object is " + - "non-extensible or has non-configurable properties"; + "non-extensible or has non-configurable properties." + + "Revised (bug 1389752): Throw TypeError for duplicate properties."; print(BUGNUMBER + ": " + summary); @@ -18,11 +19,11 @@ print(BUGNUMBER + ": " + summary); var target = Object.preventExtensions({ a: 1 }); var proxy = new Proxy(target, { ownKeys(t) { return ["a", "a"]; } }); -assertDeepEq(Object.getOwnPropertyNames(proxy), ["a", "a"]); +assertThrowsInstanceOf(() => Object.getOwnPropertyNames(proxy), TypeError); target = Object.freeze({ a: 1 }); proxy = new Proxy(target, { ownKeys(t) { return ["a", "a"]; } }); -assertDeepEq(Object.getOwnPropertyNames(proxy), ["a", "a"]); +assertThrowsInstanceOf(() => Object.getOwnPropertyNames(proxy), TypeError); /******************************************************************************/ diff --git a/js/src/tests/jstests.list b/js/src/tests/jstests.list index 720973847efd..ff7304b2705c 100644 --- a/js/src/tests/jstests.list +++ b/js/src/tests/jstests.list @@ -744,3 +744,6 @@ skip script test262/language/expressions/class/dstr-gen-meth-static-dflt-obj-ptr # https://github.com/tc39/test262/pull/1170 skip script test262/language/module-code/namespace/internals/define-own-property.js skip script test262/language/module-code/namespace/internals/set.js + +# Not updated in https://github.com/tc39/test262/commit/a775c243bc36a9a4b5656824ef210c01dc6afffd +skip script test262/built-ins/Object/getOwnPropertyDescriptors/duplicate-keys.js diff --git a/testing/web-platform/tests/fetch/api/headers/headers-record.html b/testing/web-platform/tests/fetch/api/headers/headers-record.html index a91b9b58c03b..85dfadd269d2 100644 --- a/testing/web-platform/tests/fetch/api/headers/headers-record.html +++ b/testing/web-platform/tests/fetch/api/headers/headers-record.html @@ -251,59 +251,21 @@ test(function() { ownKeys: function() { return [ "a", "c", "a", "c" ]; }, - getCalls: 0, - gotCOnce: false, - get: function(target, name, receiver) { - if (name == "c") { - this.gotCOnce = true; - } - if (typeof name == "string") { - return ++this.getCalls; - } - return Reflect.get(target, name, receiver); - }, - getOwnPropertyDescriptor: function(target, name) { - var desc = Reflect.getOwnPropertyDescriptor(target, name); - if (name == "c" && this.gotCOnce) { - desc.enumerable = false; - } - return desc; - } }; var lyingProxy = new Proxy(record, lyingHandler); var proxy = new Proxy(lyingProxy, loggingHandler); - var h = new Headers(proxy); - assert_equals(log.length, 9); + // Returning duplicate keys from ownKeys() throws a TypeError. + assert_throws(new TypeError(), + function() { var h = new Headers(proxy); }); + + assert_equals(log.length, 2); // The first thing is the [[Get]] of Symbol.iterator to figure out whether // we're a sequence, during overload resolution. assert_array_equals(log[0], ["get", lyingProxy, Symbol.iterator, proxy]); // Then we have the [[OwnPropertyKeys]] from // https://heycam.github.io/webidl/#es-to-record step 4. assert_array_equals(log[1], ["ownKeys", lyingProxy]); - // Then the [[GetOwnProperty]] from step 5.1. - assert_array_equals(log[2], ["getOwnPropertyDescriptor", lyingProxy, "a"]); - // Then the [[Get]] from step 5.2. - assert_array_equals(log[3], ["get", lyingProxy, "a", proxy]); - // Then the second [[GetOwnProperty]] from step 5.1. - assert_array_equals(log[4], ["getOwnPropertyDescriptor", lyingProxy, "c"]); - // Then the second [[Get]] from step 5.2. - assert_array_equals(log[5], ["get", lyingProxy, "c", proxy]); - // Then the third [[GetOwnProperty]] from step 5.1. - assert_array_equals(log[6], ["getOwnPropertyDescriptor", lyingProxy, "a"]); - // Then the third [[Get]] from step 5.2. - assert_array_equals(log[7], ["get", lyingProxy, "a", proxy]); - // Then the fourth [[GetOwnProperty]] from step 5.1. - assert_array_equals(log[8], ["getOwnPropertyDescriptor", lyingProxy, "c"]); - // No [[Get]] because not enumerable. - - // Check the results. - assert_equals([...h].length, 2); - assert_array_equals([...h.keys()], ["a", "c"]); - assert_true(h.has("a")); - assert_equals(h.get("a"), "3"); - assert_true(h.has("c")); - assert_equals(h.get("c"), "2"); }, "Correct operation ordering with repeated keys"); test(function() {