Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

embedding: allow creating context within inspectable node::Environment #31424

Closed
wants to merge 4 commits into from

Conversation

Dragiyski
Copy link

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

NodeJS allow native addons that can access V8 environment through V8 api or through node namespace. Currently there are two options to create v8::Context: v8::Context::New and node::NewContext. Once created the embedder can create JavaScript code using v8::ScriptCompiler. Unfortunately, this JS code cannot be inspected by node --inspect option. The reason is that the internal handle of the v8::Context does not have associated debug_context_id and the inspector frontend is not notified for the creation of the new context. For example functions created in the new context through v8::ScriptCompiler::CompileFunctionInContext would not have a location and their source would not be visible in the inspector. This might be the module's intended behavior.

Image source not visible in inspector

However, C++ addons that compile JavaScript code operating in different context within the same isolate, might want to allow users to debug their code. This could only happen when a node::Environment exists with an existing inspector agent client. Embedders can still write their own inspector agents in case of different types of embedding, but C++ addons should rely on the existing client provided by the --inspect option.

This is a proposal for a solution to this problem. A C++ addon could create thin v8::Context (similar to v8::Context::New) using polymorphic variant of node::NewContext or alternatively it can specify parameter to create full node::NewContext (with initialization). In both cases, since an environment is provided, it can be assigned to new context which will allow:

  • preview of any compiled code within that context in the inspector (the new context is reported to the inspector with ContextCreated notification);
  • v8::PrepareStackTraceCallback does not modify stack traces for contexts that are not assigned to an node::Environment; Embedder could potentially fix this by assign new callback, but the callback is assigned to an isolate and the current isolate might (and almost always will be) the node's isolate, thus rendering large part of the node's internal unusable (i.e. it is bad solution); With the new feature the embedder does not need to do that.
  • node_contextify is the way to create new context within nodejs's JavaScript environment. However, it is difficult (not impossible though) for C++ addon to require('vm') and the newly created context will have global based on some inner v8::FunctionTemplate. C++ addons and other embedders might need to be able to create v8::Context with an existing v8::ObjectTemplate and still compile inspectable code.

A more elaborate feature could be later implemented to handle situations where more than one node::Environment with an inspector agent exists for the same isolate, or to handle debugging code within a node::MultiIsolatePlatform, etc. However, currently for C++ addons, this solution is better, because they will operate within only one node::Environment, and the solution does not require any changes to an existing functions, or any additional memory.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. v8 engine Issues and PRs related to the V8 dependency. labels Jan 20, 2020
deps/v8/src/compiler/functional-list.h Outdated Show resolved Hide resolved
src/api/environment.cc Show resolved Hide resolved
src/api/environment.cc Outdated Show resolved Hide resolved
@addaleax addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 20, 2020
Co-Authored-By: Anna Henningsen <[email protected]>
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the goal is to call Environment::AssignToContext() then I think the current approach is a little on the inflexible side.

For one, it turns the context into a Node.js context. That's something you as an embedder may not necessarily want because it means, for example, that Atomics.wake() is stripped out.

A better (but still not perfect) approach is:

  1. Embedder creates a Context
  2. Embedder optionally calls InitializeContext() to turn it into a Node.js context
  3. Embedder calls AssignToContext()

(Open question: how much of ContextInfo to pass to node::AssignToContext(). I'd rather not make that class public because that sets its API and ABI in stone.)

That said, I expect that what you really want is to be able to call v8_inspector::V8Inspector::contextCreated() and v8_inspector::V8InspectorcontextDestroyed().

Maybe we should think about exposing Node's instance of v8_inspector::V8Inspector. That probably requires some refactoring on our side because we currently treat it more as a per-context/per-environment property than the per-isolate property that it really is.

@@ -362,6 +362,13 @@ NODE_EXTERN v8::Local<v8::Context> NewContext(
v8::Local<v8::ObjectTemplate> object_template =
v8::Local<v8::ObjectTemplate>());

// Create a new context for an existing environment.
// This add several fields to make inspector work properly.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// This add several fields to make inspector work properly.
// This adds several fields to make the inspector work properly.

Copy link
Author

@Dragiyski Dragiyski Jan 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would disagree, since there are three major problems in what you have described:

  • The API through which v8_inspector::V8Inspector is accessed must be aware both in compile-time and in runtime whether an inspector agent actually exists (currently node::Environment does that);
  • v8_inspector::V8Inspector::ContextCreated() is intended to use from the context-creating functions. It is not a notification and it will happily re-assign the debug_context_id leading to undefined behavior for any outstanding requests from the frontend with the old debug_context_id.
  • In the V8 API there are per-isolate properties like v8::Isolate::SetPrepareStackTraceCallback(). If the environment does not handle that there should be another class in the hierarchy: node::MultiIsolatePlatform -> (per-isolate object) -> node::Environment. This would require major untangling of the node::Environment and it is not related to the main goal here: creating a new context.

I think that hiding the context creation process steps from the API is way more stable than exposing the inspector agent to the embedder. Currently node::Environment perform some per-isolate processes like v8::Isolate::SetPrepareStackTraceCallback. In the V8 engine v8::Context objects are part of v8::Isolate, and there are certain API that are per-isolate. Therefore, if node::Environment does not handle the per-isolate API, there must be another class that handles it (and node::Environment would probably contains shared pointer towards it). And since the inspector agent would always be hidden within an instance of a node's class (see the first two points above), the best approach for the context creation process would be to have node::NewContext that accepts the node's per-isolate object and perform node's specific initialization (like reporting the context to its inspector agent).

For the current minor version, node::Environment is still per-isolate, and the exposed function signature allow proper initialization with provided embedder's v8::ObjectTemplate while reporting to the inspector agent is not exposed and how it is done can be changed in the future versions.

The only problem is the initialize argument. To become future-proof compatible, it would be better if the third parameter is some white-box (publicly exposed) structure, that can be expanded with other properties in the future versions.

The last parameter currently apply only to initialization.
Using a structire instead ensures that the API will remain
the same even when the structure changes.
@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:19
@jasnell jasnell added the stalled Issues and PRs that are stalled. label Jun 25, 2020
@jasnell
Copy link
Member

jasnell commented Jun 25, 2020

Looks like this needs a rebase and more discussion to move forward. @Dragiyski, do you intend to keep moving this forward?

@Dragiyski
Copy link
Author

Looks like this needs a rebase and more discussion to move forward. @Dragiyski, do you intend to keep moving this forward?

No, as both vm and inspector get updates, what the pull request original intention is no longer necessary.

@Dragiyski Dragiyski closed this Jun 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. semver-minor PRs that contain new features and should be released in the next minor version. stalled Issues and PRs that are stalled. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants