Bug 1389752 - Throw TypeError if [[OwnPropertyKeys]] of scripted proxies contains duplicates. r=till, r=qdot

--HG--
extra : rebase_source : 7c31e7c3443d7a1885d89a0688022f68f5034bf0

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);

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);

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")

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<GCHashSet<jsid>> uncheckedResultKeys(cx, GCHashSet<jsid>(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<PropertyDescriptor> 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<GCHashSet<jsid>> uncheckedResultKeys(cx, GCHashSet<jsid>(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);
 }
 

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);
 
 /******************************************************************************/
 

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

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() {