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

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

bug-176810-20190123142742.patch (text/plain), 6.73 KB, created by Caitlin Potter (:caitp) on 2019-01-23 11:27:43 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Caitlin Potter (:caitp)

Created: 2019-01-23 11:27:43 PST

Size: 6.73 KB

patch

obsolete

>Subversion Revision: 240301
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index bc5460edcf6e7cb2b4ddd04185d83dc16ff2ed98..7274324623aa2f545c7d095c5e57a6ac7700dbf5 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,22 @@
>+2019-01-23  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-01-23  Caitlin Potter  <caitp@igalia.com>
> 
>         [JSC] throw if ownKeys Proxy trap result contains duplicate keys
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index d9ef453f48665c98734ef9afb53cddd3db4fbab4..cd786935549fb315ff83806223fb45a0f8cce7c1 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,23 @@
>+2019-01-23  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.
>+
>+        No new tests (OOPS!).
>+
>+        * bindings/js/JSDOMConvertRecord.h:
>+
> 2019-01-22  Alex Christensen  <achristensen@webkit.org>
> 
>         Fix more builds.
>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 7e9f610307b157950b47a49bdfb2b6cb6034aa91..d1977836a9220113d4c2e039c733e38dab0e7269 100644
>--- a/Source/JavaScriptCore/runtime/ProxyObject.cpp
>+++ b/Source/JavaScriptCore/runtime/ProxyObject.cpp
>@@ -999,19 +999,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);

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