Changeset 260447 in webkit

Timestamp:

Apr 21, 2020 12:18:36 PM (4 years ago)

Author:

Alexey Shvayka

Message:

constructObjectFromPropertyDescriptor() is incorrect with partial descriptors
​https://bugs.webkit.org/show_bug.cgi?id=184629

Reviewed by Ross Kirsling.

JSTests:

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

Source/JavaScriptCore:

Before this change, constructObjectFromPropertyDescriptor() serialized a value-only descriptor
with nullish m_seenAttributes to {value, writable: false, enumerable: false, configurable: false}
instead of just {value}. This was observable when ordinarySetSlow() was called on a Proxy
receiver with "defineProperty" trap.

This patch makes constructObjectFromPropertyDescriptor() 1:1 with the spec [2], aligning JSC
with V8 and SpiderMonkey, and also cleans up its call sites from handling exceptions and
undefined value returns.

[1]: ​https://tc39.es/ecma262/#sec-ordinarysetwithowndescriptor (step 3.d.iv)
[2]: ​https://tc39.es/ecma262/#sec-frompropertydescriptor

• runtime/ObjectConstructor.cpp:

(JSC::objectConstructorGetOwnPropertyDescriptor):
(JSC::objectConstructorGetOwnPropertyDescriptors):

• runtime/ObjectConstructor.h:

(JSC::constructObjectFromPropertyDescriptor):

• runtime/ProxyObject.cpp:

(JSC::ProxyObject::performDefineOwnProperty):

Location:

trunk

Files:

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

trunk/JSTests/ChangeLog

@@ +1 @@
+2020-04-21  Alexey Shvayka  <shvaikalesh@gmail.com>
+
+        constructObjectFromPropertyDescriptor() is incorrect with partial descriptors
+        https://bugs.webkit.org/show_bug.cgi?id=184629
+
+        Reviewed by Ross Kirsling.
+
+        * test262/expectations.yaml: Mark 4 test cases as passing.
+
 2020-04-21  Yusuke Suzuki  <ysuzuki@apple.com>
 

trunk/JSTests/test262/expectations.yaml

@@ -1247 +1247 @@
   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/Proxy/set/trap-is-missing-receiver-multiple-calls-index.js:
-  default: 'Test262Error: Expected [0] and [0, 0, 0] to have the same contents. getOwnPropertyDescriptor: key present on [[ProxyTarget]]'
-  strict mode: 'TypeError: Attempted to assign to readonly property.'
-test/built-ins/Proxy/set/trap-is-missing-receiver-multiple-calls.js:
-  default: 'Test262Error: Expected [foo] and [foo, foo, foo] to have the same contents. getOwnPropertyDescriptor: key present on [[ProxyTarget]]'
-  strict mode: 'TypeError: Attempted to assign to readonly property.'
 test/built-ins/Reflect/ownKeys/order-after-define-property.js:
   default: 'Test262Error: Expected [Symbol(b), Symbol(a)] and [Symbol(a), Symbol(b)] to have the same contents. '

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2020-04-21  Alexey Shvayka  <shvaikalesh@gmail.com>
+
+        constructObjectFromPropertyDescriptor() is incorrect with partial descriptors
+        https://bugs.webkit.org/show_bug.cgi?id=184629
+
+        Reviewed by Ross Kirsling.
+
+        Before this change, constructObjectFromPropertyDescriptor() serialized a value-only descriptor
+        with nullish m_seenAttributes to {value, writable: false, enumerable: false, configurable: false}
+        instead of just {value}. This was observable when ordinarySetSlow() was called on a Proxy
+        `receiver` with "defineProperty" trap.
+
+        This patch makes constructObjectFromPropertyDescriptor() 1:1 with the spec [2], aligning JSC
+        with V8 and SpiderMonkey, and also cleans up its call sites from handling exceptions and
+        `undefined` value returns.
+
+        [1]: https://tc39.es/ecma262/#sec-ordinarysetwithowndescriptor (step 3.d.iv)
+        [2]: https://tc39.es/ecma262/#sec-frompropertydescriptor
+
+        * runtime/ObjectConstructor.cpp:
+        (JSC::objectConstructorGetOwnPropertyDescriptor):
+        (JSC::objectConstructorGetOwnPropertyDescriptors):
+        * runtime/ObjectConstructor.h:
+        (JSC::constructObjectFromPropertyDescriptor):
+        * runtime/ProxyObject.cpp:
+        (JSC::ProxyObject::performDefineOwnProperty):
+
 2020-04-20  Yusuke Suzuki  <ysuzuki@apple.com>
 

trunk/Source/JavaScriptCore/runtime/ObjectConstructor.cpp

@@ -192 +192 @@
 
     JSObject* result = constructObjectFromPropertyDescriptor(globalObject, descriptor);
-    EXCEPTION_ASSERT(!!scope.exception() == !result);
-    if (!result)
-        return jsUndefined();
+    scope.assertNoException();
+    ASSERT(result);
     return result;
 }
@@ -218 +217 @@
 
         JSObject* fromDescriptor = constructObjectFromPropertyDescriptor(globalObject, descriptor);
-        EXCEPTION_ASSERT(!!scope.exception() == !fromDescriptor);
-        if (!fromDescriptor)
-            return jsUndefined();
+        scope.assertNoException();
+        ASSERT(fromDescriptor);
 
         PutPropertySlot slot(descriptors);

trunk/Source/JavaScriptCore/runtime/ObjectConstructor.h

@@ -90 +90 @@
 }
 
-// Section 6.2.4.4 of the ES6 specification.
-// https://tc39.github.io/ecma262/#sec-frompropertydescriptor
+// https://tc39.es/ecma262/#sec-frompropertydescriptor
 inline JSObject* constructObjectFromPropertyDescriptor(JSGlobalObject* globalObject, const PropertyDescriptor& descriptor)
 {
     VM& vm = getVM(globalObject);
-    auto scope = DECLARE_THROW_SCOPE(vm);
-    JSObject* description = constructEmptyObject(globalObject);
-    RETURN_IF_EXCEPTION(scope, nullptr);
+    JSObject* result = constructEmptyObject(globalObject);
 
-    if (!descriptor.isAccessorDescriptor()) {
-        description->putDirect(vm, vm.propertyNames->value, descriptor.value() ? descriptor.value() : jsUndefined(), 0);
-        description->putDirect(vm, vm.propertyNames->writable, jsBoolean(descriptor.writable()), 0);
-    } else {
-        ASSERT(descriptor.getter() || descriptor.setter());
-        if (descriptor.getter())
-            description->putDirect(vm, vm.propertyNames->get, descriptor.getter(), 0);
-        if (descriptor.setter())
-            description->putDirect(vm, vm.propertyNames->set, descriptor.setter(), 0);
-    }
-    
-    description->putDirect(vm, vm.propertyNames->enumerable, jsBoolean(descriptor.enumerable()), 0);
-    description->putDirect(vm, vm.propertyNames->configurable, jsBoolean(descriptor.configurable()), 0);
+    if (descriptor.value())
+        result->putDirect(vm, vm.propertyNames->value, descriptor.value());
+    if (descriptor.writablePresent())
+        result->putDirect(vm, vm.propertyNames->writable, jsBoolean(descriptor.writable()));
+    if (descriptor.getterPresent())
+        result->putDirect(vm, vm.propertyNames->get, descriptor.getter());
+    if (descriptor.setterPresent())
+        result->putDirect(vm, vm.propertyNames->set, descriptor.setter());
+    if (descriptor.enumerablePresent())
+        result->putDirect(vm, vm.propertyNames->enumerable, jsBoolean(descriptor.enumerable()));
+    if (descriptor.configurablePresent())
+        result->putDirect(vm, vm.propertyNames->configurable, jsBoolean(descriptor.configurable()));
 
-    return description;
+    return result;
 }
 

trunk/Source/JavaScriptCore/runtime/ProxyObject.cpp

@@ -853 +853 @@
 
     JSObject* descriptorObject = constructObjectFromPropertyDescriptor(globalObject, descriptor);
-    RETURN_IF_EXCEPTION(scope, false);
+    scope.assertNoException();
+    ASSERT(descriptorObject);
 
     MarkedArgumentBuffer arguments;