Changeset 243943 in webkit


Ignore:
Timestamp:
Apr 5, 2019 2:28:10 PM (5 years ago)
Author:
caitp@igalia.com
Message:

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

Reviewed by Saam Barati.

JSTests:

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):

Source/JavaScriptCore:

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):

Source/WebCore:

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:
Location:
trunk
Files:
7 edited

Legend:

Unmodified
Added
Removed
  • trunk/JSTests/ChangeLog

    r243933 r243943  
     12019-04-05  Caitlin Potter  <caitp@igalia.com>
     2
     3        [JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames()
     4        https://bugs.webkit.org/show_bug.cgi?id=176810
     5
     6        Reviewed by Saam Barati.
     7
     8        Add tests for the DontEnum filtering, and variations of other tests
     9        take the DontEnum-filtering path.
     10
     11        * stress/proxy-own-keys.js:
     12        (i.catch):
     13        (set assert):
     14        (set add):
     15        (let.set new):
     16        (get let):
     17
    1182019-04-05  Caitlin Potter  <caitp@igalia.com>
    219
  • trunk/JSTests/stress/proxy-own-keys.js

    r243933 r243943  
    136136        called = false;
    137137    }
     138
     139    for (let i = 0; i < 500; i++) {
     140        let threw = false;
     141        let foundKey = false;
     142        try {
     143            for (let k in proxy)
     144                foundKey = true;
     145        } catch(e) {
     146            threw = true;
     147            assert(e.toString() === "TypeError: Proxy object's non-extensible 'target' has configurable property 'x' that was not in the result from the 'ownKeys' trap");
     148            assert(!foundKey);
     149        }
     150        assert(threw);
     151        assert(called);
     152        called = false;
     153    }
    138154}
    139155
     
    165181        assert(threw);
    166182        assert(called);
     183        called = false;
     184    }
     185
     186    for (let i = 0; i < 500; i++) {
     187        let threw = false;
     188        let reached = false;
     189        try {
     190            for (let k in proxy)
     191                reached = true;
     192        } catch (e) {
     193            threw = true;
     194            assert(e.toString() === "TypeError: Proxy handler's 'ownKeys' method returned a key that was not present in its non-extensible target");
     195        }
     196        assert(threw);
     197        assert(called);
     198        assert(!reached);
    167199        called = false;
    168200    }
     
    668700    }
    669701}
     702
     703{
     704    let error = null;
     705    let s1 = Symbol();
     706    let s2 = Symbol();
     707    let target = Object.defineProperties({}, {
     708        x: {
     709            value: "X",
     710            enumerable: true,
     711            configurable: true,
     712        },
     713        dontEnum1: {
     714            value: "dont-enum",
     715            enumerable: false,
     716            configurable: true,
     717        },
     718        y: {
     719            get() { return "Y"; },
     720            enumerable: true,
     721            configurable: true,
     722        },
     723        dontEnum2: {
     724            get() { return "dont-enum-accessor" },
     725            enumerable: false,
     726            configurable: true
     727        },
     728        [s1]: {
     729            value: "s1",
     730            enumerable: true,
     731            configurable: true,
     732        },
     733        [s2]: {
     734            value: "dont-enum-symbol",
     735            enumerable: false,
     736            configurable: true, 
     737        },
     738    });
     739    let checkedOwnKeys = false;
     740    let checkedPropertyDescriptor = false;
     741    let handler = {
     742        ownKeys() {
     743            checkedOwnKeys = true;
     744            return ["x", "dontEnum1", "y", "dontEnum2", s1, s2];
     745        },
     746        getOwnPropertyDescriptor(t, k) {
     747            checkedPropertyDescriptors = true;
     748            return Reflect.getOwnPropertyDescriptor(t, k);
     749        }
     750    };
     751    let proxy = new Proxy(target, handler);
     752    for (let i = 0; i < 500; i++) {
     753        checkedPropertyDescriptors = false;
     754        assert(shallowEq(["x", "dontEnum1", "y", "dontEnum2", s1, s2], Reflect.ownKeys(proxy)));
     755        assert(checkedOwnKeys);
     756        assert(!checkedPropertyDescriptors);
     757        checkedOwnKeys = false;
     758
     759        let enumerableStringKeys = [];
     760        for (let k in proxy)
     761            enumerableStringKeys.push(k);
     762        assert(shallowEq(["x", "y"], enumerableStringKeys));
     763        assert(checkedOwnKeys);
     764        assert(checkedPropertyDescriptors);
     765    }
     766}
  • trunk/Source/JavaScriptCore/ChangeLog

    r243934 r243943  
     12019-04-05  Caitlin Potter  <caitp@igalia.com>
     2
     3        [JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames()
     4        https://bugs.webkit.org/show_bug.cgi?id=176810
     5
     6        Reviewed by Saam Barati.
     7
     8        This adds conditional logic following the invariant checks, to perform
     9        filtering in common uses of getOwnPropertyNames.
     10
     11        While this would ideally only be done in JSPropertyNameEnumerator, adding
     12        the filtering to ProxyObject::performGetOwnPropertyNames maintains the
     13        invariant that the EnumerationMode is properly followed.
     14
     15        * runtime/PropertyNameArray.h:
     16        (JSC::PropertyNameArray::reset):
     17        * runtime/ProxyObject.cpp:
     18        (JSC::ProxyObject::performGetOwnPropertyNames):
     19
    1202019-04-05  Commit Queue  <commit-queue@webkit.org>
    221
  • trunk/Source/JavaScriptCore/runtime/PropertyNameArray.h

    r222017 r243943  
    5454        , m_privateSymbolMode(privateSymbolMode)
    5555    {
     56    }
     57
     58    void reset()
     59    {
     60        m_set.clear();
     61        m_data = PropertyNameArrayData::create();
    5662    }
    5763
  • trunk/Source/JavaScriptCore/runtime/ProxyObject.cpp

    r243933 r243943  
    10071007    }
    10081008
    1009     if (targetIsExensible)
    1010         return;
    1011 
    1012     for (UniquedStringImpl* impl : targetConfigurableKeys) {
    1013         if (removeIfContainedInUncheckedResultKeys(impl) == IsNotContainedIn) {
    1014             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"));
     1009    if (!targetIsExensible) {
     1010        for (UniquedStringImpl* impl : targetConfigurableKeys) {
     1011            if (removeIfContainedInUncheckedResultKeys(impl) == IsNotContainedIn) {
     1012                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"));
     1013                return;
     1014            }
     1015        }
     1016
     1017        if (uncheckedResultKeys.size()) {
     1018            throwVMTypeError(exec, scope, "Proxy handler's 'ownKeys' method returned a key that was not present in its non-extensible target"_s);
    10151019            return;
    10161020        }
    10171021    }
    10181022
    1019     if (uncheckedResultKeys.size()) {
    1020         throwVMTypeError(exec, scope, "Proxy handler's 'ownKeys' method returned a key that was not present in its non-extensible target"_s);
    1021         return;
     1023    if (!enumerationMode.includeDontEnumProperties()) {
     1024        // Filtering DontEnum properties is observable in proxies and must occur following the invariant checks above.
     1025        auto data = trapResult.releaseData();
     1026        trapResult.reset();
     1027
     1028        for (auto propertyName : data->propertyNameVector()) {
     1029            PropertySlot slot(this, PropertySlot::InternalMethodType::GetOwnProperty);
     1030            if (!getOwnPropertySlotCommon(exec, propertyName, slot))
     1031                continue;
     1032            if (slot.attributes() & PropertyAttribute::DontEnum)
     1033                continue;
     1034            trapResult.addUnchecked(propertyName.impl());
     1035        }
    10221036    }
    10231037}
  • trunk/Source/WebCore/ChangeLog

    r243941 r243943  
     12019-04-05  Caitlin Potter  <caitp@igalia.com>
     2
     3        [JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames()
     4        https://bugs.webkit.org/show_bug.cgi?id=176810
     5
     6        Reviewed by Saam Barati.
     7
     8        Previously, there was a comment here indicating uncertainty of whether it
     9        was necessary to filter DontEnum properties explicitly or not. It turns
     10        out that it was necessary in the case of JSC ProxyObjects.
     11
     12        This patch adds DontEnum filtering for ProxyObjects, however we continue
     13        to explicitly filter them in JSDOMConvertRecord, which needs to use the
     14        property descriptor after filtering. This change prevents observably
     15        fetching the property descriptor twice per property.
     16
     17        * bindings/js/JSDOMConvertRecord.h:
     18
    1192019-04-05  Michael Catanzaro  <mcatanzaro@igalia.com>
    220
  • trunk/Source/WebCore/bindings/js/JSDOMConvertRecord.h

    r228218 r243943  
    8787        // 4. Let keys be ? O.[[OwnPropertyKeys]]().
    8888        JSC::PropertyNameArray keys(&vm, JSC::PropertyNameMode::Strings, JSC::PrivateSymbolMode::Exclude);
    89         object->methodTable(vm)->getOwnPropertyNames(object, &state, keys, JSC::EnumerationMode());
     89        object->methodTable(vm)->getOwnPropertyNames(object, &state, keys, JSC::EnumerationMode(JSC::DontEnumPropertiesMode::Include));
    9090
    9191        RETURN_IF_EXCEPTION(scope, { });
     
    100100            // 2. If desc is not undefined and desc.[[Enumerable]] is true:
    101101
    102             // FIXME: Do we need to check for enumerable / undefined, or is this handled by the default
    103             // enumeration mode?
    104 
     102            // It's necessary to filter enumerable here rather than using the default EnumerationMode,
     103            // to prevent an observable extra [[GetOwnProperty]] operation in the case of ProxyObject records.
    105104            if (didGetDescriptor && descriptor.enumerable()) {
    106105                // 1. Let typedKey be key converted to an IDL value of type K.
Note: See TracChangeset for help on using the changeset viewer.