- Revision
- 243943
- Author
- [email protected]
- Date
- 2019-04-05 14:28:10 -0700 (Fri, 05 Apr 2019)
Log 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/_javascript_Core:
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:
Modified Paths
Diff
Modified: trunk/JSTests/ChangeLog (243942 => 243943)
--- trunk/JSTests/ChangeLog 2019-04-05 21:13:47 UTC (rev 243942)
+++ trunk/JSTests/ChangeLog 2019-04-05 21:28:10 UTC (rev 243943)
@@ -1,5 +1,22 @@
2019-04-05 Caitlin Potter <[email protected]>
+ [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-05 Caitlin Potter <[email protected]>
+
[JSC] throw if 'ownKeys' Proxy trap result contains duplicate keys
https://bugs.webkit.org/show_bug.cgi?id=185211
Modified: trunk/JSTests/stress/proxy-own-keys.js (243942 => 243943)
--- trunk/JSTests/stress/proxy-own-keys.js 2019-04-05 21:13:47 UTC (rev 243942)
+++ trunk/JSTests/stress/proxy-own-keys.js 2019-04-05 21:28:10 UTC (rev 243943)
@@ -135,6 +135,22 @@
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 @@
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 @@
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);
+ }
+}
Modified: trunk/Source/_javascript_Core/ChangeLog (243942 => 243943)
--- trunk/Source/_javascript_Core/ChangeLog 2019-04-05 21:13:47 UTC (rev 243942)
+++ trunk/Source/_javascript_Core/ChangeLog 2019-04-05 21:28:10 UTC (rev 243943)
@@ -1,3 +1,22 @@
+2019-04-05 Caitlin Potter <[email protected]>
+
+ [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.
+
+ * runtime/PropertyNameArray.h:
+ (JSC::PropertyNameArray::reset):
+ * runtime/ProxyObject.cpp:
+ (JSC::ProxyObject::performGetOwnPropertyNames):
+
2019-04-05 Commit Queue <[email protected]>
Unreviewed, rolling out r243833.
Modified: trunk/Source/_javascript_Core/runtime/PropertyNameArray.h (243942 => 243943)
--- trunk/Source/_javascript_Core/runtime/PropertyNameArray.h 2019-04-05 21:13:47 UTC (rev 243942)
+++ trunk/Source/_javascript_Core/runtime/PropertyNameArray.h 2019-04-05 21:28:10 UTC (rev 243943)
@@ -55,6 +55,12 @@
{
}
+ void reset()
+ {
+ m_set.clear();
+ m_data = PropertyNameArrayData::create();
+ }
+
VM* vm() { return m_vm; }
void add(uint32_t index)
Modified: trunk/Source/_javascript_Core/runtime/ProxyObject.cpp (243942 => 243943)
--- trunk/Source/_javascript_Core/runtime/ProxyObject.cpp 2019-04-05 21:13:47 UTC (rev 243942)
+++ trunk/Source/_javascript_Core/runtime/ProxyObject.cpp 2019-04-05 21:28:10 UTC (rev 243943)
@@ -1006,19 +1006,33 @@
}
}
- 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.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());
+ }
}
}
Modified: trunk/Source/WebCore/ChangeLog (243942 => 243943)
--- trunk/Source/WebCore/ChangeLog 2019-04-05 21:13:47 UTC (rev 243942)
+++ trunk/Source/WebCore/ChangeLog 2019-04-05 21:28:10 UTC (rev 243943)
@@ -1,3 +1,21 @@
+2019-04-05 Caitlin Potter <[email protected]>
+
+ [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-05 Michael Catanzaro <[email protected]>
Unreviewed manual rollout of r243929
Modified: trunk/Source/WebCore/bindings/js/JSDOMConvertRecord.h (243942 => 243943)
--- trunk/Source/WebCore/bindings/js/JSDOMConvertRecord.h 2019-04-05 21:13:47 UTC (rev 243942)
+++ trunk/Source/WebCore/bindings/js/JSDOMConvertRecord.h 2019-04-05 21:28:10 UTC (rev 243943)
@@ -86,7 +86,7 @@
// 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 @@
// 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);
