Attachment 408139 Details for Bug 216227 – Patch

[patch] Patch

bug-216227-20200907012319.patch (text/plain), 17.30 KB, created by Alexey Shvayka on 2020-09-06 15:23:20 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Alexey Shvayka

Created: 2020-09-06 15:23:20 PDT

Size: 17.30 KB

patch

obsolete

>Index: JSTests/ChangeLog
>===================================================================
>--- JSTests/ChangeLog	(revision 266681)
>+++ JSTests/ChangeLog	(working copy)
>@@ -1,3 +1,16 @@
>+2020-09-06  Alexey Shvayka  <shvaikalesh@gmail.com>
>+
>+        Proxy's "ownKeys" trap result should not be sorted
>+        https://bugs.webkit.org/show_bug.cgi?id=216227
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * 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-04  Yusuke Suzuki  <ysuzuki@apple.com>
> 
>         [JSC] Align legacy Intl constructor behavior to spec
>Index: JSTests/microbenchmarks/object-get-own-property-symbols-on-large-array.js
>===================================================================
>--- JSTests/microbenchmarks/object-get-own-property-symbols-on-large-array.js	(revision 266637)
>+++ JSTests/microbenchmarks/object-get-own-property-symbols-on-large-array.js	(working copy)
>@@ -3,12 +3,13 @@ function trial()
> {
>     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();
>Index: JSTests/microbenchmarks/object-get-own-property-symbols.js
>===================================================================
>--- JSTests/microbenchmarks/object-get-own-property-symbols.js	(revision 266637)
>+++ JSTests/microbenchmarks/object-get-own-property-symbols.js	(working copy)
>@@ -10,5 +10,5 @@ function test(object)
> }
> noInline(test);
> 
>-for (var i = 0; i < 1e3; ++i)
>+for (var i = 0; i < 2500; ++i)
>     test(object);
>Index: JSTests/microbenchmarks/reflect-own-keys.js
>===================================================================
>--- JSTests/microbenchmarks/reflect-own-keys.js	(nonexistent)
>+++ JSTests/microbenchmarks/reflect-own-keys.js	(working copy)
>@@ -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);
>Index: JSTests/stress/proxy-own-keys.js
>===================================================================
>--- JSTests/stress/proxy-own-keys.js	(revision 266637)
>+++ JSTests/stress/proxy-own-keys.js	(working copy)
>@@ -482,7 +482,7 @@ function shallowEq(a, b) {
>     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;
>     }
>Index: JSTests/test262/expectations.yaml
>===================================================================
>--- JSTests/test262/expectations.yaml	(revision 266637)
>+++ JSTests/test262/expectations.yaml	(working copy)
>@@ -786,21 +786,9 @@ test/built-ins/Function/prototype/toStri
> 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/DefineOw
> 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/return-no
> 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 (`á³º`)'
>@@ -2379,9 +2355,6 @@ test/language/expressions/object/covered
> 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'"
>@@ -2638,9 +2611,6 @@ test/language/expressions/object/method-
>   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:
>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 266641)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,29 @@
>+2020-09-06  Alexey Shvayka  <shvaikalesh@gmail.com>
>+
>+        Proxy's "ownKeys" trap result should not be sorted
>+        https://bugs.webkit.org/show_bug.cgi?id=216227
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        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-ish on uncached Object.getOwnPropertySymbols() and
>+        Reflect.ownKeys() microbenchmarks and doesn't affect property name collection
>+        besides PropertyNameMode::StringsAndSymbols cases.
>+
>+        [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-04  Alexey Shvayka  <shvaikalesh@gmail.com>
> 
>         Array.prototype.push should always perform [[Set]] in strict mode
>Index: Source/JavaScriptCore/runtime/ObjectConstructor.cpp
>===================================================================
>--- Source/JavaScriptCore/runtime/ObjectConstructor.cpp	(revision 266637)
>+++ Source/JavaScriptCore/runtime/ObjectConstructor.cpp	(working copy)
>@@ -363,51 +363,30 @@ EncodedJSValue JSC_HOST_CALL objectConst
>         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.isSymbol() ? !propertyName.isPrivateName() : true);
>+
>             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()))
>                 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 @@ JSArray* ownPropertyKeys(JSGlobalObject*
>     }
> 
>     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;
>     }
>     }
>Index: Source/JavaScriptCore/runtime/Structure.cpp
>===================================================================
>--- Source/JavaScriptCore/runtime/Structure.cpp	(revision 266637)
>+++ Source/JavaScriptCore/runtime/Structure.cpp	(working copy)
>@@ -1177,18 +1177,35 @@ void Structure::getPropertyNamesFromStru
>         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);
>         }
>     }
> }

Actions: View | Formatted Diff | Diff

Attachments on bug 216227: 408127 | 408139 | 408677