Attachment 395885 Details for Bug 200560 – Patch

[patch] Patch

bug-200560-20200409023229.patch (text/plain), 13.50 KB, created by Alexey Shvayka on 2020-04-08 16:32:31 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Alexey Shvayka

Created: 2020-04-08 16:32:31 PDT

Size: 13.50 KB

patch

obsolete

>Index: JSTests/ChangeLog
>===================================================================
>--- JSTests/ChangeLog	(revision 259763)
>+++ JSTests/ChangeLog	(working copy)
>@@ -1,3 +1,12 @@
>+2020-04-08  Alexey Shvayka  <shvaikalesh@gmail.com>
>+
>+        getOwnPropertyDescriptor() is incorrect with Proxy of exotic object
>+        https://bugs.webkit.org/show_bug.cgi?id=200560
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * test262/expectations.yaml: Mark 14 test cases as passing.
>+
> 2020-04-08  Yusuke Suzuki  <ysuzuki@apple.com>
> 
>         [JSC] MultiDeleteByOffset should tell correct result AbstractValue in AI
>Index: JSTests/test262/expectations.yaml
>===================================================================
>--- JSTests/test262/expectations.yaml	(revision 259682)
>+++ JSTests/test262/expectations.yaml	(working copy)
>@@ -672,9 +672,6 @@ test/built-ins/Array/prototype/splice/S1
> test/built-ins/Array/prototype/splice/clamps-length-to-integer-limit.js:
>   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'
>   strict mode: 'Test262Error: length and deleteCount were correctly clamped to 2^53-1 Expected SameValue(Â«4294967295Â», Â«9007199254740991Â») to be true'
>@@ -1255,15 +1252,6 @@ test/built-ins/Proxy/create-target-is-re
> test/built-ins/Proxy/defineProperty/trap-is-undefined-target-is-proxy.js:
>   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. '
>   strict mode: 'Test262Error: Expected [length, foo, 0, Symbol()] and [Symbol(), length, foo, 0] to have the same contents. '
>@@ -1273,15 +1261,6 @@ test/built-ins/Proxy/set/trap-is-missing
> 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/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. '
>   strict mode: 'Test262Error: Expected [Symbol(b), Symbol(a)] and [Symbol(a), Symbol(b)] to have the same contents. '
>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 259682)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,27 @@
>+2020-04-08  Alexey Shvayka  <shvaikalesh@gmail.com>
>+
>+        getOwnPropertyDescriptor() is incorrect with Proxy of exotic object
>+        https://bugs.webkit.org/show_bug.cgi?id=200560
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        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-07  Saam Barati  <sbarati@apple.com>
> 
>         Delete ICs can't cache dictionaries
>Index: Source/JavaScriptCore/runtime/JSObject.cpp
>===================================================================
>--- Source/JavaScriptCore/runtime/JSObject.cpp	(revision 259682)
>+++ Source/JavaScriptCore/runtime/JSObject.cpp	(working copy)
>@@ -3479,44 +3479,23 @@ bool JSObject::getOwnPropertyDescriptor(
> {
>     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);
>     EXCEPTION_ASSERT(!scope.exception() || !result);
>     if (!result)
>         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) {
>                 thisObject->reifyAllStaticProperties(globalObject);
>@@ -3530,6 +3509,7 @@ bool JSObject::getOwnPropertyDescriptor(
>         if (!getterSetter)
>             return false;
> 
>+        descriptor.setCustomDescriptor(slot.attributes());
>         if (getterSetter->getter())
>             descriptor.setGetter(getCustomGetterSetterFunctionForGetterSetter(globalObject, propertyName, getterSetter, JSCustomGetterSetterFunction::Type::Getter));
>         if (getterSetter->setter())
>Index: LayoutTests/ChangeLog
>===================================================================
>--- LayoutTests/ChangeLog	(revision 259682)
>+++ LayoutTests/ChangeLog	(working copy)
>@@ -1,3 +1,13 @@
>+2020-04-08  Alexey Shvayka  <shvaikalesh@gmail.com>
>+
>+        getOwnPropertyDescriptor() is incorrect with Proxy of exotic object
>+        https://bugs.webkit.org/show_bug.cgi?id=200560
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * js/getOwnPropertyDescriptor-host-object-proxy-expected.txt: Added.
>+        * js/getOwnPropertyDescriptor-host-object-proxy.html: Added.
>+
> 2020-04-07  Jacob Uphoff  <jacob_uphoff@apple.com>
> 
>         [ iOS wk2 ] http/wpt/cache-storage/cache-put-keys.https.any.html is flaky failing.
>Index: LayoutTests/js/getOwnPropertyDescriptor-host-object-proxy-expected.txt
>===================================================================
>--- LayoutTests/js/getOwnPropertyDescriptor-host-object-proxy-expected.txt	(nonexistent)
>+++ LayoutTests/js/getOwnPropertyDescriptor-host-object-proxy-expected.txt	(working copy)
>@@ -0,0 +1,9 @@
>+
>+PASS Window target, no trap, "name" attribute 
>+PASS Window target, forwarding trap, [Unforgeable] "document" attribute 
>+PASS Window proxy target, custom trap, "onclick" event handler attribute 
>+PASS Document target, forwarding trap, [Unforgeable] "location" attribute 
>+PASS Document proxy target, custom trap, non-existent value attribute 
>+PASS Location target, no trap, [Unforgeable] "hash" attribute 
>+PASS Location proxy target, forwarding trap, [Unforgeable] "reload" method 
>+
>Index: LayoutTests/js/getOwnPropertyDescriptor-host-object-proxy.html
>===================================================================
>--- LayoutTests/js/getOwnPropertyDescriptor-host-object-proxy.html	(nonexistent)
>+++ LayoutTests/js/getOwnPropertyDescriptor-host-object-proxy.html	(working copy)
>@@ -0,0 +1,92 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+  <title>getOwnPropertyDescriptor() is correct for Proxy with host object target</title>
>+  <script src="../resources/testharness.js"></script>
>+  <script src="../resources/testharnessreport.js"></script>
>+</head>
>+<body>
>+<script>
>+'use strict';
>+
>+test(() => {
>+  let windowProxy = new Proxy(window, {});
>+  name = 'old_name';
>+  let descriptor = Object.getOwnPropertyDescriptor(windowProxy, 'name');
>+
>+  assert_equals(descriptor.get.call(window), 'old_name');
>+  descriptor.set.call(window, 'new_name');
>+  assert_equals(name, 'new_name');
>+  assert_true(descriptor.enumerable);
>+  assert_true(descriptor.configurable);
>+}, 'Window target, no trap, "name" attribute');
>+
>+test(() => {
>+  let windowProxy = new Proxy(window, {});
>+
>+  assert_object_equals(
>+    Object.getOwnPropertyDescriptor(windowProxy, 'document'),
>+    Object.getOwnPropertyDescriptor(window, 'document')
>+  );
>+}, 'Window target, forwarding trap, [Unforgeable] "document" attribute');
>+
>+test(() => {
>+  let trapResult = {get() {}, set(_val) {}, enumerable: false, configurable: true};
>+  let windowProxy = new Proxy(new Proxy(window, {}), {
>+    getOwnPropertyDescriptor: () => trapResult,
>+  });
>+
>+  assert_object_equals(
>+    Object.getOwnPropertyDescriptor(windowProxy, 'onclick'),
>+    trapResult
>+  );
>+}, 'Window proxy target, custom trap, "onclick" event handler attribute');
>+
>+test(() => {
>+  let documentProxy = new Proxy(document, {
>+    getOwnPropertyDescriptor: Reflect.getOwnPropertyDescriptor,
>+  });
>+
>+  assert_object_equals(
>+    Object.getOwnPropertyDescriptor(documentProxy, 'location'),
>+    Object.getOwnPropertyDescriptor(document, 'location')
>+  );
>+}, 'Document target, forwarding trap, [Unforgeable] "location" attribute');
>+
>+test(() => {
>+  let trapResult = {value: 4, writable: false, enumerable: true, configurable: true};
>+  let documentProxy = new Proxy(new Proxy(document, {}), {
>+    getOwnPropertyDescriptor: () => trapResult,
>+  });
>+
>+  assert_object_equals(
>+    Object.getOwnPropertyDescriptor(documentProxy, 'foo'),
>+    trapResult
>+  );
>+}, 'Document proxy target, custom trap, non-existent value attribute');
>+
>+test(() => {
>+  let locationProxy = new Proxy(location, {});
>+  location.hash = '#old';
>+  let descriptor = Object.getOwnPropertyDescriptor(locationProxy, 'hash');
>+
>+  assert_equals(descriptor.get.call(location), '#old');
>+  descriptor.set.call(location, '#new');
>+  assert_equals(location.hash, '#new');
>+  assert_true(descriptor.enumerable);
>+  assert_false(descriptor.configurable);
>+}, 'Location target, no trap, [Unforgeable] "hash" attribute');
>+
>+test(() => {
>+  let locationProxy = new Proxy(new Proxy(location, {}), {
>+    getOwnPropertyDescriptor: Reflect.getOwnPropertyDescriptor,
>+  });
>+
>+  assert_object_equals(
>+    Object.getOwnPropertyDescriptor(locationProxy, 'reload'),
>+    Object.getOwnPropertyDescriptor(location, 'reload')
>+  );
>+}, 'Location proxy target, forwarding trap, [Unforgeable] "reload" method');
>+</script>
>+</body>
>+</html>

Actions: View | Formatted Diff | Diff

Attachments on bug 200560: 395703 | 395885