JSTests:
authorcaitp@igalia.com <caitp@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 5 Apr 2019 13:45:08 +0000 (13:45 +0000)
committercaitp@igalia.com <caitp@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 5 Apr 2019 13:45:08 +0000 (13:45 +0000)
[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):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@243933 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JSTests/ChangeLog
JSTests/es6/Proxy_ownKeys_duplicates.js
JSTests/stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js
JSTests/stress/proxy-own-keys.js
LayoutTests/imported/w3c/ChangeLog
LayoutTests/imported/w3c/web-platform-tests/fetch/api/headers/headers-record-expected.txt
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/runtime/ProxyObject.cpp

index bbf7bfd..a1f2bed 100644 (file)
@@ -1,3 +1,25 @@
+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>
 
         [JSC] makeBoundFunction should not assume incoming "length" value is Int32 because it performs some calculation in bytecode
index d4f96ab..f3722b1 100644 (file)
@@ -1,29 +1,50 @@
+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);
-
-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;
+var proxyNamed = new Proxy({}, handler("A"));
+var proxyIndexed = new Proxy({}, handler(0));
+var proxySymbol = new Proxy({}, handler(symbol));
+
+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;
 
 }
 
index 1d09086..a4c659f 100644 (file)
@@ -18,6 +18,17 @@ function shouldBeDataProperty(expected, value, name) {
     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}`);
+        }
+    }
+}
+
 (function testPropertyFilteringAndOrder() {
     var log = [];
     var sym = Symbol('test');
@@ -80,13 +91,8 @@ function shouldBeDataProperty(expected, value, name) {
     defineProperty() { throw new Error('[[DefineOwnProperty]] trap should be unreachable'); }
   });
 
-  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('|'));
 })();
 
 (function testUndefinedPropertyDescriptor() {
index 189b20b..01756fb 100644 (file)
@@ -187,13 +187,12 @@ function assert(b) {
     });
 
     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;
         }
         assert(threw);
@@ -222,13 +221,12 @@ function assert(b) {
     });
 
     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;
         }
         assert(threw);
@@ -255,9 +253,16 @@ function assert(b) {
 
     let proxy = new Proxy(target, handler);
     for (let i = 0; i < 500; i++) {
-        Object.keys(proxy);
+        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;
     }
 }
 
index bff1c59..d591c05 100644 (file)
@@ -1,3 +1,16 @@
+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>
 
         Unreviewed, rolling out r243807 and r243824.
index 6147cba..ab44815 100644 (file)
@@ -9,7 +9,7 @@ PASS Correct operation ordering with two properties one of which has an invalid
 PASS Correct operation ordering with two properties one of which has an invalid value 
 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
 
index 71f5a9a..3d2396b 100644 (file)
@@ -1,3 +1,19 @@
+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>
 
         [JSC] makeBoundFunction should not assume incoming "length" value is Int32 because it performs some calculation in bytecode
index bbebf3a..8cafad5 100644 (file)
@@ -939,17 +939,30 @@ void ProxyObject::performGetOwnPropertyNames(ExecState* exec, PropertyNameArray&
     ASSERT(resultFilter);
     RuntimeTypeMask dontThrowAnExceptionTypeFilter = TypeString | TypeSymbol;
     HashSet<UniquedStringImpl*> uncheckedResultKeys;
+    HashSet<UniquedStringImpl*> seenKeys;
 
     auto addPropName = [&] (JSValue value, RuntimeType type) -> bool {
         static const bool doExitEarly = true;
         static const bool dontExitEarly = false;
 
-        if (!(type & resultFilter))
-            return dontExitEarly;
-
         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;
+
         uncheckedResultKeys.add(ident.impl());
         trapResult.addUnchecked(ident.impl());
         return dontExitEarly;