Diff
Modified: trunk/JSTests/ChangeLog (243932 => 243933)
--- trunk/JSTests/ChangeLog 2019-04-05 12:42:44 UTC (rev 243932)
+++ trunk/JSTests/ChangeLog 2019-04-05 13:45:08 UTC (rev 243933)
@@ -1,3 +1,25 @@
+2019-04-05 Caitlin Potter <ca...@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 <ysuz...@apple.com>
[JSC] makeBoundFunction should not assume incoming "length" value is Int32 because it performs some calculation in bytecode
Modified: trunk/JSTests/es6/Proxy_ownKeys_duplicates.js (243932 => 243933)
--- trunk/JSTests/es6/Proxy_ownKeys_duplicates.js 2019-04-05 12:42:44 UTC (rev 243932)
+++ trunk/JSTests/es6/Proxy_ownKeys_duplicates.js 2019-04-05 13:45:08 UTC (rev 243933)
@@ -1,30 +1,51 @@
+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;
+
}
if (!test())
Modified: trunk/JSTests/stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js (243932 => 243933)
--- trunk/JSTests/stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js 2019-04-05 12:42:44 UTC (rev 243932)
+++ trunk/JSTests/stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js 2019-04-05 13:45:08 UTC (rev 243933)
@@ -18,6 +18,17 @@
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 @@
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() {
Modified: trunk/JSTests/stress/proxy-own-keys.js (243932 => 243933)
--- trunk/JSTests/stress/proxy-own-keys.js 2019-04-05 12:42:44 UTC (rev 243932)
+++ trunk/JSTests/stress/proxy-own-keys.js 2019-04-05 13:45:08 UTC (rev 243933)
@@ -187,13 +187,12 @@
});
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 @@
});
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 @@
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;
}
}
Modified: trunk/LayoutTests/imported/w3c/ChangeLog (243932 => 243933)
--- trunk/LayoutTests/imported/w3c/ChangeLog 2019-04-05 12:42:44 UTC (rev 243932)
+++ trunk/LayoutTests/imported/w3c/ChangeLog 2019-04-05 13:45:08 UTC (rev 243933)
@@ -1,3 +1,16 @@
+2019-04-05 Caitlin Potter <ca...@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-qu...@webkit.org>
Unreviewed, rolling out r243807 and r243824.
Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/fetch/api/headers/headers-record-expected.txt (243932 => 243933)
--- trunk/LayoutTests/imported/w3c/web-platform-tests/fetch/api/headers/headers-record-expected.txt 2019-04-05 12:42:44 UTC (rev 243932)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/fetch/api/headers/headers-record-expected.txt 2019-04-05 13:45:08 UTC (rev 243933)
@@ -9,7 +9,7 @@
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
Modified: trunk/Source/_javascript_Core/ChangeLog (243932 => 243933)
--- trunk/Source/_javascript_Core/ChangeLog 2019-04-05 12:42:44 UTC (rev 243932)
+++ trunk/Source/_javascript_Core/ChangeLog 2019-04-05 13:45:08 UTC (rev 243933)
@@ -1,3 +1,19 @@
+2019-04-05 Caitlin Potter <ca...@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 <ysuz...@apple.com>
[JSC] makeBoundFunction should not assume incoming "length" value is Int32 because it performs some calculation in bytecode
Modified: trunk/Source/_javascript_Core/runtime/ProxyObject.cpp (243932 => 243933)
--- trunk/Source/_javascript_Core/runtime/ProxyObject.cpp 2019-04-05 12:42:44 UTC (rev 243932)
+++ trunk/Source/_javascript_Core/runtime/ProxyObject.cpp 2019-04-05 13:45:08 UTC (rev 243933)
@@ -939,17 +939,30 @@
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;
+ 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());
trapResult.addUnchecked(ident.impl());
return dontExitEarly;
