Changeset 270664 in webkit

Timestamp:

Dec 10, 2020 8:14:18 PM (20 months ago)

Author:

Alexey Shvayka

Message:

Align DefineOwnProperty? method of mapped arguments object with the spec
​https://bugs.webkit.org/show_bug.cgi?id=219750

Reviewed by Yusuke Suzuki.

JSTests:

• test262/expectations.yaml: Mark 5 test cases as passing.

Source/JavaScriptCore:

This patch reimplements DefineOwnProperty? method to resemble the spec [1] as
closely as possible, aligning JSC with V8 and SpiderMonkey on remaining test262 cases.

Unlike the spec [2], JSC doesn't materialize mapped indices with initial values,
so putDirectIndex() is performed on the first call to handle incomplete descriptors.

Even though there is a possibility to avoid JSObject storage puts for a handful of
super rare descriptors, it's not worth the increased complexity.

[1]: ​https://tc39.es/ecma262/#sec-arguments-exotic-objects-defineownproperty-p-desc
[2]: ​https://tc39.es/ecma262/#sec-createmappedargumentsobject (step 15.b)

• runtime/GenericArgumentsInlines.h:

(JSC::GenericArguments<Type>::defineOwnProperty):

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/test262/expectations.yaml (modified) (1 diff)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/runtime/GenericArgumentsInlines.h (modified) (2 diffs)

trunk/JSTests/ChangeLog

@@ +1 @@
+2020-12-10  Alexey Shvayka  <shvaikalesh@gmail.com>
+
+        Align [[DefineOwnProperty]] method of mapped arguments object with the spec
+        https://bugs.webkit.org/show_bug.cgi?id=219750
+
+        Reviewed by Yusuke Suzuki.
+
+        * test262/expectations.yaml: Mark 5 test cases as passing.
+
 2020-12-09  Michael Catanzaro  <mcatanzaro@gnome.org>
 

trunk/JSTests/test262/expectations.yaml

@@ -908 +908 @@
   default: 'Test262Error: "und".minimize() should be "en" Expected SameValue(«en-u-va-posix», «en») to be true'
   strict mode: 'Test262Error: "und".minimize() should be "en" Expected SameValue(«en-u-va-posix», «en») to be true'
-test/language/arguments-object/mapped/nonconfigurable-nonenumerable-nonwritable-descriptors-set-by-arguments.js:
-  default: 'Test262Error: Expected obj[0] to have enumerable:false.'
-test/language/arguments-object/mapped/nonconfigurable-nonenumerable-nonwritable-descriptors-set-by-param.js:
-  default: 'Test262Error: Expected obj[0] to have enumerable:false.'
-test/language/arguments-object/mapped/nonconfigurable-nonwritable-descriptors-define-property-consecutive.js:
-  default: 'Test262Error: Expected obj[0] to have configurable:false.'
-test/language/arguments-object/mapped/nonconfigurable-nonwritable-descriptors-set-by-arguments.js:
-  default: 'Test262Error: Expected obj[0] to have configurable:false.'
-test/language/arguments-object/mapped/nonconfigurable-nonwritable-descriptors-set-by-param.js:
-  default: 'Test262Error: Expected obj[0] to have configurable:false.'
 test/language/block-scope/syntax/redeclaration/async-function-name-redeclaration-attempt-with-async-function.js:
   default: 'Test262: This statement should not be evaluated.'

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2020-12-10  Alexey Shvayka  <shvaikalesh@gmail.com>
+
+        Align [[DefineOwnProperty]] method of mapped arguments object with the spec
+        https://bugs.webkit.org/show_bug.cgi?id=219750
+
+        Reviewed by Yusuke Suzuki.
+
+        This patch reimplements [[DefineOwnProperty]] method to resemble the spec [1] as
+        closely as possible, aligning JSC with V8 and SpiderMonkey on remaining test262 cases.
+
+        Unlike the spec [2], JSC doesn't materialize mapped indices with initial values,
+        so putDirectIndex() is performed on the first call to handle incomplete descriptors.
+
+        Even though there is a possibility to avoid JSObject storage puts for a handful of
+        super rare descriptors, it's not worth the increased complexity.
+
+        [1]: https://tc39.es/ecma262/#sec-arguments-exotic-objects-defineownproperty-p-desc
+        [2]: https://tc39.es/ecma262/#sec-createmappedargumentsobject (step 15.b)
+
+        * runtime/GenericArgumentsInlines.h:
+        (JSC::GenericArguments<Type>::defineOwnProperty):
+
 2020-12-10  Tadeu Zagallo  <tzagallo@apple.com>
 

trunk/Source/JavaScriptCore/runtime/GenericArgumentsInlines.h

@@ -210 +210 @@
 }
 
+// https://tc39.es/ecma262/#sec-arguments-exotic-objects-defineownproperty-p-desc
 template<typename Type>
 bool GenericArguments<Type>::defineOwnProperty(JSObject* object, JSGlobalObject* globalObject, PropertyName ident, const PropertyDescriptor& descriptor, bool shouldThrow)
@@ -222 +223 @@
         thisObject->overrideThingsIfNecessary(globalObject);
         RETURN_IF_EXCEPTION(scope, false);
-    } else {
-        Optional<uint32_t> optionalIndex = parseIndex(ident);
-        if (optionalIndex) {
-            uint32_t index = optionalIndex.value();
-            if (!descriptor.isAccessorDescriptor() && thisObject->isMappedArgument(optionalIndex.value())) {
-                // If the property is not deleted and we are using a non-accessor descriptor, then
-                // make sure that the aliased argument sees the value.
+    } else if (Optional<uint32_t> optionalIndex = parseIndex(ident)) {
+        uint32_t index = optionalIndex.value();
+        bool isMapped = thisObject->isMappedArgument(index);
+        PropertyDescriptor newDescriptor = descriptor;
+
+        if (isMapped) {
+            if (thisObject->isModifiedArgumentDescriptor(index)) {
+                if (!descriptor.value() && descriptor.writablePresent() && !descriptor.writable())
+                    newDescriptor.setValue(thisObject->getIndexQuickly(index));
+            } else
+                thisObject->putDirectIndex(globalObject, index, thisObject->getIndexQuickly(index));
+
+            scope.assertNoException();
+        }
+
+        bool status = thisObject->defineOwnIndexedProperty(globalObject, index, newDescriptor, shouldThrow);
+        if (!status) {
+            ASSERT(!isMapped || thisObject->isModifiedArgumentDescriptor(index));
+            RELEASE_AND_RETURN(scope, false);
+        }
+
+        scope.assertNoException();
+        thisObject->setModifiedArgumentDescriptor(globalObject, index);
+        RETURN_IF_EXCEPTION(scope, false);
+
+        if (isMapped) {
+            if (descriptor.isAccessorDescriptor())
+                thisObject->unmapArgument(globalObject, index);
+            else {
                 if (descriptor.value())
                     thisObject->setIndexQuickly(vm, index, descriptor.value());
-            
-                // If the property is not deleted and we are using a non-accessor, writable,
-                // configurable and enumerable descriptor and isn't modified, then we are done.
-                // The argument continues to be aliased.
-                if (descriptor.writable() && descriptor.configurable() && descriptor.enumerable() && !thisObject->isModifiedArgumentDescriptor(index))
-                    return true;
-                
-                if (!thisObject->isModifiedArgumentDescriptor(index)) {
-                    // If it is a new entry, we need to put direct to initialize argument[i] descriptor properly
-                    JSValue value = thisObject->getIndexQuickly(index);
-                    ASSERT(value);
-                    object->putDirectMayBeIndex(globalObject, ident, value);
-                    scope.assertNoException();
-
-                    thisObject->setModifiedArgumentDescriptor(globalObject, index);
-                    RETURN_IF_EXCEPTION(scope, false);
-                }
+                if (descriptor.writablePresent() && !descriptor.writable())
+                    thisObject->unmapArgument(globalObject, index);
             }
-            
-            if (thisObject->isMappedArgument(index)) {
-                // Just unmap arguments if its descriptor contains {writable: false}.
-                // Check https://tc39.github.io/ecma262/#sec-createunmappedargumentsobject
-                // and https://tc39.github.io/ecma262/#sec-createmappedargumentsobject to verify that all data
-                // property from arguments object are {writable: true, configurable: true, enumerable: true} by default
-                if ((descriptor.writablePresent() && !descriptor.writable()) || descriptor.isAccessorDescriptor()) {
-                    if (!descriptor.isAccessorDescriptor()) {
-                        JSValue value = thisObject->getIndexQuickly(index);
-                        ASSERT(value);
-                        object->putDirectMayBeIndex(globalObject, ident, value);
-                        scope.assertNoException();
-                    }
-                    thisObject->unmapArgument(globalObject, index);
-                    RETURN_IF_EXCEPTION(scope, false);
-                    thisObject->setModifiedArgumentDescriptor(globalObject, index);
-                    RETURN_IF_EXCEPTION(scope, false);
-                }
-            }
-        }
-    }
-
-    // Now just let the normal object machinery do its thing.
+
+            RETURN_IF_EXCEPTION(scope, false);
+        }
+
+        return true;
+    }
+
     RELEASE_AND_RETURN(scope, Base::defineOwnProperty(object, globalObject, ident, descriptor, shouldThrow));
 }