Log Message
Proxy's "ownKeys" trap result should not be sorted https://bugs.webkit.org/show_bug.cgi?id=216227
Reviewed by Yusuke Suzuki.
JSTests:
* microbenchmarks/object-get-own-property-symbols-on-large-array.js:
* microbenchmarks/object-get-own-property-symbols.js:
* microbenchmarks/reflect-own-keys.js: Added.
* stress/proxy-own-keys.js: Fix incorrect assert.
* test262/expectations.yaml: Mark 20 test cases as passing.
Source/_javascript_Core:
Given that we can't know whether ownPropertyKeys() received property names from
userland Proxy's "ownKeys" trap, this patch moves symbols after strings sorting [1]
to Structure::getPropertyNamesFromStructure(), aligning observed property order
(via Proxy's "getOwnPropertyDescriptor" trap) with V8 and SpiderMonkey.
Also, removes sorting logic duplication in objectConstructorAssign().
This change is neutral on provided Reflect.ownKeys microbenchmark. Although property
name collection besides PropertyNameMode::StringsAndSymbols cases is unaffected,
Object.{keys,getOwnPropertySymbols} microbenchmarks regress by 6-12% due to
increased Structure::getPropertyNamesFromStructure() code size.
[1]: https://tc39.es/ecma262/#sec-ordinaryownpropertykeys (steps 3-4)
* runtime/ObjectConstructor.cpp:
(JSC::objectConstructorAssign):
(JSC::ownPropertyKeys):
* runtime/Structure.cpp:
(JSC::Structure::getPropertyNamesFromStructure):
Modified Paths
- trunk/JSTests/ChangeLog
- trunk/JSTests/microbenchmarks/object-get-own-property-symbols-on-large-array.js
- trunk/JSTests/microbenchmarks/object-get-own-property-symbols.js
- trunk/JSTests/stress/proxy-own-keys.js
- trunk/JSTests/test262/expectations.yaml
- trunk/Source/_javascript_Core/ChangeLog
- trunk/Source/_javascript_Core/runtime/ObjectConstructor.cpp
- trunk/Source/_javascript_Core/runtime/Structure.cpp
Added Paths
Diff
Modified: trunk/JSTests/ChangeLog (267039 => 267040)
--- trunk/JSTests/ChangeLog 2020-09-14 21:00:54 UTC (rev 267039)
+++ trunk/JSTests/ChangeLog 2020-09-14 21:30:25 UTC (rev 267040)
@@ -1,5 +1,18 @@
2020-09-14 Alexey Shvayka <[email protected]>
+ Proxy's "ownKeys" trap result should not be sorted
+ https://bugs.webkit.org/show_bug.cgi?id=216227
+
+ Reviewed by Yusuke Suzuki.
+
+ * microbenchmarks/object-get-own-property-symbols-on-large-array.js:
+ * microbenchmarks/object-get-own-property-symbols.js:
+ * microbenchmarks/reflect-own-keys.js: Added.
+ * stress/proxy-own-keys.js: Fix incorrect assert.
+ * test262/expectations.yaml: Mark 20 test cases as passing.
+
+2020-09-14 Alexey Shvayka <[email protected]>
+
ArraySetLength should coerce [[Value]] before descriptor validation
https://bugs.webkit.org/show_bug.cgi?id=158791
Modified: trunk/JSTests/microbenchmarks/object-get-own-property-symbols-on-large-array.js (267039 => 267040)
--- trunk/JSTests/microbenchmarks/object-get-own-property-symbols-on-large-array.js 2020-09-14 21:00:54 UTC (rev 267039)
+++ trunk/JSTests/microbenchmarks/object-get-own-property-symbols-on-large-array.js 2020-09-14 21:30:25 UTC (rev 267040)
@@ -3,12 +3,13 @@
{
var buffer = new ArrayBuffer(10000000);
var int8View = new Int8Array(buffer);
- var start = Date.now();
- var result = Object.getOwnPropertySymbols(int8View);
- var delta = Date.now() - start;
- if (delta > 1000)
- throw new Error(`time consuming (${delta}ms)`);
- return result;
+ for (var i = 0; i < 300000; ++i) {
+ var start = Date.now();
+ var result = Object.getOwnPropertySymbols(int8View);
+ var delta = Date.now() - start;
+ if (delta > 1000)
+ throw new Error(`time consuming (${delta}ms)`);
+ }
}
trial();
Modified: trunk/JSTests/microbenchmarks/object-get-own-property-symbols.js (267039 => 267040)
--- trunk/JSTests/microbenchmarks/object-get-own-property-symbols.js 2020-09-14 21:00:54 UTC (rev 267039)
+++ trunk/JSTests/microbenchmarks/object-get-own-property-symbols.js 2020-09-14 21:30:25 UTC (rev 267040)
@@ -10,5 +10,5 @@
}
noInline(test);
-for (var i = 0; i < 1e3; ++i)
+for (var i = 0; i < 2500; ++i)
test(object);
Added: trunk/JSTests/microbenchmarks/reflect-own-keys.js (0 => 267040)
--- trunk/JSTests/microbenchmarks/reflect-own-keys.js (rev 0)
+++ trunk/JSTests/microbenchmarks/reflect-own-keys.js 2020-09-14 21:30:25 UTC (rev 267040)
@@ -0,0 +1,10 @@
+var obj = {}, i;
+for (i = 0; i < 100; ++i)
+ obj[`k${i}`] = i;
+for (i = 0; i < 100; ++i)
+ obj[Symbol(i)] = i;
+
+noInline(Reflect.ownKeys);
+
+for (i = 0; i < 1e4; ++i)
+ Reflect.ownKeys(obj);
Modified: trunk/JSTests/stress/proxy-own-keys.js (267039 => 267040)
--- trunk/JSTests/stress/proxy-own-keys.js 2020-09-14 21:00:54 UTC (rev 267039)
+++ trunk/JSTests/stress/proxy-own-keys.js 2020-09-14 21:30:25 UTC (rev 267040)
@@ -482,7 +482,7 @@
let proxy = new Proxy(target, handler);
for (let i = 0; i < 500; i++) {
let result = Reflect.ownKeys(proxy);
- assert(shallowEq(result, ["a", "b", "c", s1, s2]));
+ assert(shallowEq(result, arr));
assert(called);
called = false;
}
Modified: trunk/JSTests/test262/expectations.yaml (267039 => 267040)
--- trunk/JSTests/test262/expectations.yaml 2020-09-14 21:00:54 UTC (rev 267039)
+++ trunk/JSTests/test262/expectations.yaml 2020-09-14 21:30:25 UTC (rev 267040)
@@ -786,21 +786,9 @@
test/built-ins/Function/prototype/toString/unicode.js:
default: 'Test262Error: Conforms to NativeFunction Syntax: "function a(\\u{62}, \\u0063) { \\u0062 = \\u{00063}; return b; }" (function \u0061(\u{62}, \u0063) { \u0062 = \u{00063}; return b; })'
strict mode: 'Test262Error: Conforms to NativeFunction Syntax: "function a(\\u{62}, \\u0063) { \\u0062 = \\u{00063}; return b; }" (function \u0061(\u{62}, \u0063) { \u0062 = \u{00063}; return b; })'
-test/built-ins/Object/assign/strings-and-symbol-order-proxy.js:
- default: 'Test262Error: Expected [foo, 0, Symbol()] and [Symbol(), foo, 0] to have the same contents. '
- strict mode: 'Test262Error: Expected [foo, 0, Symbol()] and [Symbol(), foo, 0] to have the same contents. '
-test/built-ins/Object/defineProperties/proxy-no-ownkeys-returned-keys-order.js:
- default: 'Test262Error: Expected [0, Symbol(), foo] and [0, foo, Symbol()] to have the same contents. '
- strict mode: 'Test262Error: Expected [0, Symbol(), foo] and [0, foo, Symbol()] to have the same contents. '
test/built-ins/Object/entries/order-after-define-property.js:
default: 'Test262Error: Expected [a, name] and [name, a] to have the same contents. '
strict mode: 'Test262Error: Expected [a, name] and [name, a] to have the same contents. '
-test/built-ins/Object/freeze/proxy-no-ownkeys-returned-keys-order.js:
- default: 'Test262Error: Expected [0, Symbol(), foo] and [0, foo, Symbol()] to have the same contents. '
- strict mode: 'Test262Error: Expected [0, Symbol(), foo] and [0, foo, Symbol()] to have the same contents. '
-test/built-ins/Object/getOwnPropertyDescriptors/proxy-no-ownkeys-returned-keys-order.js:
- default: 'Test262Error: Expected [0, Symbol(), foo] and [0, foo, Symbol()] to have the same contents. '
- strict mode: 'Test262Error: Expected [0, Symbol(), foo] and [0, foo, Symbol()] to have the same contents. '
test/built-ins/Object/internals/DefineOwnProperty/consistent-value-function-arguments.js:
default: 'Test262Error: Expected SameValue(«null», «[object Arguments]») to be true'
test/built-ins/Object/internals/DefineOwnProperty/consistent-value-function-caller.js:
@@ -808,18 +796,9 @@
test/built-ins/Object/internals/DefineOwnProperty/consistent-value-regexp-dollar1.js:
default: 'Test262Error: Expected SameValue(«», «x») to be true'
strict mode: 'Test262Error: Expected SameValue(«», «x») to be true'
-test/built-ins/Object/isFrozen/proxy-no-ownkeys-returned-keys-order.js:
- default: 'Test262Error: Expected [0, Symbol(), foo] and [0, foo, Symbol()] to have the same contents. '
- strict mode: 'Test262Error: Expected [0, Symbol(), foo] and [0, foo, Symbol()] to have the same contents. '
-test/built-ins/Object/isSealed/proxy-no-ownkeys-returned-keys-order.js:
- default: 'Test262Error: Expected [0, Symbol(), foo] and [0, foo, Symbol()] to have the same contents. '
- strict mode: 'Test262Error: Expected [0, Symbol(), foo] and [0, foo, Symbol()] to have the same contents. '
test/built-ins/Object/keys/order-after-define-property.js:
default: 'Test262Error: Expected [a, length] and [length, a] to have the same contents. '
strict mode: 'Test262Error: Expected [a, length] and [length, a] to have the same contents. '
-test/built-ins/Object/seal/proxy-no-ownkeys-returned-keys-order.js:
- default: 'Test262Error: Expected [0, Symbol(), foo] and [0, foo, Symbol()] to have the same contents. '
- strict mode: 'Test262Error: Expected [0, Symbol(), foo] and [0, foo, Symbol()] to have the same contents. '
test/built-ins/Proxy/apply/arguments-realm.js:
default: 'Test262Error: Expected SameValue(«function Array() {'
strict mode: 'Test262Error: Expected SameValue(«function Array() {'
@@ -856,9 +835,6 @@
test/built-ins/Proxy/construct/trap-is-not-callable-realm.js:
default: 'Test262Error: Expected a TypeError but got a TypeError'
strict mode: 'Test262Error: Expected a TypeError but got a TypeError'
-test/built-ins/Proxy/ownKeys/trap-is-undefined-target-is-proxy.js:
- default: 'Test262Error: Expected [length, foo, 0, Symbol()] and [Symbol(), length, foo, 0] to have the same contents. '
- strict mode: 'Test262Error: Expected [length, foo, 0, Symbol()] and [Symbol(), length, foo, 0] to have the same contents. '
test/built-ins/RegExp/property-escapes/generated/Alphabetic.js:
default: 'Test262Error: `\p{Alphabetic}` should match U+001CFA (`ᳺ`)'
strict mode: 'Test262Error: `\p{Alphabetic}` should match U+001CFA (`ᳺ`)'
@@ -2659,9 +2635,6 @@
test/language/expressions/object/covered-ident-name-prop-name-literal-with-escaped.js:
default: "SyntaxError: Unexpected escaped characters in keyword token: 'w\\u0069th'"
strict mode: "SyntaxError: Unexpected escaped characters in keyword token: 'w\\u0069th'"
-test/language/expressions/object/dstr/object-rest-proxy-ownkeys-returned-keys-order.js:
- default: 'Test262Error: Expected [foo, 0, Symbol()] and [Symbol(), foo, 0] to have the same contents. '
- strict mode: 'Test262Error: Expected [foo, 0, Symbol()] and [Symbol(), foo, 0] to have the same contents. '
test/language/expressions/object/ident-name-method-def-break-escaped.js:
default: "SyntaxError: Unexpected escaped characters in keyword token: 'bre\\u0061k'"
strict mode: "SyntaxError: Unexpected escaped characters in keyword token: 'bre\\u0061k'"
@@ -2918,9 +2891,6 @@
default: 'Test262Error: Expected a SyntaxError to be thrown but no exception was thrown at all'
test/language/expressions/object/method-definition/meth-eval-var-scope-syntax-err.js:
default: 'Test262Error: Expected a SyntaxError to be thrown but no exception was thrown at all'
-test/language/expressions/object/object-spread-proxy-ownkeys-returned-keys-order.js:
- default: 'Test262Error: Expected [foo, 0, Symbol()] and [Symbol(), foo, 0] to have the same contents. '
- strict mode: 'Test262Error: Expected [foo, 0, Symbol()] and [Symbol(), foo, 0] to have the same contents. '
test/language/expressions/object/scope-gen-meth-body-lex-distinct.js:
default: 'Test262Error: Expected a SyntaxError to be thrown but no exception was thrown at all'
test/language/expressions/object/scope-gen-meth-param-rest-elem-var-open.js:
Modified: trunk/Source/_javascript_Core/ChangeLog (267039 => 267040)
--- trunk/Source/_javascript_Core/ChangeLog 2020-09-14 21:00:54 UTC (rev 267039)
+++ trunk/Source/_javascript_Core/ChangeLog 2020-09-14 21:30:25 UTC (rev 267040)
@@ -1,5 +1,32 @@
2020-09-14 Alexey Shvayka <[email protected]>
+ Proxy's "ownKeys" trap result should not be sorted
+ https://bugs.webkit.org/show_bug.cgi?id=216227
+
+ Reviewed by Yusuke Suzuki.
+
+ Given that we can't know whether ownPropertyKeys() received property names from
+ userland Proxy's "ownKeys" trap, this patch moves symbols after strings sorting [1]
+ to Structure::getPropertyNamesFromStructure(), aligning observed property order
+ (via Proxy's "getOwnPropertyDescriptor" trap) with V8 and SpiderMonkey.
+
+ Also, removes sorting logic duplication in objectConstructorAssign().
+
+ This change is neutral on provided Reflect.ownKeys microbenchmark. Although property
+ name collection besides PropertyNameMode::StringsAndSymbols cases is unaffected,
+ Object.{keys,getOwnPropertySymbols} microbenchmarks regress by 6-12% due to
+ increased Structure::getPropertyNamesFromStructure() code size.
+
+ [1]: https://tc39.es/ecma262/#sec-ordinaryownpropertykeys (steps 3-4)
+
+ * runtime/ObjectConstructor.cpp:
+ (JSC::objectConstructorAssign):
+ (JSC::ownPropertyKeys):
+ * runtime/Structure.cpp:
+ (JSC::Structure::getPropertyNamesFromStructure):
+
+2020-09-14 Alexey Shvayka <[email protected]>
+
ArraySetLength should coerce [[Value]] before descriptor validation
https://bugs.webkit.org/show_bug.cgi?id=158791
Modified: trunk/Source/_javascript_Core/runtime/ObjectConstructor.cpp (267039 => 267040)
--- trunk/Source/_javascript_Core/runtime/ObjectConstructor.cpp 2020-09-14 21:00:54 UTC (rev 267039)
+++ trunk/Source/_javascript_Core/runtime/ObjectConstructor.cpp 2020-09-14 21:30:25 UTC (rev 267040)
@@ -363,14 +363,18 @@
source->methodTable(vm)->getOwnPropertyNames(source, globalObject, properties, EnumerationMode(DontEnumPropertiesMode::Include));
RETURN_IF_EXCEPTION(scope, { });
- auto assign = [&] (PropertyName propertyName) {
+ unsigned numProperties = properties.size();
+ for (unsigned j = 0; j < numProperties; j++) {
+ const auto& propertyName = properties[j];
+ ASSERT(!propertyName.isPrivateName());
+
PropertySlot slot(source, PropertySlot::InternalMethodType::GetOwnProperty);
bool hasProperty = source->methodTable(vm)->getOwnPropertySlot(source, globalObject, propertyName, slot);
- RETURN_IF_EXCEPTION(scope, void());
+ RETURN_IF_EXCEPTION(scope, { });
if (!hasProperty)
- return;
+ continue;
if (slot.attributes() & PropertyAttribute::DontEnum)
- return;
+ continue;
JSValue value;
if (LIKELY(!slot.isTaintedByOpaqueObject()))
@@ -377,37 +381,12 @@
value = slot.getValue(globalObject, propertyName);
else
value = source->get(globalObject, propertyName);
- RETURN_IF_EXCEPTION(scope, void());
+ RETURN_IF_EXCEPTION(scope, { });
PutPropertySlot putPropertySlot(target, true);
target->putInline(globalObject, propertyName, value, putPropertySlot);
- };
-
- // First loop is for strings. Second loop is for symbols to keep standardized order requirement in the spec.
- // https://tc39.github.io/ecma262/#sec-ordinaryownpropertykeys
- bool foundSymbol = false;
- unsigned numProperties = properties.size();
- for (unsigned j = 0; j < numProperties; j++) {
- const auto& propertyName = properties[j];
- if (propertyName.isSymbol()) {
- foundSymbol = true;
- continue;
- }
-
- assign(propertyName);
RETURN_IF_EXCEPTION(scope, { });
}
-
- if (foundSymbol) {
- for (unsigned j = 0; j < numProperties; j++) {
- const auto& propertyName = properties[j];
- if (propertyName.isSymbol()) {
- ASSERT(!propertyName.isPrivateName());
- assign(propertyName);
- RETURN_IF_EXCEPTION(scope, { });
- }
- }
- }
}
return JSValue::encode(target);
}
@@ -982,26 +961,16 @@
}
case PropertyNameMode::StringsAndSymbols: {
- Vector<Identifier, 16> propertySymbols;
size_t numProperties = properties.size();
for (size_t i = 0; i < numProperties; i++) {
const auto& identifier = properties[i];
if (identifier.isSymbol()) {
ASSERT(!identifier.isPrivateName());
- propertySymbols.append(identifier);
- continue;
- }
-
- pushDirect(globalObject, keys, jsOwnedString(vm, identifier.string()));
+ pushDirect(globalObject, keys, Symbol::create(vm, static_cast<SymbolImpl&>(*identifier.impl())));
+ } else
+ pushDirect(globalObject, keys, jsOwnedString(vm, identifier.string()));
RETURN_IF_EXCEPTION(scope, nullptr);
}
-
- // To ensure the order defined in the spec (9.1.12), we append symbols at the last elements of keys.
- for (const auto& identifier : propertySymbols) {
- pushDirect(globalObject, keys, Symbol::create(vm, static_cast<SymbolImpl&>(*identifier.impl())));
- RETURN_IF_EXCEPTION(scope, nullptr);
- }
-
break;
}
}
Modified: trunk/Source/_javascript_Core/runtime/Structure.cpp (267039 => 267040)
--- trunk/Source/_javascript_Core/runtime/Structure.cpp 2020-09-14 21:00:54 UTC (rev 267039)
+++ trunk/Source/_javascript_Core/runtime/Structure.cpp 2020-09-14 21:30:25 UTC (rev 267040)
@@ -1210,20 +1210,37 @@
return;
bool knownUnique = propertyNames.canAddKnownUniqueForStructure();
+ bool foundSymbol = false;
+
+ auto checkDontEnumAndAdd = [&](PropertyTable::iterator iter) {
+ if (!(iter->attributes & PropertyAttribute::DontEnum) || mode.includeDontEnumProperties()) {
+ if (knownUnique)
+ propertyNames.addUnchecked(iter->key);
+ else
+ propertyNames.add(iter->key);
+ }
+ };
PropertyTable::iterator end = table->end();
for (PropertyTable::iterator iter = table->begin(); iter != end; ++iter) {
ASSERT(!isQuickPropertyAccessAllowedForEnumeration() || !(iter->attributes & PropertyAttribute::DontEnum));
ASSERT(!isQuickPropertyAccessAllowedForEnumeration() || !iter->key->isSymbol());
- if (!(iter->attributes & PropertyAttribute::DontEnum) || mode.includeDontEnumProperties()) {
- if (iter->key->isSymbol() && !propertyNames.includeSymbolProperties())
+ if (iter->key->isSymbol()) {
+ foundSymbol = true;
+ if (propertyNames.propertyNameMode() != PropertyNameMode::Symbols)
continue;
- if (knownUnique)
- propertyNames.addUnchecked(iter->key);
- else
- propertyNames.add(iter->key);
}
+ checkDontEnumAndAdd(iter);
}
+
+ if (foundSymbol && propertyNames.propertyNameMode() == PropertyNameMode::StringsAndSymbols) {
+ // To ensure the order defined in the spec, we append symbols at the last elements of keys.
+ // https://tc39.es/ecma262/#sec-ordinaryownpropertykeys
+ for (PropertyTable::iterator iter = table->begin(); iter != end; ++iter) {
+ if (iter->key->isSymbol())
+ checkDontEnumAndAdd(iter);
+ }
+ }
}
void StructureFireDetail::dump(PrintStream& out) const
_______________________________________________ webkit-changes mailing list [email protected] https://lists.webkit.org/mailman/listinfo/webkit-changes
