Attachment 364304 Details for Bug 185211 – [JSC] throw if ownKeys Proxy trap result contains duplicate keys

[patch] [JSC] throw if ownKeys Proxy trap result contains duplicate keys

bug-185211-20190311191159.patch (text/plain), 13.17 KB, created by Caitlin Potter (:caitp) on 2019-03-11 16:12:02 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Caitlin Potter (:caitp)

Created: 2019-03-11 16:12:02 PDT

Size: 13.17 KB

patch

obsolete

>Subversion Revision: 242741
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index e16e4500dd752f93152e7d947d17a4e119c64be2..2edcfff36ca833215abdd02ec2b5ff6fd9d1b5ac 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,19 @@
>+2019-03-11  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 NOBODY (OOPS!).
>+
>+        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-03-08  Yusuke Suzuki  <ysuzuki@apple.com>
> 
>         [JSC] BuiltinExecutables should behave like a WeakSet instead of generic WeakHandleOwner for memory footprint
>diff --git a/Source/JavaScriptCore/runtime/ProxyObject.cpp b/Source/JavaScriptCore/runtime/ProxyObject.cpp
>index bbebf3a86ef56a4c34b33655f31828d0b1148f70..8cafad558c354e7f453a8d67179223396afeb599 100644
>--- a/Source/JavaScriptCore/runtime/ProxyObject.cpp
>+++ b/Source/JavaScriptCore/runtime/ProxyObject.cpp
>@@ -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;
>diff --git a/LayoutTests/imported/w3c/ChangeLog b/LayoutTests/imported/w3c/ChangeLog
>index b84566210b8bb14c9a88fc667d70d38946c851c4..7d147fbfaa9324dbb8f111127009248345b3db24 100644
>--- a/LayoutTests/imported/w3c/ChangeLog
>+++ b/LayoutTests/imported/w3c/ChangeLog
>@@ -1,3 +1,16 @@
>+2019-03-11  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 NOBODY (OOPS!).
>+
>+        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-03-07  Frederic Wang  <fwang@igalia.com>
> 
>         Update WPT tests for embedded content
>diff --git a/LayoutTests/imported/w3c/web-platform-tests/fetch/api/headers/headers-record-expected.txt b/LayoutTests/imported/w3c/web-platform-tests/fetch/api/headers/headers-record-expected.txt
>index 6147cbac0b24ec0c69935f9a1cb1c12c8d0377d2..ab448156e819e2ac54a103200f158d54a2248b28 100644
>--- a/LayoutTests/imported/w3c/web-platform-tests/fetch/api/headers/headers-record-expected.txt
>+++ b/LayoutTests/imported/w3c/web-platform-tests/fetch/api/headers/headers-record-expected.txt
>@@ -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
> 
>diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
>index f345dd3f1132eb8388f1f076d31cc75d6279c000..59085e854641335e96c3f36ba3febc759c74db40 100644
>--- a/JSTests/ChangeLog
>+++ b/JSTests/ChangeLog
>@@ -1,3 +1,25 @@
>+2019-03-11  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 NOBODY (OOPS!).
>+
>+        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-03-11  Caio Lima  <ticaiolima@gmail.com>
> 
>         [ESNext][BigInt] Implement "~" unary operation
>diff --git a/JSTests/es6/Proxy_ownKeys_duplicates.js b/JSTests/es6/Proxy_ownKeys_duplicates.js
>index d4f96aba9760b6dcb64fad1f57d2ca8f03e7eced..f3722b1bf97878ab5ac488f4f81424df47604854 100644
>--- a/JSTests/es6/Proxy_ownKeys_duplicates.js
>+++ b/JSTests/es6/Proxy_ownKeys_duplicates.js
>@@ -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;
> 
> }
> 
>diff --git a/JSTests/stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js b/JSTests/stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js
>index 1d09086dab57411bae0d8df73e9172abde49c868..a4c659f076a971552ece3c7acdf3dcd2eeec61e8 100644
>--- a/JSTests/stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js
>+++ b/JSTests/stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js
>@@ -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() {
>diff --git a/JSTests/stress/proxy-own-keys.js b/JSTests/stress/proxy-own-keys.js
>index 189b20b36ef047d466edee765e215c98019eed4a..01756fb3bcd78240e2cba8590a2974d94df545f4 100644
>--- a/JSTests/stress/proxy-own-keys.js
>+++ b/JSTests/stress/proxy-own-keys.js
>@@ -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;
>     }
> }
>

Actions: View | Formatted Diff | Diff

Attachments on bug 185211: 362734 | 362796 | 362910 | 362914 | 364289 | 364303 | 364304 | 364305 | 364358