Object::PreviewEntries returns incorrect results for Set Iterator

[RbertKo]

• Version: 10.10 -> 11.0 this two version have bug too
• Platform: IDE : vscode
• Subsystem:

let set = new Set();
set.add(1); 
set.add(2); 
set.add(3);
set.add(1); 
set.add(2); 
set.add(3);
set.delete(1);
console.log( set.size );
console.log( set.has(3) );
console.log( set.has(2) );
console.log( set.has(1));
console.log( set.keys());
console.log( set.values() );
console.log( set.entries() );

return :

2
true
true
false
[Set Iterator] { 3 }
[Set Iterator] { 3 }

I added 1, 2, 3 in 'set' instance. and I deleted '2'.
so, 'set' have '2', '3' element, but 'set' return values only '3'.
set.has(2) show 'true'!

10.10 version had a bug, so i upgraded my node version to 11.0, but can't resolve it.

[devsnek]

It looks like the inspector is wrong. The set still has 2 and 3 if you actually iterate over it: for (const element of a) { console.log(e); }

cc @BridgeAR

[RbertKo]

It looks like the inspector is wrong. The set still has 2 and 3 if you actually iterate over it: for (const element of a) { console.log(e); }

cc @BridgeAR

yeah, i think too. so, set.had(2) is returned 'true'.

[devsnek]

further debugging reveals that the problem lies in Object::PreviewEntries. @nodejs/v8

[ghost]

After all the addition to the set, the set looks like this: Set { 1, 2, 3 }
Next you execute: set.delete(1); which removes the element 1 from the set so now the set looks like this: Set { 2, 3 }
Isn't this the right behavior?
The set.delete function takes the value which it removes from the set and that's what it is doing here( In the snippet written in the description), removing 1 from the set

[devsnek]

@Flowkrad the issue is the util.inspect output of the set iterator.

const a = new Set([1, 2, 3]);
a.delete(1);
util.inspect(a); // Set { 2, 3 }
util.inspect(a.keys()); // [Set Iterator] { 3 }

^ @addaleax @nodejs/v8 minimal repro btw

[ghost]

Thank for pointing that out (^_^)
P.S. Got confused when RobertKo replied with:

yeah, i think too. so, set.had(2) is returned 'true'.

[shobhitchittora]

Yeah! Happens with Map iterator too. Any idea how to debug in c++. I'm using lldb and find it difficult to inspect values.

node/src/node_util.cc

Line 91 in 56ce6f1

static void PreviewEntries(const FunctionCallbackInfo<Value>& args) {

[addaleax]

@shobhitchittora Have you seen https://github.com/nodejs/llnode/? It’s pretty neat for debugging Node/V8 code.

[shobhitchittora]

@addaleax Sounds cool! Thanks. Let me have a look at it.

[shobhitchittora]

On further inspection, this happens only when you delete the first value in a set or a map.

const s = new Set([1,2,3])

// this creates problem
s.delete(1)
s.keys() // [3]

// while this works fine
s.delete(2)
s.keys()

[shobhitchittora]

More investigation shows that the latest Chrome also has the same behavior. Checking v8 bug list for the same.

Reported the same to upstream v8 - https://bugs.chromium.org/p/v8/issues/detail?id=8433.

Edit - v8 api code pointer - https://github.com/v8/v8/blob/master/src/api.cc#L9512

[addaleax]

@shobhitchittora Okay, sounds like nothing we could fix on the Node side? If you end up fixing this in V8 and want any guidance on how to submit a patch to them (this can be tricky if you’re not familiar with it), just let us know. :)

[hashseed]

I already have a fix. Coming up soon.

[shobhitchittora]

Hey @addaleax The fix has been merged as per this - https://chromium.googlesource.com/v8/v8/+/88f8fe19a863c6392bd296faf86c06eff2a41bc1.

How can we now get the dep updated in node?

[Trott]

@hashseed At this point, is there anything left to do besides wait for your fix to end up in a V8 release, and then wait for Node.js to pick up that V8 release? /cc @addaleax

[targos]

Fixed in #24514