Changeset 244020 in webkit

Timestamp:

Apr 8, 2019 8:51:19 AM (5 years ago)

Author:

Ryan Haddad

Message:

Unreviewed, rolling out r243943.

Caused test262 failures.

Reverted changeset:

"[JSC] Filter DontEnum properties in
ProxyObject::getOwnPropertyNames()"
​https://bugs.webkit.org/show_bug.cgi?id=176810
​https://trac.webkit.org/changeset/243943

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/stress/proxy-own-keys.js (modified) (3 diffs)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/runtime/PropertyNameArray.h (modified) (1 diff)
• Source/JavaScriptCore/runtime/ProxyObject.cpp (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/bindings/js/JSDOMConvertRecord.h (modified) (2 diffs)

trunk/JSTests/ChangeLog

@@ +1 @@
+2019-04-08  Ryan Haddad  <ryanhaddad@apple.com>
+
+        Unreviewed, rolling out r243943.
+
+        Caused test262 failures.
+
+        Reverted changeset:
+
+        "[JSC] Filter DontEnum properties in
+        ProxyObject::getOwnPropertyNames()"
+        https://bugs.webkit.org/show_bug.cgi?id=176810
+        https://trac.webkit.org/changeset/243943
+
 2019-04-07  Michael Saboff  <msaboff@apple.com>
 

trunk/JSTests/stress/proxy-own-keys.js

@@ -136 +136 @@
         called = false;
     }
-
-    for (let i = 0; i < 500; i++) {
-        let threw = false;
-        let foundKey = false;
-        try {
-            for (let k in proxy)
-                foundKey = true;
-        } catch(e) {
-            threw = true;
-            assert(e.toString() === "TypeError: Proxy object's non-extensible 'target' has configurable property 'x' that was not in the result from the 'ownKeys' trap");
-            assert(!foundKey);
-        }
-        assert(threw);
-        assert(called);
-        called = false;
-    }
 }
 
@@ -181 +165 @@
         assert(threw);
         assert(called);
-        called = false;
-    }
-
-    for (let i = 0; i < 500; i++) {
-        let threw = false;
-        let reached = false;
-        try {
-            for (let k in proxy)
-                reached = true;
-        } catch (e) {
-            threw = true;
-            assert(e.toString() === "TypeError: Proxy handler's 'ownKeys' method returned a key that was not present in its non-extensible target");
-        }
-        assert(threw);
-        assert(called);
-        assert(!reached);
         called = false;
     }
@@ -700 +668 @@
     }
 }
-
-{
-    let error = null;
-    let s1 = Symbol();
-    let s2 = Symbol();
-    let target = Object.defineProperties({}, {
-        x: {
-            value: "X",
-            enumerable: true,
-            configurable: true,
-        },
-        dontEnum1: {
-            value: "dont-enum",
-            enumerable: false,
-            configurable: true,
-        },
-        y: {
-            get() { return "Y"; },
-            enumerable: true,
-            configurable: true,
-        },
-        dontEnum2: {
-            get() { return "dont-enum-accessor" },
-            enumerable: false,
-            configurable: true
-        },
-        [s1]: {
-            value: "s1",
-            enumerable: true,
-            configurable: true,
-        },
-        [s2]: {
-            value: "dont-enum-symbol",
-            enumerable: false,
-            configurable: true,  
-        },
-    });
-    let checkedOwnKeys = false;
-    let checkedPropertyDescriptor = false;
-    let handler = {
-        ownKeys() {
-            checkedOwnKeys = true;
-            return ["x", "dontEnum1", "y", "dontEnum2", s1, s2];
-        },
-        getOwnPropertyDescriptor(t, k) {
-            checkedPropertyDescriptors = true;
-            return Reflect.getOwnPropertyDescriptor(t, k);
-        }
-    };
-    let proxy = new Proxy(target, handler);
-    for (let i = 0; i < 500; i++) {
-        checkedPropertyDescriptors = false;
-        assert(shallowEq(["x", "dontEnum1", "y", "dontEnum2", s1, s2], Reflect.ownKeys(proxy)));
-        assert(checkedOwnKeys);
-        assert(!checkedPropertyDescriptors);
-        checkedOwnKeys = false;
-
-        let enumerableStringKeys = [];
-        for (let k in proxy)
-            enumerableStringKeys.push(k);
-        assert(shallowEq(["x", "y"], enumerableStringKeys));
-        assert(checkedOwnKeys);
-        assert(checkedPropertyDescriptors);
-    }
-}

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2019-04-08  Ryan Haddad  <ryanhaddad@apple.com>
+
+        Unreviewed, rolling out r243943.
+
+        Caused test262 failures.
+
+        Reverted changeset:
+
+        "[JSC] Filter DontEnum properties in
+        ProxyObject::getOwnPropertyNames()"
+        https://bugs.webkit.org/show_bug.cgi?id=176810
+        https://trac.webkit.org/changeset/243943
+
 2019-04-08  Claudio Saavedra  <csaavedra@igalia.com>
 

trunk/Source/JavaScriptCore/runtime/PropertyNameArray.h

@@ -54 +54 @@
         , m_privateSymbolMode(privateSymbolMode)
     {
-    }
-
-    void reset()
-    {
-        m_set.clear();
-        m_data = PropertyNameArrayData::create();
     }
 

trunk/Source/JavaScriptCore/runtime/ProxyObject.cpp

@@ -1007 +1007 @@
     }
 
-    if (!targetIsExensible) {
-        for (UniquedStringImpl* impl : targetConfigurableKeys) {
-            if (removeIfContainedInUncheckedResultKeys(impl) == IsNotContainedIn) {
-                throwVMTypeError(exec, scope, makeString("Proxy object's non-extensible 'target' has configurable property '", String(impl), "' that was not in the result from the 'ownKeys' trap"));
-                return;
-            }
-        }
-
-        if (uncheckedResultKeys.size()) {
-            throwVMTypeError(exec, scope, "Proxy handler's 'ownKeys' method returned a key that was not present in its non-extensible target"_s);
+    if (targetIsExensible)
+        return;
+
+    for (UniquedStringImpl* impl : targetConfigurableKeys) {
+        if (removeIfContainedInUncheckedResultKeys(impl) == IsNotContainedIn) {
+            throwVMTypeError(exec, scope, makeString("Proxy object's non-extensible 'target' has configurable property '", String(impl), "' that was not in the result from the 'ownKeys' trap"));
             return;
         }
     }
 
-    if (!enumerationMode.includeDontEnumProperties()) {
-        // Filtering DontEnum properties is observable in proxies and must occur following the invariant checks above.
-        auto data = trapResult.releaseData();
-        trapResult.reset();
-
-        for (auto propertyName : data->propertyNameVector()) {
-            PropertySlot slot(this, PropertySlot::InternalMethodType::GetOwnProperty);
-            if (!getOwnPropertySlotCommon(exec, propertyName, slot))
-                continue;
-            if (slot.attributes() & PropertyAttribute::DontEnum)
-                continue;
-            trapResult.addUnchecked(propertyName.impl());
-        }
+    if (uncheckedResultKeys.size()) {
+        throwVMTypeError(exec, scope, "Proxy handler's 'ownKeys' method returned a key that was not present in its non-extensible target"_s);
+        return;
     }
 }

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2019-04-08  Ryan Haddad  <ryanhaddad@apple.com>
+
+        Unreviewed, rolling out r243943.
+
+        Caused test262 failures.
+
+        Reverted changeset:
+
+        "[JSC] Filter DontEnum properties in
+        ProxyObject::getOwnPropertyNames()"
+        https://bugs.webkit.org/show_bug.cgi?id=176810
+        https://trac.webkit.org/changeset/243943
+
 2019-04-05  Sergio Villar Senin  <svillar@igalia.com>
 

trunk/Source/WebCore/bindings/js/JSDOMConvertRecord.h

@@ -87 +87 @@
         // 4. Let keys be ? O.[[OwnPropertyKeys]]().
         JSC::PropertyNameArray keys(&vm, JSC::PropertyNameMode::Strings, JSC::PrivateSymbolMode::Exclude);
-        object->methodTable(vm)->getOwnPropertyNames(object, &state, keys, JSC::EnumerationMode(JSC::DontEnumPropertiesMode::Include));
+        object->methodTable(vm)->getOwnPropertyNames(object, &state, keys, JSC::EnumerationMode());
 
         RETURN_IF_EXCEPTION(scope, { });
@@ -100 +100 @@
             // 2. If desc is not undefined and desc.[[Enumerable]] is true:
 
-            // It's necessary to filter enumerable here rather than using the default EnumerationMode,
-            // to prevent an observable extra [[GetOwnProperty]] operation in the case of ProxyObject records.
+            // FIXME: Do we need to check for enumerable / undefined, or is this handled by the default
+            // enumeration mode?
+
             if (didGetDescriptor && descriptor.enumerable()) {
                 // 1. Let typedKey be key converted to an IDL value of type K.