Changeset 259800 in webkit

Timestamp:

Apr 9, 2020, 8:13:07 AM (6 years ago)

Author:

Alexey Shvayka

Message:

getOwnPropertyDescriptor() is incorrect with Proxy of exotic object
​https://bugs.webkit.org/show_bug.cgi?id=200560

Reviewed by Yusuke Suzuki.

JSTests:

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

Source/JavaScriptCore:

PropertyAttribute::CustomValue path in JSObject::getOwnPropertyDescriptor() needs to perform
getDirect() on correct target. A correct target may be different since getOwnPropertySlot()
may return not *own* property.

This change removes a hack that was covering only JSProxy case and invokes getDirect() on
slotBase(), ensuring ProxyObject instances with exotic targets return correct descriptors
and aligning JSC with V8 and SpiderMonkey.

getDirect() can be safely called on slotBase(): if getOwnPropertySlot() result is returned
from JS code of ProxyObject's trap, it will never be a PropertyAttribute::CustomValue.

This patch also moves setCustomDescriptor() down below to avoid mutating a descriptor when
returning false.

• runtime/JSObject.cpp:

(JSC::JSObject::getOwnPropertyDescriptor):

LayoutTests:

• js/getOwnPropertyDescriptor-host-object-proxy-expected.txt: Added.
• js/getOwnPropertyDescriptor-host-object-proxy.html: Added.

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/test262/expectations.yaml (modified) (3 diffs)
• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/js/getOwnPropertyDescriptor-host-object-proxy-expected.txt (added)
• LayoutTests/js/getOwnPropertyDescriptor-host-object-proxy.html (added)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/runtime/JSObject.cpp (modified) (3 diffs)

trunk/JSTests/ChangeLog

@@ +1 @@
+2020-04-09  Alexey Shvayka  <shvaikalesh@gmail.com>
+
+        getOwnPropertyDescriptor() is incorrect with Proxy of exotic object
+        https://bugs.webkit.org/show_bug.cgi?id=200560
+
+        Reviewed by Yusuke Suzuki.
+
+        * test262/expectations.yaml: Mark 14 test cases as passing.
+
 2020-04-08  Yusuke Suzuki  <ysuzuki@apple.com>
 

trunk/JSTests/test262/expectations.yaml

@@ -673 +673 @@
   default: 'Test262Error: Length is 2**53 - 1 Expected SameValue(«4294967295», «9007199254740991») to be true'
   strict mode: 'Test262Error: Length is 2**53 - 1 Expected SameValue(«4294967295», «9007199254740991») to be true'
-test/built-ins/Array/prototype/splice/create-proxy.js:
-  default: 'TypeError: Attempted to assign to readonly property.'
-  strict mode: 'TypeError: Attempted to assign to readonly property.'
 test/built-ins/Array/prototype/splice/create-species-length-exceeding-integer-limit.js:
   default: 'Test262Error: length and deleteCount were correctly clamped to 2^53-1 Expected SameValue(«4294967295», «9007199254740991») to be true'
@@ -1256 +1253 @@
   default: 'Test262Error: Expected a TypeError to be thrown but no exception was thrown at all'
   strict mode: 'Test262Error: Expected a TypeError to be thrown but no exception was thrown at all'
-test/built-ins/Proxy/getOwnPropertyDescriptor/trap-is-missing-target-is-proxy.js:
-  default: "TypeError: undefined is not an object (evaluating 'originalDesc.enumerable')"
-  strict mode: "TypeError: undefined is not an object (evaluating 'originalDesc.enumerable')"
-test/built-ins/Proxy/getOwnPropertyDescriptor/trap-is-null-target-is-proxy.js:
-  default: "TypeError: undefined is not an object (evaluating 'originalDesc.value')"
-  strict mode: "TypeError: undefined is not an object (evaluating 'originalDesc.value')"
-test/built-ins/Proxy/getOwnPropertyDescriptor/trap-is-undefined-target-is-proxy.js:
-  default: "TypeError: undefined is not an object (evaluating 'originalDesc.value')"
-  strict mode: "TypeError: undefined is not an object (evaluating 'originalDesc.value')"
 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. '
@@ -1274 +1262 @@
   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/Proxy/set/trap-is-missing-target-is-proxy.js:
-  default: 'Test262Error: Expected SameValue(«0», «1») to be true'
-  strict mode: 'TypeError: Attempting to change configurable attribute of unconfigurable property.'
-test/built-ins/Proxy/set/trap-is-null-target-is-proxy.js:
-  default: 'Test262Error: Expected [1, 2, 3] and [] to have the same contents. '
-  strict mode: 'TypeError: Attempting to change configurable attribute of unconfigurable property.'
-test/built-ins/Proxy/set/trap-is-undefined-target-is-proxy.js:
-  default: 'Test262Error: Expected true but got false'
-  strict mode: 'Test262Error: Expected true but got false'
 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/LayoutTests/ChangeLog

@@ +1 @@
+2020-04-09  Alexey Shvayka  <shvaikalesh@gmail.com>
+
+        getOwnPropertyDescriptor() is incorrect with Proxy of exotic object
+        https://bugs.webkit.org/show_bug.cgi?id=200560
+
+        Reviewed by Yusuke Suzuki.
+
+        * js/getOwnPropertyDescriptor-host-object-proxy-expected.txt: Added.
+        * js/getOwnPropertyDescriptor-host-object-proxy.html: Added.
+
 2020-04-09  Diego Pino Garcia  <dpino@igalia.com>
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2020-04-09  Alexey Shvayka  <shvaikalesh@gmail.com>
+
+        getOwnPropertyDescriptor() is incorrect with Proxy of exotic object
+        https://bugs.webkit.org/show_bug.cgi?id=200560
+
+        Reviewed by Yusuke Suzuki.
+
+        PropertyAttribute::CustomValue path in JSObject::getOwnPropertyDescriptor() needs to perform
+        getDirect() on correct target. A correct target may be different since getOwnPropertySlot()
+        may return not *own* property.
+
+        This change removes a hack that was covering only JSProxy case and invokes getDirect() on
+        slotBase(), ensuring ProxyObject instances with exotic targets return correct descriptors
+        and aligning JSC with V8 and SpiderMonkey.
+
+        getDirect() can be safely called on slotBase(): if getOwnPropertySlot() result is returned
+        from JS code of ProxyObject's trap, it will never be a PropertyAttribute::CustomValue.
+
+        This patch also moves setCustomDescriptor() down below to avoid mutating a descriptor when
+        returning `false`.
+
+        * runtime/JSObject.cpp:
+        (JSC::JSObject::getOwnPropertyDescriptor):
+
 2020-04-09  Angelos Oikonomopoulos  <angelos@igalia.com>
 

trunk/Source/JavaScriptCore/runtime/JSObject.cpp

@@ -3480 +3480 @@
     VM& vm = globalObject->vm();
     auto scope = DECLARE_THROW_SCOPE(vm);
-    JSC::PropertySlot slot(this, PropertySlot::InternalMethodType::GetOwnProperty);
+    PropertySlot slot(this, PropertySlot::InternalMethodType::GetOwnProperty);
 
     bool result = methodTable(vm)->getOwnPropertySlot(this, globalObject, propertyName, slot);
@@ -3487 +3487 @@
         return false;
 
-
-    // FIXME: https://bugs.webkit.org/show_bug.cgi?id=200560
-    // This breaks the assumption that getOwnPropertySlot should return "own" property.
-    // We should fix DebuggerScope, ProxyObject etc. to remove this.
-    //
-    // DebuggerScope::getOwnPropertySlot() (and possibly others) may return attributes from the prototype chain
-    // but getOwnPropertyDescriptor() should only work for 'own' properties so we exit early if we detect that
-    // the property is not an own property.
-    if (slot.slotBase() != this && slot.slotBase()) {
-        JSProxy* jsProxy = jsDynamicCast<JSProxy*>(vm, this);
-        if (!jsProxy || jsProxy->target() != slot.slotBase()) {
-            // Try ProxyObject.
-            ProxyObject* proxyObject = jsDynamicCast<ProxyObject*>(vm, this);
-            if (!proxyObject || proxyObject->target() != slot.slotBase())
-                return false;
-        }
-    }
-
     if (slot.isAccessor())
         descriptor.setAccessorDescriptor(slot.getterSetter(), slot.attributes());
     else if (slot.attributes() & PropertyAttribute::CustomAccessor) {
-        descriptor.setCustomDescriptor(slot.attributes());
-
-        JSObject* thisObject = this;
-        if (auto* proxy = jsDynamicCast<JSProxy*>(vm, this))
-            thisObject = proxy->target();
-
         CustomGetterSetter* getterSetter;
         if (slot.isCustomAccessor())
             getterSetter = slot.customGetterSetter();
         else {
+            ASSERT(slot.slotBase());
+            JSObject* thisObject = slot.slotBase();
+
             JSValue maybeGetterSetter = thisObject->getDirect(vm, propertyName);
             if (!maybeGetterSetter) {
@@ -3531 +3510 @@
             return false;
 
+        descriptor.setCustomDescriptor(slot.attributes());
         if (getterSetter->getter())
             descriptor.setGetter(getCustomGetterSetterFunctionForGetterSetter(globalObject, propertyName, getterSetter, JSCustomGetterSetterFunction::Type::Getter));