src: expose node::MakeContextify to make a node managed vm context

[legendecas]

This allows addons to create a context, with Node.js inspector support.
The created context is identicaly to the one created with vm.createContext
in node:vm. However, node:vm does not allow an addon to unwrap the
v8::Context out of the sandbox object.

The API exposed is intended to be minimum, so that it will not block
the pending vm Modules API.

[nodejs-github-bot]

Review requested:

• @nodejs/gyp

[addaleax]

The created context is identicaly to the one created with vm.createContext
in node:vm. However, node:vm does not allow an addon to unwrap the
v8::Context out of the sandbox object.

The API exposed is intended to be minimum

That kinda sounds like what we primarily need/want is an API to unwrap the v8::Context from a sandbox object – couldn't we do that instead? (It's also not super difficult to do that tbh, you could run vm.runInContext('{}', context) and do obj->GetCreationContext() on the result)

[addaleax]

I'd suggest adding a version field on this struct for easier ABI compatibility if we want to make changes at some point

[codecov]

Codecov Report

❌ Patch coverage is 81.39535% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.69%. Comparing base (06a8240) to head (7a31490).
⚠️ Report is 18 commits behind head on main.

Files with missing lines	Patch %	Lines
src/api/environment.cc	78.94%	5 Missing and 3 partials ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main   #62322    +/-   ##
========================================
  Coverage   89.68%   89.69%            
========================================
  Files         676      676            
  Lines      206578   206736   +158     
  Branches    39555    39587    +32     
========================================
+ Hits       185267   185423   +156     
  Misses      13446    13446            
- Partials     7865     7867     +2     

Files with missing lines	Coverage Δ
src/node.h	92.98% <100.00%> (+0.67%)	⬆️
src/node_contextify.h	82.35% <ø> (ø)
src/api/environment.cc	79.21% <78.94%> (+0.13%)	⬆️

... and 44 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[legendecas]

@addaleax: That kinda sounds like what we primarily need/want is an API to unwrap the v8::Context from a sandbox object – couldn't we do that instead?

Yeah, that's the main purpose. We could expose an API to unwrap the v8::Context out from sandbox object directly. However, I feel like it would be more straitforward to allow addons to create and manage a Node.js managed v8::Context directly, similar to node::NewContext (maybe we should encourage addons to use this new API instead of node::NewContext because it is not configured to support inspector).

[addaleax]

We could expose an API to unwrap the v8::Context out from sandbox object directly.

I still feel like that's both easier and a bit more powerful, but I'll leave it up to you 👍

[legendecas]

I still feel like that's both easier and a bit more powerful

I can add both. A node::MakeContextify could avoid jumping between JS and C++ unnecessarily for creating a context, and as a replacement of node::NewContext. And an unwrap method like you suggested could help dealing with existing vm contexts.

[joyeecheung]

Is this mostly for that NodeInspectorClient::contextCreated call?

I think alternatively we can consider just exposing an API that returns an opaque pointer to the NodeInspectorClient, and selectively expose some APIs that are stable. That might allow better inspector integration for embedders (e.g. dispatching inspector messages to Node.js more directly so that they surface nicely in the devtools through the protocol). I don't think exposing vm context to the API would hinder other vm efforts since they are a bit orthogonal, but vm context's interceptor behaviors are quite quirky, so by using that the embedders are also inheriting the quirks that are mostly only there due to a specific JS API compat. If the overall desirable thing lies in inspector instead it seems like an unnecessary baggage, a normal v8::Context + embedder defined global behavior is much less quirky than the interceptors (this is why we have vm.constants.DONT_CONTEXTIFY, because even in JS land it's less weird to just not contextify it and define what you need directly on the global proxy instead of going through the legacy interceptors).

[joyeecheung]

I think unless you are specifically after those weird quirky interceptor behaviors e.g. #31808, if we want a Node.js-managed context API, the most natural thing is to have DONT_CONTEXTIFY semantics i.e. context->Global() is that sandbox/proxy that embedders can manipulate (for this to work the internal plumbing, just leave sandbox_obj as empty, so that it goes this path, and have most vm context initializations including that NodeInspectorClient::contextCreated call, except the installation of quirky interceptors).

[legendecas]

If the context_object is empty, the context is initialized as vanilla mode (or say DONT_CONTEXTIFY).