Skip to content
Permalink
Browse files
[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.

This was originally rolled out in r244020, as DontEnum filtering code
in ObjectConstructor.cpp's ownPropertyKeys() had not been removed. It's
now redundant due to being handled in ProxyObject::getOwnPropertyNames().

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


Canonical link: https://commits.webkit.org/211229@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@244330 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
caitp committed Apr 16, 2019
1 parent 560b5be commit fc978ba6beba461977d8a4b2a64774689725bed0
@@ -1,3 +1,20 @@
2019-04-16 Caitlin Potter <caitp@igalia.com>

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

Reviewed by Saam Barati.

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-04-15 Saam barati <sbarati@apple.com>

Modify how we do SetArgument when we inline varargs calls
@@ -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;
}
}

{
@@ -667,3 +699,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);
}
}
@@ -1,3 +1,26 @@
2019-04-16 Caitlin Potter <caitp@igalia.com>

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

Reviewed by Saam Barati.

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.

This was originally rolled out in r244020, as DontEnum filtering code
in ObjectConstructor.cpp's ownPropertyKeys() had not been removed. It's
now redundant due to being handled in ProxyObject::getOwnPropertyNames().

* runtime/PropertyNameArray.h:
(JSC::PropertyNameArray::reset):
* runtime/ProxyObject.cpp:
(JSC::ProxyObject::performGetOwnPropertyNames):

2019-04-15 Saam barati <sbarati@apple.com>

Modify how we do SetArgument when we inline varargs calls
@@ -920,23 +920,9 @@ JSArray* ownPropertyKeys(ExecState* exec, JSObject* object, PropertyNameMode pro
object->methodTable(vm)->getOwnPropertyNames(object, exec, properties, EnumerationMode(dontEnumPropertiesMode));
RETURN_IF_EXCEPTION(scope, nullptr);

// https://tc39.github.io/ecma262/#sec-enumerableownproperties
// If {object} is a Proxy, an explicit and observable [[GetOwnProperty]] op is required to filter out non-enumerable properties.
// In other cases, filtering has already been performed.
const bool mustFilterProperty = dontEnumPropertiesMode == DontEnumPropertiesMode::Exclude && object->type() == ProxyObjectType;
auto filterPropertyIfNeeded = [exec, object, mustFilterProperty](const Identifier& identifier) {
if (!mustFilterProperty)
return true;
PropertyDescriptor descriptor;
PropertyName name(identifier);
return object->getOwnPropertyDescriptor(exec, name, descriptor) && descriptor.enumerable();
};

// If !mustFilterProperty and PropertyNameMode::Strings mode, we do not need to filter out any entries in PropertyNameArray.
// We can use fast allocation and initialization.
if (propertyNameMode != PropertyNameMode::StringsAndSymbols) {
ASSERT(propertyNameMode == PropertyNameMode::Strings || propertyNameMode == PropertyNameMode::Symbols);
if (!mustFilterProperty && properties.size() < MIN_SPARSE_ARRAY_INDEX) {
if (properties.size() < MIN_SPARSE_ARRAY_INDEX) {
if (LIKELY(!globalObject->isHavingABadTime())) {
if (isObjectKeys) {
Structure* structure = object->structure(vm);
@@ -993,10 +979,7 @@ JSArray* ownPropertyKeys(ExecState* exec, JSObject* object, PropertyNameMode pro
for (size_t i = 0; i < numProperties; i++) {
const auto& identifier = properties[i];
ASSERT(!identifier.isSymbol());
bool hasProperty = filterPropertyIfNeeded(identifier);
EXCEPTION_ASSERT(!scope.exception() || !hasProperty);
if (hasProperty)
pushDirect(exec, keys, jsOwnedString(exec, identifier.string()));
pushDirect(exec, keys, jsOwnedString(exec, identifier.string()));
RETURN_IF_EXCEPTION(scope, nullptr);
}
break;
@@ -1008,10 +991,7 @@ JSArray* ownPropertyKeys(ExecState* exec, JSObject* object, PropertyNameMode pro
const auto& identifier = properties[i];
ASSERT(identifier.isSymbol());
ASSERT(!identifier.isPrivateName());
bool hasProperty = filterPropertyIfNeeded(identifier);
EXCEPTION_ASSERT(!scope.exception() || !hasProperty);
if (hasProperty)
pushDirect(exec, keys, Symbol::create(vm, static_cast<SymbolImpl&>(*identifier.impl())));
pushDirect(exec, keys, Symbol::create(vm, static_cast<SymbolImpl&>(*identifier.impl())));
RETURN_IF_EXCEPTION(scope, nullptr);
}
break;
@@ -1028,19 +1008,13 @@ JSArray* ownPropertyKeys(ExecState* exec, JSObject* object, PropertyNameMode pro
continue;
}

bool hasProperty = filterPropertyIfNeeded(identifier);
EXCEPTION_ASSERT(!scope.exception() || !hasProperty);
if (hasProperty)
pushDirect(exec, keys, jsOwnedString(exec, identifier.string()));
pushDirect(exec, keys, jsOwnedString(exec, identifier.string()));
RETURN_IF_EXCEPTION(scope, nullptr);
}

// To ensure the order defined in the spec (9.1.12), we append symbols at the last elements of keys.
for (const auto& identifier : propertySymbols) {
bool hasProperty = filterPropertyIfNeeded(identifier);
EXCEPTION_ASSERT(!scope.exception() || !hasProperty);
if (hasProperty)
pushDirect(exec, keys, Symbol::create(vm, static_cast<SymbolImpl&>(*identifier.impl())));
pushDirect(exec, keys, Symbol::create(vm, static_cast<SymbolImpl&>(*identifier.impl())));
RETURN_IF_EXCEPTION(scope, nullptr);
}

@@ -55,6 +55,12 @@ class PropertyNameArray {
{
}

void reset()
{
m_set.clear();
m_data = PropertyNameArrayData::create();
}

VM* vm() { return m_vm; }

void add(uint32_t index)
@@ -1006,19 +1006,35 @@ 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);
auto result = getOwnPropertySlotCommon(exec, propertyName, slot);
RETURN_IF_EXCEPTION(scope, void());
if (!result)
continue;
if (slot.attributes() & PropertyAttribute::DontEnum)
continue;
trapResult.addUnchecked(propertyName.impl());
}
}
}

@@ -1,3 +1,21 @@
2019-04-16 Caitlin Potter <caitp@igalia.com>

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

Reviewed by Saam Barati.

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-04-15 Antoine Quint <graouts@apple.com>

[iOS] Redundant pointer events causes material design buttons to flush twice
@@ -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);

0 comments on commit fc978ba

Please sign in to comment.