Changeset 247811 in webkit

Timestamp:

Jul 24, 2019 7:09:42 PM (5 years ago)

Author:

commit-queue@webkit.org

Message:

Three checks are missing in Proxy internal methods
​https://bugs.webkit.org/show_bug.cgi?id=198630

Patch by Alexey Shvayka <Alexey Shvayka> on 2019-07-24
Reviewed by Darin Adler.

JSTests:

• stress/proxy-delete.js: Assert isExtensible is called in correct order.
• test262/expectations.yaml: Mark 6 test cases as passing.

Source/JavaScriptCore:

Add three missing checks in Proxy internal methods.
These checks are necessary to maintain the invariants of the essential internal methods.
(​https://github.com/tc39/ecma262/pull/666)

1. GetOwnProperty? shouldn't return non-configurable and non-writable descriptor when the target's property is writable.
2. Delete? should return false when the target has property and is not extensible.
3. DefineOwnProperty? should return true for a non-writable input descriptor when the target's property is non-configurable and writable.

Shipping in SpiderMonkey since ​https://hg.mozilla.org/integration/autoland/rev/3a06bc818bc4 (version 69)
Shipping in V8 since ​https://chromium.googlesource.com/v8/v8.git/+/e846ad9fa5109428be50b1989314e0e4e7267919

• runtime/ProxyObject.cpp:

(JSC::ProxyObject::performInternalMethodGetOwnProperty): Add writability check.
(JSC::ProxyObject::performDelete): Add extensibility check.
(JSC::ProxyObject::performDefineOwnProperty): Add writability check.

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/stress/proxy-delete.js (modified) (1 diff)
• JSTests/test262/expectations.yaml (modified) (1 diff)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/runtime/ProxyObject.cpp (modified) (3 diffs)

trunk/JSTests/ChangeLog

@@ +1 @@
+2019-07-24  Alexey Shvayka  <shvaikalesh@gmail.com>
+
+        Three checks are missing in Proxy internal methods
+        https://bugs.webkit.org/show_bug.cgi?id=198630
+
+        Reviewed by Darin Adler.
+
+        * stress/proxy-delete.js: Assert isExtensible is called in correct order.
+        * test262/expectations.yaml: Mark 6 test cases as passing.
+
 2019-07-23  Justin Michaud  <justin_michaud@apple.com>
 

trunk/JSTests/stress/proxy-delete.js

@@ -90 +90 @@
         }
         assert(threw);
+    }
+}
+
+{
+    let target = new Proxy({}, {
+        isExtensible: function() {
+            throw new Error("should not be called if [[GetOwnProperty]] returns undefined");
+        }
+    });
+    let proxy = new Proxy(target, {
+        deleteProperty: function() {
+            return true;
+        }
+    });
+    for (let i = 0; i < 500; i++) {
+        assert(delete proxy.nonExistentProperty);
+    }
+}
+
+{
+    let calls;
+    let target = new Proxy({}, {
+        getOwnPropertyDescriptor: function() {
+            calls.push('getOwnPropertyDescriptor');
+            return { configurable: true };
+        },
+        isExtensible: function() {
+            calls.push('isExtensible');
+            return true;
+        }
+    });
+    let proxy = new Proxy(target, {
+        deleteProperty: function() {
+            calls.push('trap');
+            return true;
+        }
+    });
+    for (let i = 0; i < 500; i++) {
+        calls = [];
+        assert(delete proxy.prop);
+        assert(calls.join() == 'trap,getOwnPropertyDescriptor,isExtensible');
     }
 }

trunk/JSTests/test262/expectations.yaml

@@ -1344 +1344 @@
   strict mode: 'Test262Error: Expected a TypeError but got a TypeError'
 test/built-ins/Proxy/create-handler-is-revoked-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/defineProperty/targetdesc-not-configurable-writable-desc-not-writable.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/deleteProperty/targetdesc-is-configurable-target-is-not-extensible.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/resultdesc-is-not-configurable-not-writable-targetdesc-is-writable.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'

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2019-07-24  Alexey Shvayka  <shvaikalesh@gmail.com>
+
+        Three checks are missing in Proxy internal methods
+        https://bugs.webkit.org/show_bug.cgi?id=198630
+
+        Reviewed by Darin Adler.
+
+        Add three missing checks in Proxy internal methods.
+        These checks are necessary to maintain the invariants of the essential internal methods.
+        (https://github.com/tc39/ecma262/pull/666)
+
+        1. [[GetOwnProperty]] shouldn't return non-configurable and non-writable descriptor when the target's property is writable.
+        2. [[Delete]] should return `false` when the target has property and is not extensible.
+        3. [[DefineOwnProperty]] should return `true` for a non-writable input descriptor when the target's property is non-configurable and writable.
+
+        Shipping in SpiderMonkey since https://hg.mozilla.org/integration/autoland/rev/3a06bc818bc4 (version 69)
+        Shipping in V8 since https://chromium.googlesource.com/v8/v8.git/+/e846ad9fa5109428be50b1989314e0e4e7267919
+
+        * runtime/ProxyObject.cpp:
+        (JSC::ProxyObject::performInternalMethodGetOwnProperty): Add writability check.
+        (JSC::ProxyObject::performDelete): Add extensibility check.
+        (JSC::ProxyObject::performDefineOwnProperty): Add writability check.
+
 2019-07-24  Mark Lam  <mark.lam@apple.com>
 

trunk/Source/JavaScriptCore/runtime/ProxyObject.cpp

@@ -288 +288 @@
             return false;
         }
+        if (trapResultAsDescriptor.writablePresent() && !trapResultAsDescriptor.writable() && targetPropertyDescriptor.writable()) {
+            throwVMTypeError(exec, scope, "Result from 'getOwnPropertyDescriptor' can't be non-configurable and non-writable when the target's property is writable"_s);
+            return false;
+        }
     }
 
@@ -663 +667 @@
             return false;
         }
+        bool targetIsExtensible = target->isExtensible(exec);
+        RETURN_IF_EXCEPTION(scope, false);
+        if (!targetIsExtensible) {
+            throwVMTypeError(exec, scope, "Proxy handler's 'deleteProperty' method should return false when the target has property and is not extensible"_s);
+            return false;
+        }
     }
 
@@ -886 +896 @@
         throwVMTypeError(exec, scope, "Proxy's 'defineProperty' trap did not define a non-configurable property on its target even though the input descriptor to the trap said it must do so"_s);
         return false;
+    }
+    if (targetDescriptor.isDataDescriptor() && !targetDescriptor.configurable() && targetDescriptor.writable()) {
+        if (descriptor.writablePresent() && !descriptor.writable()) {
+            throwTypeError(exec, scope, "Proxy's 'defineProperty' trap returned true for a non-writable input descriptor when the target's property is non-configurable and writable"_s);
+            return false;
+        }
     }