Interest in solving issue id 13728

[Debadree Chatterjee]

Hello v8-dev,

I am trying to work on the issue https://bugs.chromium.org/p/v8/issues/detail?id=13728 which is a bug noticed from nodejs, as a first time contributor the docs suggest that I reach out here first before working on a issue. Already some of the debugging of the issue has been done on the original thread on nodejs repo @ https://github.com/nodejs/node/issues/41714, the crux of the matter being that the function `FilterProxyKeys` seems to ignore the `v8::IndexFilter::kSkipIndices` property passed to `GetPropertyNames`, I attempted to solve this with the following diff

```diff
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
```

but this doesn't seem to work mainly because I think I am using `key->IsNumber()` wrong. I am not sure what would be the correct way to check if the given key is indeed an index hence I ask for any guidance here, I understand this is a very beginner question but any help would be greatly appreciated.

Your Sincerely,
Debadree

[Debadree Chatterjee]

Anu guidance toward any relevant documentation would also be helpful

[Leszek Swirski]

Hi Debadree,

Sorry for not replying to your original email - often with bugs like this, the reason we haven't fixed them is because no one thought about it too much, and thinking about the solution is 90% of the time needed to actually fix it.

In your case, I haven't looked at the details of the issue whether your overall approach is correct - at a skim it seems right but you never know with weird edge cases. To answer your question about IsNumber, you likely want to also check for index strings, like "123". You can detect these with String::AsArrayIndex, which returns false if the conversion to a numeric index fails.

Hope that's helpful,

Leszek

--
--
v8-dev mailing list
v8-...@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+un...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/9307c0b4-fd2c-4774-9614-71d0f7799091n%40googlegroups.com.

[Debadree Chatterjee]

Hi Leszek,

Thank you so much for the response, will try to apply your suggestions and iron out any bugs and submit a CL, will ask here if encounter anything I don't understand

Thank you once again!

Yours Sincerely,

Debadree

[Jakob Kummerow]

On Mon, Mar 13, 2023 at 8:59 AM Debadree Chatterjee <debad...@gmail.com> wrote:

Hi Leszek,

Thank you so much for the response, will try to apply your suggestions and iron out any bugs and submit a CL, will ask here if encounter anything I don't understand

Thank you once again!

Yours Sincerely,

Debadree

On Monday, March 13, 2023 at 12:30:11 PM UTC+5:30 les...@chromium.org wrote:

Hi Debadree,

Sorry for not replying to your original email - often with bugs like this, the reason we haven't fixed them is because no one thought about it too much, and thinking about the solution is 90% of the time needed to actually fix it.

In your case, I haven't looked at the details of the issue whether your overall approach is correct - at a skim it seems right but you never know with weird edge cases. To answer your question about IsNumber, you likely want to also check for index strings, like "123".

I think that's "instead", not "also". Asking ->IsNumber() for something that's typed Handle<Name> never makes sense, because it'll always return false, because Names aren't Numbers.

[Debadree Chatterjee]

Hi 

Yes was wrong in using IsNumber(), have corrected that, added tests and have submitted a CL here https://chromium-review.googlesource.com/c/v8/v8/+/4333698, I dont know who exactly to cc as reviewers hence posting here only.

Yours Sincerely,

Debadree