inspector: use inspector API for "break on start" #12076
Conversation
nodejs-github-bot
added
c++
|
cc @ofrobots |
lib/internal/bootstrap_node.js
Outdated
| @@ -258,7 +258,9 @@ | |||
| if (process.inspector) { | |||
| inspectorConsole = global.console; | |||
| wrapConsoleCall = process.inspector.wrapConsoleCall; | |||
| delete process.inspector; | |||
| delete process.inspector.wrapConsoleCall; | |||
| if (Object.keys(process.inspector).length == 0) | |||
lib/module.js
Outdated
| } | ||
| var wrapper = inspector.callAndPauseOnStart.bind(inspector); | ||
| delete inspector.callAndPauseOnStart; | ||
| if (Object.keys(process.inspector).length == 0) { |
src/inspector_agent.cc
Outdated
| void AgentImpl::CallAndPauseOnStart( | ||
| const v8::FunctionCallbackInfo<v8::Value>& args) { | ||
| Environment* env = Environment::GetCurrent(args); | ||
| v8::HandleScope scope(env->isolate()); |
There was a problem hiding this comment.
The HandleScope isn't necessary, API functions get one implicitly.
src/inspector_agent.cc
Outdated
| return env->ThrowError("First argument for a " | ||
| "inspector.callAndPauseOnStart call " | ||
| "should be a function"); | ||
| } |
There was a problem hiding this comment.
Just CHECK(args.Length() > 1) and CHECK(args[0]->IsFunction())? This is only called by built-in code, right?
There was a problem hiding this comment.
For now, it is not supposed to be a public API. This might change in the future.
src/inspector_agent.cc
Outdated
| call_args.push_back(args[i]); | ||
| } | ||
|
|
||
| env->inspector_agent()->SchedulePauseOnNextStatement("Break on start"); |
There was a problem hiding this comment.
This effectively breaks on the next call into JS land? If this function were to return instead of calling the function, it would break at the next line in lib/module.js?
There was a problem hiding this comment.
It breaks on the next JS instruction. E.g. I just noticed that it is not breaking inside the wrapper - it seems to break on the wrapper call (e.g. one step is required to reach the same point it was breaking before). It does not make any difference for the tests as the line number and variables are the same.
|
Thank you for the review. I addressed the comments, please take another look. |
src/inspector_agent.cc
Outdated
| @@ -310,6 +313,14 @@ class V8NodeInspector : public v8_inspector::V8InspectorClient { | |||
| session_->dispatchProtocolMessage(message); | |||
| } | |||
|
|
|||
| void schedulePauseOnNextStatement(const std::string& reason) { | |||
| if (session_) { | |||
There was a problem hiding this comment.
Can you check that session_ != nullptr?
src/inspector_agent.cc
Outdated
|
|
||
| env->inspector_agent()->SchedulePauseOnNextStatement("Break on start"); | ||
|
|
||
| v8::Local<v8::Context> context = args.GetIsolate()->GetCurrentContext(); |
There was a problem hiding this comment.
env->context() might be simpler
|
CI has some challenges but they appear unrelated. |
|
Landed as 7954d2a |
|
cc @eugeneo |
|
@italoacasas - sorry, I forgot to add labels. This code does not need to be ported to pre-8x as those versions still rely on debug context. Thanks! |
This change removes a need for using deprecated debug context for
breaking at the start of the main module.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
inspector