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

inspector: Allow require in Runtime.evaluate #8837

Closed
wants to merge 3 commits into from

Conversation

jkrems
Copy link
Contributor

@jkrems jkrems commented Sep 28, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

inspector

Description of change

Being able to require modules from the console can be very handy while debugging issues. This allows to do it even when require is no longer in scope or the process isn't currently paused. It also creates the foundation for adding other command line API methods in the future (if more use cases come up).

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. inspector Issues and PRs related to the V8 inspector protocol labels Sep 28, 2016
@jkrems
Copy link
Contributor Author

jkrems commented Sep 28, 2016

I wasn't sure if this warrants a mention in the docs. So far I assume that require is something people will implicitly assume works (the behavior is roughly the same as in the repl / node -e / etc.).

@@ -225,12 +225,27 @@
global.setTimeout = timers.setTimeout;
}

function setupConsoleAPI(addCommandLineAPIMethod) {
addCommandLineAPIMethod('require', function require(request) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of passing, this could just access process.inspector.addCommandLineAPIMethod directly, whcih would be preferred.

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I would move this below setupGlobalConsole

Copy link
Contributor Author

@jkrems jkrems Sep 28, 2016

Choose a reason for hiding this comment

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

Moved setupConsoleAPI below and renamed it to setupInspectorAPI (closer to inspector.addCommandLineAPIMethod). Also switched to accessing the method from process instead of passing it.

@@ -249,6 +250,21 @@
});
}

function setupInspectorAPI() {
var addCommandLineAPIMethod = process.inspector.addCommandLineAPIMethod;
Copy link
Member

Choose a reason for hiding this comment

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

nit: const?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -151,6 +151,37 @@ function testInspectScope(session) {
]);
}

function testCommandLineAPI(session) {
console.log('[test]', 'Verify that command line API was injected');
Copy link
Member

@jasnell jasnell Sep 30, 2016

Choose a reason for hiding this comment

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

nit: I'd prefer if the tests did not include unnecessary console.log statements

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here - should I remove the console.log from the other functions in this file as well? Only added it for consistency with the existing code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the console.log I added already.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM if CI is happy and @Fishrock123's concerns are met.


void AgentImpl::AddAPIMethod(const v8::FunctionCallbackInfo<v8::Value>& info) {
auto env = Environment::GetCurrent(info);
v8::Local<v8::Object> console_api = env->inspector_console_api_object();
Copy link
Member

Choose a reason for hiding this comment

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

Could you check arguments here and ThrowError if something wrong in the same way like in InspectorWrapConsoleCall?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

@@ -573,6 +585,16 @@ void AgentImpl::InstallInspectorOnProcess() {
v8::Local<v8::Object> inspector = v8::Object::New(env->isolate());
READONLY_PROPERTY(process, "inspector", inspector);
env->SetMethod(inspector, "wrapConsoleCall", InspectorWrapConsoleCall);
env->SetMethod(inspector, "addCommandLineAPIMethod", AddAPIMethod);
Copy link
Member

Choose a reason for hiding this comment

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

nit: I'd prefer InspectorAddCommandLineAPIMethod or AddCommandLineAPIMethod as a name of this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the method name might have gotten shorter after I discovered the 80 char limit... Went back to a more descriptive name & more line breaks.

src/env.h Outdated
@@ -240,6 +240,7 @@ namespace node {
V(domains_stack_array, v8::Array) \
V(fs_stats_constructor_function, v8::Function) \
V(generic_internal_field_template, v8::ObjectTemplate) \
V(inspector_console_api_object, v8::Object) \
Copy link
Member

Choose a reason for hiding this comment

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

Just to check, is this object accessible from JS code?

Copy link
Contributor

Choose a reason for hiding this comment

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

this isn't an object per-se, it is not exposed to JS

Copy link
Contributor Author

@jkrems jkrems Oct 3, 2016

Choose a reason for hiding this comment

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

As @Fishrock123 said, this is just used as a bucket of named persistent values from native code, it's never directly exposed to JS.

@@ -231,6 +231,7 @@
if (process.inspector) {
inspectorConsole = global.console;
wrapConsoleCall = process.inspector.wrapConsoleCall;
setupInspectorAPI();
Copy link
Member

Choose a reason for hiding this comment

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

nit: I'd prefer API -> commandLineAPI. API sounds too broad for me.

Copy link
Contributor

Choose a reason for hiding this comment

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

nah, I think this is fine.

Copy link
Member

Choose a reason for hiding this comment

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

I'm with @ak239, setupInspectorCommandLineAPI() is a more descriptive name.

@@ -249,6 +250,21 @@
});
}

function setupInspectorAPI() {
var addCommandLineAPIMethod = process.inspector.addCommandLineAPIMethod;
Copy link
Member

@alexkozy alexkozy Sep 30, 2016

Choose a reason for hiding this comment

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

Can we remove this method from inspector object right here to be sure that it won't leak?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, will do.

@@ -253,6 +253,7 @@ class AgentImpl {
static void WriteCbIO(uv_async_t* async);

void InstallInspectorOnProcess();
static void AddAPIMethod(const v8::FunctionCallbackInfo<v8::Value>& info);
Copy link
Member

Choose a reason for hiding this comment

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

Could you define this method outside of AgentImpl?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this out and renamed (see below).

@@ -249,6 +250,21 @@
});
}

function setupInspectorAPI() {
const addCommandLineAPIMethod = process.inspector.addCommandLineAPIMethod;
addCommandLineAPIMethod('require', function require(request) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this callback called synchronously? if not, I need to inspect the startup timing a bit more

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The line you're commenting on is executed synchronously during bootup. From my understanding that should prevent any debug client from connecting (and calling Runtime.evaluate) before we registered the API method.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me.

@jkrems
Copy link
Contributor Author

jkrems commented Oct 3, 2016

Sorry for force-pushing the last couple of times, missed that sentence in the contribution guide.

Pushed a commit to address the last round of feedback.

@gibfahn
Copy link
Member

gibfahn commented Oct 3, 2016

@jkrems I don't think force-pushing is a problem, AFAIK it's a matter of personal preference. I like to use --amend so the commits look tidier, but either way should be fine. Which sentence in the contribution guide were you referring to?

@alexkozy
Copy link
Member

alexkozy commented Oct 3, 2016

thanks, lgtm

@@ -567,12 +578,30 @@ void AgentImpl::WaitForDisconnect() {
v8::ReadOnly).FromJust(); \
} while (0)

void AddCommandLineAPIMethod(const v8::FunctionCallbackInfo<v8::Value>& info) {
Copy link
Member

Choose a reason for hiding this comment

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

This can be static, right? If so, it should probably be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My C++ is a bit rusty, so the following might be just me missing one of the "things static can mean": It's no longer a class member but a top-level function as of the latest commit.

Copy link
Member

Choose a reason for hiding this comment

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

@jkrems Yeah, for top-level functions (i.e. in namespace scope, like this one), adding static basically means that the function will only be visible in the file in which it is defined. I think it’s generally good practice to have any top-level functions either appear in a header file to make clear that they are shared between files/part of the public C++ API, or be static to indicate that they are intended to be local. You can grep src/ for FunctionCallbackInfo<Value>& args, most of these functions are static.

(Also, yeah, no doubt that assigning multiple distinct meanings to static is not an ideal thing for C++. 😄)

Copy link
Member

Choose a reason for hiding this comment

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

That's why I have a mild preference for inline. It does the same thing as static but there is never any ambiguity as to whether it gets applied to a function or a variable declaration because it's only valid for the former. Helpful when grepping around.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@addaleax & @bnoordhuis: Thanks a lot for the explanation. I'm pretty sure I learned & forgot this before. Once or twice.

I updated this function and also made the other two exposed C++ methods (InspectorConsoleCall/InspectorWrapConsoleCall) static so the style is consistent inside of the file. Went with static over inline because it seemed to be the dominant style in other *.cc files.

auto env = Environment::GetCurrent(info);

if (info.Length() != 2 || !info[0]->IsString() || !info[1]->IsFunction()) {
return env->ThrowError("inspector.addCommandLineAPIMethod takes exactly "
Copy link
Member

Choose a reason for hiding this comment

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

env->ThrowTypeError?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought so but it was using ThrowError for the same thing above. Should I change both or just my copy?

Copy link
Member

Choose a reason for hiding this comment

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

@jkrems It’s probably okay change both. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@jkrems
Copy link
Contributor Author

jkrems commented Oct 3, 2016

@gibfahn Here's the quote I was referring to:

If there are comments to address, apply your changes in a separate commit and push that to your branch.

@jkrems
Copy link
Contributor Author

jkrems commented Oct 17, 2016

Anything left to address here?

@jkrems
Copy link
Contributor Author

jkrems commented Dec 13, 2016

Rebased on origin/master & make -j8 test (the only conflict was another test case that was added to test/inspector/test-inspector.js in the meantime).

@@ -231,6 +231,7 @@
if (process.inspector) {
inspectorConsole = global.console;
wrapConsoleCall = process.inspector.wrapConsoleCall;
setupInspectorAPI();
Copy link
Member

Choose a reason for hiding this comment

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

I'm with @ak239, setupInspectorCommandLineAPI() is a more descriptive name.

module.filename = path.join(cwd, name);
module.paths = Module._nodeModulePaths(cwd);
return module.require(request);
});
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't module be defined outside the function? I think this breaks require('foo') === require('foo'), i.e., that the second require returns the cached module instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The module cache is maintained by Module, so individual module instances shouldn't matter. It does influence module.parent (e.g. two modules that were loaded initially using the global require will not share the same module.parent). But that seemed less important.

Adding a test case to verify.

Copy link
Member

Choose a reason for hiding this comment

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

Somebody is bound to run into that sooner or later. If it's easy to avoid, I'd fix it now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Somebody is bound to run into that sooner or later.

Can you clarify what "it" is in this context? There's now a test case that verifies that it doesn't break require('foo') === require('foo'), unless I misunderstood your initial concern?

Copy link
Member

Choose a reason for hiding this comment

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

That was my initial concern, yes, but you mentioned that the .parent property will be different for each require() call. If that's easy to avoid, I'd do it now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, will add additional tests for require('foo').parent === require('bar').parent and fix this.

for (uint32_t i = 0; i < properties->Length(); ++i) {
v8::Local<v8::Value> key = properties->Get(i);
target->Set(key, console_api->Get(key));
}
Copy link
Member

Choose a reason for hiding this comment

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

Can you use the GetOwnPropertyNames, Get and Set overloads that take a context as their first argument and return a Maybe/MaybeLocal?

}

v8::Local<v8::Object> console_api = env->inspector_console_api_object();
console_api->Set(info[0], info[1]);
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

}, (message) => assert.strictEqual('object', message['result']['value'])
],
]);
}
Copy link
Member

Choose a reason for hiding this comment

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

Can you check that:

  1. require(name) twice returns the same object, and
  2. that delete require.cache[require.resolve(name)] followed by require again returns a new object, and
  3. that calling require again returns the same object?

name could be e.g. const name = path.join(common.fixturesDir, 'empty');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try. I'm not sure if require.cache is exposed globally by this PR. Since the require goes through the normal module.require, is there any reason to believe that deleting the require cache won't affect it?

@gibfahn
Copy link
Member

gibfahn commented Dec 13, 2016

@jkrems FYI, this is no longer in CONTIBUTING.md, so feel free to squash or not, whatever you prefer...

@jkrems
Copy link
Contributor Author

jkrems commented Dec 13, 2016

Updated, went with adding it as another commit because consistency.

@TimothyGu
Copy link
Member

I'm willing to take this up and fix whatever is needed to get this in, so reopening.

@TimothyGu TimothyGu reopened this Jul 25, 2017
@TimothyGu TimothyGu force-pushed the jk-global-require branch from 6a6619c to 268790f Compare July 25, 2017 14:02
@TimothyGu
Copy link
Member

@bnoordhuis Comments addressed, so PTAL.

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

consoleAPIModule.paths = Module._nodeModulePaths(cwd);
const consoleAPIModule = new Module('<inspector console>');
consoleAPIModule.paths =
Module._nodeModulePaths(cwd).concat(Module.globalPaths);
Copy link
Member

Choose a reason for hiding this comment

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

Teeny tiny nit: four space indent.

Copy link
Member

Choose a reason for hiding this comment

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

@bnoordhuis Does the four-space indent rule apply even to JS?

Copy link
Member

Choose a reason for hiding this comment

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

It's used pretty consistently in both JS and C++ code. In fact, I thought the JS linter enforced it, even.

Copy link
Member

Choose a reason for hiding this comment

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

lib/buffer.js consistently uses 2-space indentation (grep for =$), but either way it's fine for me.

@TimothyGu TimothyGu self-assigned this Jul 31, 2017
@TimothyGu TimothyGu removed the stalled Issues and PRs that are stalled. label Jul 31, 2017
@TimothyGu TimothyGu force-pushed the jk-global-require branch from 268790f to a77c0fa Compare July 31, 2017 01:04
@TimothyGu
Copy link
Member

@TimothyGu
Copy link
Member

Rebased. I intend to land this soon if CI doesn't have any problems with that.

CI: https://ci.nodejs.org/job/node-test-commit/11573/

@TimothyGu
Copy link
Member

Landed in 5fd2f03.

@TimothyGu TimothyGu closed this Aug 7, 2017
TimothyGu pushed a commit that referenced this pull request Aug 7, 2017
Some parts were written by Timothy Gu <timothygu99@gmail.com>.

PR-URL: #8837
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Aleksei Koziatinskii <ak239spb@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
@jkrems jkrems deleted the jk-global-require branch August 7, 2017 14:11
addaleax pushed a commit that referenced this pull request Aug 10, 2017
Some parts were written by Timothy Gu <timothygu99@gmail.com>.

PR-URL: #8837
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Aleksei Koziatinskii <ak239spb@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
@addaleax addaleax added the notable-change PRs with changes that should be highlighted in changelogs. label Aug 13, 2017
@addaleax addaleax mentioned this pull request Aug 13, 2017
addaleax added a commit that referenced this pull request Aug 13, 2017
Notable changes

* **Inspector**
  * `require()` is available in the inspector console now.
    [#8837](#8837)

* **N-API**
  * New APIs for creating number values have been introduced.
    [#14573](#14573)

* **Stream**
  * For `Duplex` streams, the high water mark option can now be set
    independently for the readable and the writable side.
    [#14636](#14636)

* **Util**
  * `util.format` now supports the `%o` and `%O` specifiers for printing
    objects.
    [#14558](#14558)

PR-URL: #14811
addaleax added a commit that referenced this pull request Aug 14, 2017
Notable changes

* **HTTP2**
  * Experimental support for the built-in `http2` has been added via the
    `--expose-http2` flag.
    [#14239](#14239)

* **Inspector**
  * `require()` is available in the inspector console now.
    [#8837](#8837)
  * Multiple contexts, as created by the `vm` module, are supported now.
    [#14465](#14465)

* **N-API**
  * New APIs for creating number values have been introduced.
    [#14573](#14573)

* **Stream**
  * For `Duplex` streams, the high water mark option can now be set
    independently for the readable and the writable side.
    [#14636](#14636)

* **Util**
  * `util.format` now supports the `%o` and `%O` specifiers for printing
    objects.
    [#14558](#14558)

PR-URL: #14811
evanlucas pushed a commit that referenced this pull request Aug 15, 2017
Notable changes

* **HTTP2**
  * Experimental support for the built-in `http2` has been added via the
    `--expose-http2` flag.
    [#14239](#14239)

* **Inspector**
  * `require()` is available in the inspector console now.
    [#8837](#8837)
  * Multiple contexts, as created by the `vm` module, are supported now.
    [#14465](#14465)

* **N-API**
  * New APIs for creating number values have been introduced.
    [#14573](#14573)

* **Stream**
  * For `Duplex` streams, the high water mark option can now be set
    independently for the readable and the writable side.
    [#14636](#14636)

* **Util**
  * `util.format` now supports the `%o` and `%O` specifiers for printing
    objects.
    [#14558](#14558)

PR-URL: #14811
MSLaguana pushed a commit to nodejs/node-chakracore that referenced this pull request Aug 21, 2017
Notable changes

* **HTTP2**
  * Experimental support for the built-in `http2` has been added via the
    `--expose-http2` flag.
    [#14239](nodejs/node#14239)

* **Inspector**
  * `require()` is available in the inspector console now.
    [#8837](nodejs/node#8837)
  * Multiple contexts, as created by the `vm` module, are supported now.
    [#14465](nodejs/node#14465)

* **N-API**
  * New APIs for creating number values have been introduced.
    [#14573](nodejs/node#14573)

* **Stream**
  * For `Duplex` streams, the high water mark option can now be set
    independently for the readable and the writable side.
    [#14636](nodejs/node#14636)

* **Util**
  * `util.format` now supports the `%o` and `%O` specifiers for printing
    objects.
    [#14558](nodejs/node#14558)

PR-URL: nodejs/node#14811
@gibfahn
Copy link
Member

gibfahn commented Jan 15, 2018

Release team were -1 on landing on v6.x, if you disagree let us know.

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++. inspector Issues and PRs related to the V8 inspector protocol lib / src Issues and PRs related to general changes in the lib or src directory. notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.