Attachment 362762 Details for Bug 176810 – [JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames()

[patch] [JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames()

bug-176810-20190222170033.patch (text/plain), 11.00 KB, created by Caitlin Potter (:caitp) on 2019-02-22 14:00:36 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Caitlin Potter (:caitp)

Created: 2019-02-22 14:00:36 PST

Size: 11.00 KB

patch

obsolete

>Subversion Revision: 241963
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 41003adb37cba63c62cfa4752dabd4410486e193..95f764e9e04e41a4379e5818dc11a8408345b94b 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,22 @@
>+2019-02-22  Caitlin Potter  <caitp@igalia.com>
>+
>+        [JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames()
>+        https://bugs.webkit.org/show_bug.cgi?id=176810
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        This adds conditional logic following the invariant checks, to perform
>+        filtering in common uses of getOwnPropertyNames.
>+
>+        While this would ideally only be done in JSPropertyNameEnumerator, adding
>+        the filtering to ProxyObject::performGetOwnPropertyNames maintains the
>+        invariant that the EnumerationMode is properly followed.
>+
>+        * runtime/PropertyNameArray.h:
>+        (JSC::PropertyNameArray::reset):
>+        * runtime/ProxyObject.cpp:
>+        (JSC::ProxyObject::performGetOwnPropertyNames):
>+
> 2019-02-22  Yusuke Suzuki  <ysuzuki@apple.com>
> 
>         [JSC] putNonEnumerable in JSWrapperMap is too costly
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 8e793d00f733f8073f2e44115fdb41270b0b65c5..1f75d4ae7657dd7566f0be97ef52349fd2cc0270 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,21 @@
>+2019-02-22  Caitlin Potter  <caitp@igalia.com>
>+
>+        [JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames()
>+        https://bugs.webkit.org/show_bug.cgi?id=176810
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Previously, there was a comment here indicating uncertainty of whether it
>+        was necessary to filter DontEnum properties explicitly or not. It turns
>+        out that it was necessary in the case of JSC ProxyObjects.
>+
>+        This patch adds DontEnum filtering for ProxyObjects, however we continue
>+        to explicitly filter them in JSDOMConvertRecord, which needs to use the
>+        property descriptor after filtering. This change prevents observably
>+        fetching the property descriptor twice per property.
>+
>+        * bindings/js/JSDOMConvertRecord.h:
>+
> 2019-02-22  Wenson Hsieh  <wenson_hsieh@apple.com>
> 
>         Input type "formatSetInlineTextDirection" is dispatched when changing paragraph-level text direction
>diff --git a/Source/JavaScriptCore/runtime/PropertyNameArray.h b/Source/JavaScriptCore/runtime/PropertyNameArray.h
>index 5e93b79abf560234eae8bf4a9e720150e7e14aed..8ec9ac1a70aebf11beaf747e91c77518c46e06e2 100644
>--- a/Source/JavaScriptCore/runtime/PropertyNameArray.h
>+++ b/Source/JavaScriptCore/runtime/PropertyNameArray.h
>@@ -55,6 +55,12 @@ public:
>     {
>     }
> 
>+    void reset()
>+    {
>+        m_set.clear();
>+        m_data = PropertyNameArrayData::create();
>+    }
>+
>     VM* vm() { return m_vm; }
> 
>     void add(uint32_t index)
>diff --git a/Source/JavaScriptCore/runtime/ProxyObject.cpp b/Source/JavaScriptCore/runtime/ProxyObject.cpp
>index bbebf3a86ef56a4c34b33655f31828d0b1148f70..38dc87375888224a27e5f66b14544dc356bd5dde 100644
>--- a/Source/JavaScriptCore/runtime/ProxyObject.cpp
>+++ b/Source/JavaScriptCore/runtime/ProxyObject.cpp
>@@ -993,19 +993,33 @@ void ProxyObject::performGetOwnPropertyNames(ExecState* exec, PropertyNameArray&
>         }
>     }
> 
>-    if (targetIsExensible)
>-        return;
>+    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;
>+            }
>+        }
> 
>-    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"));
>+        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;
>         }
>     }
> 
>-    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;
>+    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());
>+        }
>     }
> }
> 
>diff --git a/Source/WebCore/bindings/js/JSDOMConvertRecord.h b/Source/WebCore/bindings/js/JSDOMConvertRecord.h
>index 75b2eb6aecd2c08db2ac3c4ca63077d119f8f587..df9c3a6ae90cf015bc9697ca2608ce0f8fd1b586 100644
>--- a/Source/WebCore/bindings/js/JSDOMConvertRecord.h
>+++ b/Source/WebCore/bindings/js/JSDOMConvertRecord.h
>@@ -86,7 +86,7 @@ template<typename K, typename V> struct Converter<IDLRecord<K, V>> : DefaultConv
>     
>         // 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());
>+        object->methodTable(vm)->getOwnPropertyNames(object, &state, keys, JSC::EnumerationMode(JSC::DontEnumPropertiesMode::Include));
> 
>         RETURN_IF_EXCEPTION(scope, { });
> 
>@@ -99,9 +99,8 @@ template<typename K, typename V> struct Converter<IDLRecord<K, V>> : DefaultConv
> 
>             // 2. If desc is not undefined and desc.[[Enumerable]] is true:
> 
>-            // FIXME: Do we need to check for enumerable / undefined, or is this handled by the default
>-            // enumeration mode?
>-
>+            // 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.
>             if (didGetDescriptor && descriptor.enumerable()) {
>                 // 1. Let typedKey be key converted to an IDL value of type K.
>                 auto typedKey = Detail::IdentifierConverter<K>::convert(state, key);
>diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
>index 261d8178b3a76236d1fa30ce6ecabe0386111f4a..978286b713b3cb77679ca626b372e0258e9ff361 100644
>--- a/JSTests/ChangeLog
>+++ b/JSTests/ChangeLog
>@@ -1,3 +1,20 @@
>+2019-02-22  Caitlin Potter  <caitp@igalia.com>
>+
>+        [JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames()
>+        https://bugs.webkit.org/show_bug.cgi?id=176810
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Add tests for the DontEnum filtering, and variations of other tests
>+        take the DontEnum-filtering path.
>+
>+        * stress/proxy-own-keys.js:
>+        (i.catch):
>+        (set assert):
>+        (set add):
>+        (let.set new):
>+        (get let):
>+
> 2019-02-19  Joseph Pecoraro  <pecoraro@apple.com>
> 
>         Web Inspector: Improve ES6 Class instances in Heap Snapshot instances view
>diff --git a/JSTests/stress/proxy-own-keys.js b/JSTests/stress/proxy-own-keys.js
>index 189b20b36ef047d466edee765e215c98019eed4a..90c61fc4d8429b18fed1cb6858ebe44dfc598320 100644
>--- a/JSTests/stress/proxy-own-keys.js
>+++ b/JSTests/stress/proxy-own-keys.js
>@@ -135,6 +135,22 @@ function assert(b) {
>         assert(called);
>         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;
>+    }
> }
> 
> {
>@@ -166,6 +182,22 @@ function assert(b) {
>         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;
>+    }
> }
> 
> {
>@@ -662,3 +694,68 @@ function shallowEq(a, b) {
>         error = null;
>     }
> }
>+
>+{
>+    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);
>+    }
>+}

Actions: View | Formatted Diff | Diff

Attachments on bug 176810: 351389 | 351402 | 351405 | 351412 | 351468 | 351472 | 359910 | 359911 | 359912 | 359922 | 359930 | 359935 | 359937 | 359941 | 359952 | 359953 | 359955 | 359959 | 359961 | 359975 | 359979 | 359980 | 359988 | 360004 | 360032 | 362731 | 362762 | 362812 | 362896 | 366956 | 367326