Attachment 360004 Details for Bug 176810 – (1) [JSC] throw if 'ownKeys' Proxy trap result contains duplicate keys

[patch] (1) [JSC] throw if 'ownKeys' Proxy trap result contains duplicate keys

bug-176810-20190124073136.patch (text/plain), 13.02 KB, created by Caitlin Potter (:caitp) on 2019-01-24 04:31:38 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Caitlin Potter (:caitp)

Created: 2019-01-24 04:31:38 PST

Size: 13.02 KB

patch

obsolete

>Subversion Revision: 240301
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index c34ced4859fe3931fbbc2a2aa5f7cb5ce03beb94..95ff4ac6d908730562ee33e17598efb6cc946b9b 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,19 @@
>+2019-01-23  Caitlin Potter  <caitp@igalia.com>
>+
>+        [JSC] throw if ownKeys Proxy trap result contains duplicate keys
>+        https://bugs.webkit.org/show_bug.cgi?id=176810
>+
>+        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-01-22  Yusuke Suzuki  <ysuzuki@apple.com>
> 
>         [JSC] Intl constructors should fit in sizeof(InternalFunction)
>diff --git a/Source/JavaScriptCore/runtime/ProxyObject.cpp b/Source/JavaScriptCore/runtime/ProxyObject.cpp
>index bbebf3a86ef56a4c34b33655f31828d0b1148f70..1ca574a13101508bc51b1760db7f99348b8fbb63 100644
>--- a/Source/JavaScriptCore/runtime/ProxyObject.cpp
>+++ b/Source/JavaScriptCore/runtime/ProxyObject.cpp
>@@ -939,17 +939,29 @@ 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, 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).
>+        if (seenKeys.contains(ident.impl())) {
>+            throwTypeError(exec, scope, "Proxy handler's 'ownKeys' trap result must not contain any duplicate names"_s);
>+            return doExitEarly;
>+        }
>+        seenKeys.add(ident.impl());
>+
>+        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 7c6f04ef7e1fabcd8d397f4472fa59c431b49a6a..39206f5a42675243ed5ecfb35bd27282f1e6a7ec 100644
>--- a/LayoutTests/imported/w3c/ChangeLog
>+++ b/LayoutTests/imported/w3c/ChangeLog
>@@ -1,3 +1,16 @@
>+2019-01-23  Caitlin Potter  <caitp@igalia.com>
>+
>+        [JSC] throw if 'ownKeys' Proxy trap result contains duplicate keys
>+        https://bugs.webkit.org/show_bug.cgi?id=176810
>+
>+        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-01-22  Oriol Brufau  <obrufau@igalia.com>
> 
>         [css-logical] Implement flow-relative margin, padding and border shorthands
>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 d855bd472dafbaace71fd7ec2216c2e119d52cc3..cb5ed901d235845cf2a37c34a24a66abfb264b94 100644
>--- a/JSTests/ChangeLog
>+++ b/JSTests/ChangeLog
>@@ -1,3 +1,25 @@
>+2019-01-24  Caitlin Potter  <caitp@igalia.com>
>+
>+        [JSC] throw if 'ownKeys' Proxy trap result contains duplicate keys
>+        https://bugs.webkit.org/show_bug.cgi?id=176810
>+
>+        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-01-22  Saam Barati  <sbarati@apple.com>
> 
>         Unreviewed. Rollout r240223. It regressed JetStream2 by 1%.
>diff --git a/JSTests/es6/Proxy_ownKeys_duplicates.js b/JSTests/es6/Proxy_ownKeys_duplicates.js
>index d4f96aba9760b6dcb64fad1f57d2ca8f03e7eced..41c2daa44f45597f516663a73976b2e266b1bc21 100644
>--- a/JSTests/es6/Proxy_ownKeys_duplicates.js
>+++ b/JSTests/es6/Proxy_ownKeys_duplicates.js
>@@ -1,29 +1,49 @@
>+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();
>+        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 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;
>     }
> }
>

Flags:

mark.lam: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 176810: 351389 | 351402 | 351405 | 351412 | 351468 | 351472 | 359910 | 359911 | 359912 | 359922 | 359930 | 359935 | 359937 | 359941 | 359952 | 359953 | 359955 | 359959 | 359961 | 359975 | 359979 | 359980 | 359988 | 360004 | 360032 | 362731 | 362762 | 362812 | 362896 | 366956 | 367326