Changeset 243933 in webkit

Timestamp:

Apr 5, 2019, 6:45:08 AM (7 years ago)

Author:

caitp@igalia.com

Message:

JSTests:
[JSC] throw if 'ownKeys' Proxy trap result contains duplicate keys
​https://bugs.webkit.org/show_bug.cgi?id=185211

Reviewed by Saam Barati.

This is for the normative spec change in ​https://github.com/tc39/ecma262/pull/833

This changes several assertions to expect a TypeError to be thrown (in some cases,
changing thee expected message).

• es6/Proxy_ownKeys_duplicates.js:

(handler):
(shouldThrow):
(test):

• stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js:

(shouldThrow):

• stress/proxy-own-keys.js:

(i.catch):
(assert):

LayoutTests/imported/w3c:
[JSC] throw if 'ownKeys' Proxy trap result contains duplicate keys
​https://bugs.webkit.org/show_bug.cgi?id=185211

Reviewed by Saam Barati.

This is for the normative spec change in ​https://github.com/tc39/ecma262/pull/833

Change some test expectations which were previously expected to fail.

• web-platform-tests/fetch/api/headers/headers-record-expected.txt:

Source/JavaScriptCore:
[JSC] throw if ownKeys Proxy trap result contains duplicate keys
​https://bugs.webkit.org/show_bug.cgi?id=185211

Reviewed by Saam Barati.

Implements the normative spec change in ​https://github.com/tc39/ecma262/pull/833

This involves tracking duplicate keys returned from the ownKeys trap in yet
another HashTable, and may incur a minor performance penalty in some cases. This
is not expected to significantly affect web performance.

• runtime/ProxyObject.cpp:

(JSC::ProxyObject::performGetOwnPropertyNames):

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/es6/Proxy_ownKeys_duplicates.js (modified) (1 diff)
• JSTests/stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js (modified) (2 diffs)
• JSTests/stress/proxy-own-keys.js (modified) (3 diffs)
• LayoutTests/imported/w3c/ChangeLog (modified) (1 diff)
• LayoutTests/imported/w3c/web-platform-tests/fetch/api/headers/headers-record-expected.txt (modified) (1 diff)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/runtime/ProxyObject.cpp (modified) (2 diffs)

trunk/JSTests/ChangeLog

@@ +1 @@
+2019-04-05  Caitlin Potter  <caitp@igalia.com>
+
+        [JSC] throw if 'ownKeys' Proxy trap result contains duplicate keys
+        https://bugs.webkit.org/show_bug.cgi?id=185211
+
+        Reviewed by Saam Barati.
+
+        This is for the normative spec change in https://github.com/tc39/ecma262/pull/833
+
+        This changes several assertions to expect a TypeError to be thrown (in some cases,
+        changing thee expected message).
+
+        * es6/Proxy_ownKeys_duplicates.js:
+        (handler):
+        (shouldThrow):
+        (test):
+        * stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js:
+        (shouldThrow):
+        * stress/proxy-own-keys.js:
+        (i.catch):
+        (assert):
+
 2019-04-04  Yusuke Suzuki  <ysuzuki@apple.com>
 

trunk/JSTests/es6/Proxy_ownKeys_duplicates.js

@@ +1 @@
+function handler(key) {
+    return {
+        getOwnPropertyDescriptor(t, n) {
+            // Required to prevent Object.keys() from discarding results
+            return {
+                enumerable: true,
+                configurable: true,
+            };
+        },
+        ownKeys(t) {
+            return [key, key];
+        }
+    };
+}
+
+function shouldThrow(op, errorConstructor, desc) {
+    try {
+        op();
+    } catch (e) {
+        if (!(e instanceof errorConstructor)) {
+            throw new Error(`threw ${e}, but should have thrown ${errorConstructor.name}`);
+        }
+        return;
+    }
+    throw new Error(`Expected ${desc || 'operation'} to throw ${errorConstructor.name}, but no exception thrown`);
+}
+
 function test() {
 
 var symbol = Symbol("test");
-var proxy = new Proxy({}, {
-    getOwnPropertyDescriptor(t, n) {
-        // Required to prevent Object.keys() from discarding results
-        return {
-            enumerable: true,
-            configurable: true
-        };
-    },
-    ownKeys: function (t) {
-        return ["A", "A", "0", "0", symbol, symbol];
-    }
-});
-var keys = Object.keys(proxy);
-var names = Object.getOwnPropertyNames(proxy);
-var symbols = Object.getOwnPropertySymbols(proxy);
+var proxyNamed = new Proxy({}, handler("A"));
+var proxyIndexed = new Proxy({}, handler(0));
+var proxySymbol = new Proxy({}, handler(symbol));
 
-if (keys.length === 4 && keys[0] === keys[1] && keys[2] === keys[3] &&
-    keys[0] === "A" && keys[2] === "0" &&
-    names.length === 4 && names[0] === names[1] && names[2] === names[3] &&
-    names[0] === "A" && names[2] === "0" &&
-    symbols.length === 2 && symbols[0] === symbols[1] && symbols[0] === symbol)
-    return true;
-return false;
+shouldThrow(() => Object.keys(proxyNamed), TypeError, "Object.keys with duplicate named properties");
+shouldThrow(() => Object.keys(proxyIndexed), TypeError, "Object.keys with duplicate indexed properties");
+shouldThrow(() => Object.keys(proxySymbol), TypeError, "Object.keys with duplicate symbol properties");
+
+shouldThrow(() => Object.getOwnPropertyNames(proxyNamed), TypeError, "Object.getOwnPropertyNames with duplicate named properties");
+shouldThrow(() => Object.getOwnPropertyNames(proxyIndexed), TypeError, "Object.getOwnPropertyNames with duplicate indexed properties");
+shouldThrow(() => Object.getOwnPropertyNames(proxySymbol), TypeError, "Object.getOwnPropertyNames with duplicate symbol properties");
+
+shouldThrow(() => Object.getOwnPropertySymbols(proxyNamed), TypeError, "Object.getOwnPropertySymbols with duplicate named properties");
+shouldThrow(() => Object.getOwnPropertySymbols(proxyIndexed), TypeError, "Object.getOwnPropertySymbols with duplicate indexed properties");
+shouldThrow(() => Object.getOwnPropertySymbols(proxySymbol), TypeError, "Object.getOwnPropertySymbols with duplicate symbol properties");
+
+return true;
 
 }

trunk/JSTests/stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js

@@ -17 +17 @@
     shouldBe(undefined, expected.get, name + '.get');
     shouldBe(undefined, expected.set, name + '.set');
+}
+
+function shouldThrow(op, errorConstructor, desc) {
+    try {
+        op();
+        throw new Error(`Expected ${desc || 'operation'} to throw ${errorConstructor.name}, but no exception thrown`);
+    } catch (e) {
+        if (!(e instanceof errorConstructor)) {
+            throw new Error(`threw ${e}, but should have thrown ${errorConstructor.name}`);
+        }
+    }
 }
 
@@ -81 +92 @@
   });
 
-  var result = Object.getOwnPropertyDescriptors(P);
-  shouldBe(true, result.A.configurable, 'for result.A.configurable');
-  shouldBe(false, result.A.writable, 'for result.A.writable');
-  shouldBe('VALUE', result.A.value, 'for result.A.value');
-  shouldBe(false, result.A.enumerable, 'for result.A.enumerable');
-  shouldBe(true, Object.hasOwnProperty.call(result, 'A'));
-  shouldBe('ownKeys()|getOwnPropertyDescriptor(A)|getOwnPropertyDescriptor(A)', log.join('|'));
+  shouldThrow(() => Object.getOwnPropertyDescriptors(P), TypeError, 'ownKeys returning duplicates');
+  shouldBe('ownKeys()', log.join('|'));
 })();
 

trunk/JSTests/stress/proxy-own-keys.js

@@ -188 +188 @@
 
     for (let i = 0; i < 500; i++) {
-        // FIXME: we may update the spec to make this test not throw.
-        // see: https://github.com/tc39/ecma262/pull/594
+        // Throws per https://github.com/tc39/ecma262/pull/833
         let threw = false;
         try {
             Reflect.ownKeys(p2);
         } catch(e) {
-            assert(e.toString() === "TypeError: Proxy object's 'target' has the non-configurable property 'a' that was not in the result from the 'ownKeys' trap");
+            assert(e.toString() === "TypeError: Proxy handler's 'ownKeys' trap result must not contain any duplicate names");
             threw = true;
         }
@@ -223 +222 @@
 
     for (let i = 0; i < 500; i++) {
-        // FIXME: we may update the spec to make this test not throw.
-        // see: https://github.com/tc39/ecma262/pull/594
+        // Throws per https://github.com/tc39/ecma262/pull/833
         let threw = false;
         try {
             Reflect.ownKeys(p2);
         } catch(e) {
-            assert(e.toString() === "TypeError: Proxy object's non-extensible 'target' has configurable property 'a' that was not in the result from the 'ownKeys' trap");
+            assert(e.toString() === "TypeError: Proxy handler's 'ownKeys' trap result must not contain any duplicate names");
             threw = true;
         }
@@ -256 +254 @@
     let proxy = new Proxy(target, handler);
     for (let i = 0; i < 500; i++) {
-        Object.keys(proxy);
-        assert(called);
-        called = false;
+        try {
+            Object.keys(proxy);
+        } catch(e) {
+            assert(e.toString() === "TypeError: Proxy handler's 'ownKeys' trap result must not contain any duplicate names");
+            threw = true;
+        }
+        assert(called);
+        assert(threw);
+        called = false;
+        threw = false;
     }
 }

trunk/LayoutTests/imported/w3c/ChangeLog

@@ +1 @@
+2019-04-05  Caitlin Potter  <caitp@igalia.com>
+
+        [JSC] throw if 'ownKeys' Proxy trap result contains duplicate keys
+        https://bugs.webkit.org/show_bug.cgi?id=185211
+
+        Reviewed by Saam Barati.
+
+        This is for the normative spec change in https://github.com/tc39/ecma262/pull/833
+
+        Change some test expectations which were previously expected to fail.
+
+        * web-platform-tests/fetch/api/headers/headers-record-expected.txt:
+
 2019-04-04  Commit Queue  <commit-queue@webkit.org>
 

trunk/LayoutTests/imported/w3c/web-platform-tests/fetch/api/headers/headers-record-expected.txt

@@ -10 +10 @@
 PASS Correct operation ordering with non-enumerable properties 
 PASS Correct operation ordering with undefined descriptors 
-FAIL Correct operation ordering with repeated keys assert_throws: function "function () { var h = new Headers(proxy); }" did not throw
+PASS Correct operation ordering with repeated keys 
 FAIL Basic operation with Symbol keys assert_throws: function "function () { var h = new Headers(proxy); }" did not throw
 FAIL Operation with non-enumerable Symbol keys assert_equals: expected 9 but got 8

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2019-04-05  Caitlin Potter  <caitp@igalia.com>
+
+        [JSC] throw if ownKeys Proxy trap result contains duplicate keys
+        https://bugs.webkit.org/show_bug.cgi?id=185211
+
+        Reviewed by Saam Barati.
+
+        Implements the normative spec change in https://github.com/tc39/ecma262/pull/833
+
+        This involves tracking duplicate keys returned from the ownKeys trap in yet
+        another HashTable, and may incur a minor performance penalty in some cases. This
+        is not expected to significantly affect web performance.
+
+        * runtime/ProxyObject.cpp:
+        (JSC::ProxyObject::performGetOwnPropertyNames):
+
 2019-04-04  Yusuke Suzuki  <ysuzuki@apple.com>
 

trunk/Source/JavaScriptCore/runtime/ProxyObject.cpp

@@ -940 +940 @@
     RuntimeTypeMask dontThrowAnExceptionTypeFilter = TypeString | TypeSymbol;
     HashSet<UniquedStringImpl*> uncheckedResultKeys;
+    HashSet<UniquedStringImpl*> seenKeys;
 
     auto addPropName = [&] (JSValue value, RuntimeType type) -> bool {
@@ -945 +946 @@
         static const bool dontExitEarly = false;
 
+        Identifier ident = value.toPropertyKey(exec);
+        RETURN_IF_EXCEPTION(scope, doExitEarly);
+
+        // If trapResult contains any duplicate entries, throw a TypeError exception.
+        //    
+        // Per spec[1], filtering by type should occur _after_ [[OwnPropertyKeys]], so duplicates
+        // are tracked in a separate hashtable from uncheckedResultKeys (which only contain the
+        // keys filtered by type).
+        //
+        // [1] Per https://tc39.github.io/ecma262/#sec-proxy-object-internal-methods-and-internal-slots-ownpropertykeysmust not contain any duplicate names"_s);
+        if (!seenKeys.add(ident.impl()).isNewEntry) {
+            throwTypeError(exec, scope, "Proxy handler's 'ownKeys' trap result must not contain any duplicate names"_s);
+            return doExitEarly;
+        }
+
         if (!(type & resultFilter))
             return dontExitEarly;
-
-        Identifier ident = value.toPropertyKey(exec);
-        RETURN_IF_EXCEPTION(scope, doExitEarly);
 
         uncheckedResultKeys.add(ident.impl());