vm: return all own names and symbols in property enumerator

[legendecas]

Property enumerator methods like Object.getOwnPropertyNames,
Object.getOwnPropertySymbols, and Object.keys all invokes the
named property enumerator interceptor. V8 will filter the result based
on the invoked enumerator variant. Fix the enumerator interceptor to
return all potential properties.

Refs: jsdom/jsdom#3688

/cc @nodejs/vm

[joyeecheung]

globalThis[Symbol.toStringTag] was unconditionally initialized
to the sandbox's constructor name, even if it is not present on the
sandbox object.

The original behavior was something like this:

node/src/node_contextify.cc

Line 202 in 5737c21

function_template->SetClassName(sandbox->GetConstructorName());

When we still used a function template's instance template for the global object template. The [Symbol.toStringTag] setting was used to make it look the same while allowing different vm contexts to use different global object templates, so that contexts can share a snapshot. I am not sure how necessary it is to ensure that the globalThis in the new context has the same name as the sandbox's constructor, though. Maybe another alternative is to return something specific for this in the getter/setter/definer interceptors?

[codecov]

Codecov Report

Attention: Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review.

Project coverage is 87.31%. Comparing base (cc26951) to head (acf009b).
Report is 354 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_contextify.cc	87.50%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #54522 +/- ## ========================================== - Coverage 87.34% 87.31% -0.03%  ========================================== Files 649 649 Lines 182544 182759 +215 Branches 35030 35048 +18 ========================================== + Hits 159445 159579 +134 - Misses 16372 16459 +87 + Partials 6727 6721 -6 

Files with missing lines	Coverage Δ
src/node_contextify.cc	80.73% <87.50%> (+0.10%)	⬆️

... and 72 files with indirect coverage changes

[legendecas]

@joyeecheungFunctionTemplate::SetClassName does not set up @@ToStringTag automatically. AFAICT, it is used as the function name and template uniqueness check.

On v18.8.0 where #44252 was not backported, the @@ToStringTag is not installed on the vm globalThis.

[joyeecheung]

Yes, hence it was just done to keep it “look the same as before” (which matters when it gets logged by the console, etc. I don’t know how important it is though, maybe it’s fine to break the display name)

[legendecas]

I left @@toStringTag untouched being an exception as is. The breaking behavior can be a separate PR. This PR still fix inconsistency on Object.getOwnPropertyNames, Object.getOwnPropertySymbols, and Object.keys.

@nodejs/vm @joyeecheung please take a look again, thanks!

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/61654/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/61664/

[nodejs-github-bot]

Landed in d2479fa