src: remove dependency on wrapper-descriptor-based cpp heap

[joyeecheung]

This is baking for the V8 12.5 upgrade so the base branch is canary-base. Original PR and reviews are in https://github.com/nodejs/node-v8/pull/284

As V8 is moving towards built-in CppHeap creation, change the management so that the automatic CppHeap creation on Node.js's end is also enforced at Isolate creation time.

1. If embedder uses NewIsolate(), either they use IsolateSettings::cpp_heap to specify a CppHeap that will be owned by V8, or if it's not configured, Node.js will create a CppHeap that will be owned by V8.
2. If the embedder uses SetIsolateUpForNode(), IsolateSettings::cpp_heap will be ignored (as V8 has deprecated attaching CppHeap post-isolate-creation). The embedders need to ensure that the v8::Isolate has a CppHeap attached while it's still used by Node.js, preferably using v8::CreateParams.

See https://issues.chromium.org/issues/42203693 for details. In future version of V8, this CppHeap will be created by V8 if not provided, and we can remove our own "if no CppHeap provided, create one" code in NewIsolate().

This also deprecates node::SetCppgcReference() in favor of v8::Object::Wrap() since the wrapper descriptor is no longer relevant. It is still kept as a compatibility layer for addons that need to also work on Node.js versions without v8::Object::Wrap().

Fixes: https://github.com/nodejs/node-v8/issues/283 Fixes: https://github.com/nodejs/node/issues/52718

[nodejs-github-bot]

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

[targos]

Looks like some things are broken.

[joyeecheung]

Looks like we have a timing conflict with https://github.com/nodejs/node/issues/30846#issuecomment-564365365 - if we migrate to the new CppHeap management, the CppHeap is torn down during Isolate disposal, and at this point the Isolate needs to be registered with the Platform for it to look up the task runner. But due to https://github.com/nodejs/node/issues/30846#issuecomment-564365365 we also had to unregister the Isolate before the disposal to avoid the mentioned race condition.

I wonder if this can be worked around by using something else as the Isolate map keys in the platform to avoid the said race condition & allow us to tear down the isolate while it's still discoverable by the platform...maybe using a self-incrementing ID as key is already enough? cc @addaleax

[addaleax]

@joyeecheung Yeah ... I assume in the end any key that we can look up based on the Isolate* pointer, without needing the Isolate instance itself, will work? But obviously the pointer itself is a pretty natural choice ... would it be possible to split Isolate disposal into two parts, like Isolate creation (i.e. split into allocation + initialization)? That requires a V8 change but might be the best solution apart from that

[joyeecheung]

I wonder if there's another way to fix the isolate disposal race condition...by shutting down the platform data/task runner after we remove it from the map? In that case it's not possible for the conflicting new Isolate to be mapped to the platform data being shut down since it's already out of the map and is doing its own thing. Locally that seems fine and doesn't regress the test, although I am unable to reproduce the flake in https://github.com/nodejs/node/issues/30846 after reverting the dispose-unregister order either, I'll open a separate PR for this change and investigate.

[targos]

I just had to force-push canary-base to remove https://github.com/nodejs/node/commit/919759814bf039168ca21100bc39c167b93575da (it's now on main).

[targos]

https://github.com/v8/node/pull/187 may be relevant

[joyeecheung]

https://github.com/v8/node/pull/187 contains part of what this PR does. Maybe we can just migrate the wrapper descriptors stuff first and deal with the Isolate teardown stuff in a separate PR, as that is a bit more complicated.

[joyeecheung]

Updated this branch on top of canary-base and split out the wrapper descriptor part. The CppHeap management + Isolate registration changes are now in https://github.com/nodejs/node/pull/53205 Although this is now failing to compile in the CI since the last canary-base update because there are some issues with -Werror=comment + some comment in v8-sandbox.h not playing together. Locally on macOS this passes the tests.

[github-actions[bot]]

Failed to start CI

   ⚠  No approving reviews found
   ✘  Refusing to run CI on potentially unsafe PR

https://github.com/nodejs/node/actions/runs/9291289224

[nodejs-github-bot]

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

[joyeecheung]

Pushed a commit here to fix the -Werror=comment errors in v8-sandbox.h to see if Linux CI is happy with it..

[targos]

I don't know if it's related but there are many crashes with heapdump/heapsnapshot tests: https://ci.nodejs.org/job/node-test-commit-arm-debug/13095/

[joyeecheung]

The heap snapshot failures are the same as https://github.com/nodejs/node-v8/issues/282 which are being fixed in https://chromium-review.googlesource.com/c/v8/v8/+/5531355

[joyeecheung]

v8-sandbox.h fixes upstreamed in https://chromium-review.googlesource.com/c/v8/v8/+/5584738

[nodejs-github-bot]

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

[joyeecheung]

Not sure what's the way to make progress here...the CI failures are unrelated AFAICT, but the test won't be passing since canary-base isn't working yet. What's the policy for landing patches on canary-base?

Also cc @addaleax @legendecas who reviewed the original PR.

[targos]

There isn't really a policy. From my PoV, if it is brings us towards a passing canary build, this can land.

[targos]

I updated canary-base so it includes https://github.com/v8/v8/commit/300451e869ae908f9a454c5a265162a6b3960b5d and pushed https://github.com/nodejs/node/commit/30329d06235a9f9733b1d4da479b403462d1b326 to it. Thank you!