lib: add WeakRef and FinalizationRegistry to primordials

[ExE-Boss]

This cleans up the two TODOs in:

node/lib/internal/util/iterable_weak_map.js

Lines 11 to 21 in aac2713

	// TODO(aduh95): Add FinalizationRegistry to primordials
	const SafeFinalizationRegistry = makeSafe(
	globalThis.FinalizationRegistry,
	class SafeFinalizationRegistry extends globalThis.FinalizationRegistry {}
	);
	// TODO(aduh95): Add WeakRef to primordials
	const SafeWeakRef = makeSafe(
	globalThis.WeakRef,
	class SafeWeakRef extends globalThis.WeakRef {}
	);

[benjamingr]

Thanks!

Any chance you could also make the (new'ish) weak event handler machinery in event_target use this? (It's the other place I know in the code that uses FinalizationRegistry and WeakRef).

[targos]

I wouldn't expect this to work yet. WeakRef can still be disabled with a V8 flag (--no-harmony-weak-refs) so they are not available while bootstrapping.

[devsnek]

I might just go change that, moving the error snapshot use instead of creation.

[jasnell]

FWIW, Node.js already fails to start when the --no-harmony-weak-refs flag is used...

root@DESKTOP-5KK9VIR:~/node/node# ./node --no-harmony-weak-refs
node:internal/util/iterable_weak_map:14
  class SafeFinalizationRegistry extends globalThis.FinalizationRegistry {}
                                                    ^

TypeError: Class extends value undefined is not a constructor or null
    at node:internal/util/iterable_weak_map:14:53
    at NativeModule.compileForInternalLoader (node:internal/bootstrap/loaders:283:7)
    at nativeModuleRequire (node:internal/bootstrap/loaders:312:14)
    at node:internal/source_map/source_map_cache:28:29
    at NativeModule.compileForInternalLoader (node:internal/bootstrap/loaders:283:7)
    at nativeModuleRequire (node:internal/bootstrap/loaders:312:14)
    at node:internal/modules/cjs/loader:78:5
    at NativeModule.compileForInternalLoader (node:internal/bootstrap/loaders:283:7)
    at nativeModuleRequire (node:internal/bootstrap/loaders:312:14)
    at node:internal/modules/esm/loader:4:1

[devsnek]

@jasnell the issue is that V8 disables flagged globals during snapshot, regardless of whether you expect them to be available later or not.

[aduh95]

I'v e submitted https://chromium-review.googlesource.com/c/v8/v8/+/2741582/ to unblock this.

[devsnek]

@aduh95 cloudflare uses that flag for workers, probably can't land.

[devsnek]

I'd like to talk to the V8 team about a new flag system which allows them to be snapshotted, but they don't seem to care at all about our design constraints or maintaining contact with us so it's been delayed.

[aduh95]

Flag has been removed upstream, I've opened #38162 to test if that makes the tests pass.

[aduh95]

Tests are passing :) @ExE-Boss I've forced-pushed to your branch to include the V8 patch, please have a look in case I did something wrong (oh and I added f98f3b551cc5433ba2224178e6f1a03f0af32769, I hope that's OK; if you disagree of course feel free to revert).

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[aduh95]

Landed in 78343bb