assert.deepEqual and assert.deepStrictEqual incorrectly compare Proxied arrays when using ownKeys trap

[itaylor]

Version

v17.4.0

Platform

Darwin macbook-pro-5.lan 21.2.0 Darwin Kernel Version 21.2.0: Sun Nov 28 20:28:54 PST 2021; root:xnu-8019.61.5~1/RELEASE_X86_64 x86_64

Subsystem

assert

What steps will reproduce the bug?

assert.deepEqual(new Proxy(['foo'], {}), ['foo']); // works properly
assert.deepEqual(new Proxy(['foo'], { ownKeys: (target) => Reflect.ownKeys(target) }), ['foo']) // throws

assert.deepStrictEqual(new Proxy(['foo'], {}), ['foo']); // works properly
assert.deepStrictEqual(new Proxy(['foo'], { ownKeys: (target) => Reflect.ownKeys(target) }), ['foo']) // throws

In the above, Reflect.ownKeys(target) should be the same as not setting an ownKeys trap, but it isn't. The trap works fine elsewhere

Indeed,

Reflect.ownKeys(['foo'])  // [ '0', 'length' ]
assert.deepEqual(new Proxy(['foo'], { ownKeys: (target) => ['0', 'length'] }), ['foo'])  // throws

How often does it reproduce? Is there a required condition?

Every time

What is the expected behavior?

Using a ownKeys trap shouldn't cause comparison errors.

The ownKeys trap seems to be working fine elsewhere. I traced the code to:

node/lib/internal/util/comparisons.js

Line 179 in dab8ab2

const keys1 = getOwnNonIndexProperties(val1, filter);

Where the GetOwnNonIndexProperties function is called. Perhaps it is incorrectly filtering or the filtering is broken for proxies somehow?

What do you see instead?

Comparison errors from assert.deepEquals and assert.deepStrictEquals when the Arrays compared have the same content, but are using Proxies with ownKeys traps.

Additional information

I tried this on node 14, 16, and 17, with the same results.

[Trott]

@nodejs/assert

[benjamingr]

Hmm

> require('internal/test/binding').internalBinding('util').getOwnNonIndexProperties(new Proxy(['foo'], { ownKeys: (target) => Reflect.ownKeys(target) }), 0)
[ 0, 'length' ]

So the issue is that 0 is considered an index property.

So the issue is that

object->GetPropertyNames(
        context, KeyCollectionMode::kOwnOnly,
        filter,
        IndexFilter::kSkipIndices)
          .ToLocal(&properties)

Does not ignore index properties with a proxy trap?

[benjamingr]

@ljharb it wouldn't matter in this case:

> require('internal/test/binding').internalBinding('util').getOwnNonIndexProperties(new Proxy(['foo'], { ownKeys: Reflect.ownKeys }), 0)
[ 0, 'length' ]
> (node:70102) internal/test/binding: These APIs are for internal testing only. Do not use them.
(Use `node --trace-warnings ...` to show where the warning was created)

This is a "bug" either in v8 or how we use v8 which returns index properties from the call above. (I saw bug because I don't believe we ever discussed what proxy behavior should be)

[itaylor]

@ljharb, The problem still occurs with passing {ownKeys: Reflect.ownKeys}

assert.deepEqual(new Proxy(['foo'], { ownKeys: Reflect.ownKeys }), ['foo']) // throws

I see in the spec that you're right, it should be passing a target argument, but that argument isn't listed in the MDN documentation of Reflect.ownKeys.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Reflect/ownKeys

[benjamingr]

A fix can be something like this:

diff --git a/lib/internal/util/comparisons.js b/lib/internal/util/comparisons.js
index c126bd6346..6a529c100f 100644
--- a/lib/internal/util/comparisons.js
+++ b/lib/internal/util/comparisons.js
@@ -36,6 +36,7 @@ const {
   isRegExp,
   isSet,
   isNativeError,
+  isProxy,
   isBoxedPrimitive,
   isNumberObject,
   isStringObject,
@@ -68,6 +69,14 @@ function areSimilarRegExps(a, b) {
          a.lastIndex === b.lastIndex;
 }
 
+function getOwnNonIndexPropertiesHandleProxies(target, filter) {
+  const res = getOwnNonIndexProperties(target, filter);
+  if (isProxy(target)) {
+    return res.filter((x) => typeof x !== 'number');
+  }
+  return res;
+}
+
 function areSimilarFloatArrays(a, b) {
   if (a.byteLength !== b.byteLength) {
     return false;
@@ -176,8 +185,8 @@ function innerDeepEqual(val1, val2, strict, memos) {
       return false;
     }
     const filter = strict ? ONLY_ENUMERABLE : ONLY_ENUMERABLE | SKIP_SYMBOLS;
-    const keys1 = getOwnNonIndexProperties(val1, filter);
-    const keys2 = getOwnNonIndexProperties(val2, filter);
+    const keys1 = getOwnNonIndexPropertiesHandleProxies(val1, filter);
+    const keys2 = getOwnNonIndexPropertiesHandleProxies(val2, filter);
     if (keys1.length !== keys2.length) {
       return false;
     }
@@ -217,8 +226,8 @@ function innerDeepEqual(val1, val2, strict, memos) {
     // only contain numeric keys, we don't need to exam further than checking
     // the symbols.
     const filter = strict ? ONLY_ENUMERABLE : ONLY_ENUMERABLE | SKIP_SYMBOLS;
-    const keys1 = getOwnNonIndexProperties(val1, filter);
-    const keys2 = getOwnNonIndexProperties(val2, filter);
+    const keys1 = getOwnNonIndexPropertiesHandleProxies(val1, filter);
+    const keys2 = getOwnNonIndexPropertiesHandleProxies(val2, filter);
     if (keys1.length !== keys2.length) {
       return false;
     }

We should also probably file a v8 bug?

[ljharb]

Wouldn’t that fix potentially interfere with proxies of arrays, or proxies of objects with numeric string keys?

[benjamingr]

@ljharb

Wouldn’t that fix potentially interfere with proxies of arrays, or proxies of objects with numeric string keys?

It would as in today they don't work and following this patch they do and return the same keys as the non-proxy version

[devsnek]

0 is an index regardless of what kind of object it is pulled off of. If this is coming from IndexFilter::kSkipIndices I would suggest fixing it there.

[benjamingr]

@ljharb the issue is much simpler than that I think? no one updated the assert code to support proxies and there are (likely) a bunch of bugs in the V8 APIs used like Gus said.

So I think this feature request is definitely worth doing but am waiting for someone with more regular commits to V8 to either fix it there or tell us it's intended so we work around it similar to #41714 (comment)

Note node is not distinguishing something based on whether or not it's a proxy in that workaround - it just works around the underlying v8 bug that does.

[ljharb]

@benjamingr sure, there's surely something to fix here.

What I meant is, node's inspection output in the thrown assertion error clearly marks the proxy as a Proxy; that's the part i was saying is a violation of the language's intent.

[benjamingr]

@ljharb

What I meant is, node's inspection output in the thrown assertion error clearly marks the proxy as a Proxy; that's the part i was saying is a violation of the language's intent.

Yes though that's not intentional it's just since assert was never made to work with proxies. This is a v8 bug not something intentional Node chose to do.

[BridgeAR]

I had trouble seeing the comments that were directly related to the issue, so I just hid some comments as off-topic.

@nodejs/v8 the issue here clearly looks like a V8 bug. Would it be possible to fix getPropertyNames using IndexFilter::kSkipIndices to filter indices even if there is a proxy trap? See #41714 (comment)

[BridgeAR]

There is actually an explicit comment about the behavior in V8:

node/deps/v8/src/objects/keys.h

Lines 33 to 37 in dabda03

	// This is a helper class for JSReceiver::GetKeys which collects and sorts keys.
	// GetKeys needs to sort keys per prototype level, first showing the integer
	// indices from elements then the strings from the properties. However, this
	// does not apply to proxies which are in full control of how the keys are
	// sorted.

We could either implement the behavior in FilterProxyKeys or have to check for proxies on our own and do the filtering manually as @benjamingr suggested.

node/deps/v8/src/objects/keys.cc

Lines 182 to 213 in dabda03

	MaybeHandle<FixedArray> FilterProxyKeys(KeyAccumulator* accumulator,
	Handle<JSProxy> owner,
	Handle<FixedArray> keys,
	PropertyFilter filter) {
	if (filter == ALL_PROPERTIES) {
	// Nothing to do.
	return keys;
	}
	Isolate* isolate = accumulator->isolate();
	int store_position = 0;
	for (int i = 0; i < keys->length(); ++i) {
	Handle<Name> key(Name::cast(keys->get(i)), isolate);
	if (key->FilterKey(filter)) continue; // Skip this key.
	if (filter & ONLY_ENUMERABLE) {
	PropertyDescriptor desc;
	Maybe<bool> found =
	JSProxy::GetOwnPropertyDescriptor(isolate, owner, key, &desc);
	MAYBE_RETURN(found, MaybeHandle<FixedArray>());
	if (!found.FromJust()) continue;
	if (!desc.enumerable()) {
	accumulator->AddShadowingKey(key);
	continue;
	}
	}
	// Keep this key.
	if (store_position != i) {
	keys->set(store_position, *key);
	}
	store_position++;
	}
	return FixedArray::ShrinkOrEmpty(isolate, keys, store_position);
	}

[BridgeAR]

It is actually somewhat confusing for me that there's a separate IndexFilter vs. PropertyFilter.

[targos]

What should we do with this?

[BridgeAR]

I opened https://bugs.chromium.org/p/v8/issues/detail?id=13728. Let's see if this can be fixed in V8.

[debadree25]

I tried a solution something like

diff --git a/deps/v8/src/objects/keys.cc b/deps/v8/src/objects/keys.cc
index a0796864f1..4f84cd9094 100644
--- a/deps/v8/src/objects/keys.cc
+++ b/deps/v8/src/objects/keys.cc
@@ -182,8 +182,9 @@ ExceptionStatus KeyAccumulator::AddKeys(Handle<JSObject> array_like,
 MaybeHandle<FixedArray> FilterProxyKeys(KeyAccumulator* accumulator,
                                         Handle<JSProxy> owner,
                                         Handle<FixedArray> keys,
-                                        PropertyFilter filter) {
-  if (filter == ALL_PROPERTIES) {
+                                        PropertyFilter filter,
+                                        bool skip_indices) {
+  if (filter == ALL_PROPERTIES  && !skip_indices) {
     // Nothing to do.
     return keys;
   }
@@ -191,7 +192,7 @@ MaybeHandle<FixedArray> FilterProxyKeys(KeyAccumulator* accumulator,
   int store_position = 0;
   for (int i = 0; i < keys->length(); ++i) {
     Handle<Name> key(Name::cast(keys->get(i)), isolate);
-    if (key->FilterKey(filter)) continue;  // Skip this key.
+    if (key->FilterKey(filter) || (skip_indices && key->IsNumber())) continue;  // Skip this key.
     if (filter & ONLY_ENUMERABLE) {
       PropertyDescriptor desc;
       Maybe<bool> found =
@@ -218,7 +219,7 @@ Maybe<bool> KeyAccumulator::AddKeysFromJSProxy(Handle<JSProxy> proxy,
   // Postpone the enumerable check for for-in to the ForInFilter step.
   if (!is_for_in_) {
     ASSIGN_RETURN_ON_EXCEPTION_VALUE(
-        isolate_, keys, FilterProxyKeys(this, proxy, keys, filter_),
+        isolate_, keys, FilterProxyKeys(this, proxy, keys, filter_, skip_indices_),
         Nothing<bool>());
   }
   // https://tc39.es/ecma262/#sec-proxy-object-internal-methods-and-internal-slots-ownpropertykeys

On v8 but doesn't seem to make a change 😕

[debadree25]

Hey! was able to get this fixed in v8 @ https://chromium.googlesource.com/v8/v8/+/975ff4dbfd1be3a7395e26d412774bc955b47341 so the next update of v8 in node should fix it or do we have to patch the commit here?

cc @benjamingr @BridgeAR