debug: activate inspector with _debugProcess #11431

eugeneo · 2017-02-16T21:20:09Z

This pull request switches the signal handler to start inspector socket
server instead of the legacy V8 debug protocol.

Fixes: #8464

CC: @ofrobots

eugeneo · 2017-02-16T21:20:27Z

Ref: nodejs/diagnostics#67

eugeneo · 2017-02-17T00:43:08Z

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

ARM failures are parallel/test-dgram-address that does not seem to be influenced by this change.

ofrobots · 2017-02-17T01:33:30Z

/cc @nodejs/diagnostics @jkrems

addaleax

Left a few comments … the size of the diff makes it quite difficult to tell code that was just moved around from actual changes, by the way. I don’t know if splitting into several commits would have been a possibility here, but that would make reviewing a lot easier…

addaleax · 2017-02-21T16:15:21Z

src/inspector_agent.cc

+  size_t len = string_value->Length();
+  std::vector<uint16_t> buffer(len, '\0');
+  string_value->Write(buffer.data(), 0, len);
+  return StringBuffer::create(StringView(buffer.data(), len));


Could you use a TwoByteValue for the above couple of lines?

Done. Thanks!

addaleax · 2017-02-21T16:15:56Z

src/inspector_agent.cc

+
+std::unique_ptr<StringBuffer> ToProtocolString(Local<Value> value) {
+  if (value.IsEmpty() || value->IsNull() || value->IsUndefined() ||
+      !value->IsString()) {


Why not just value.IsEmpty() || !value->IsString()?

I switched to TwoByteValue and removed this check.

addaleax · 2017-02-21T16:22:44Z

src/inspector_agent.h

+class InspectorSessionDelegate {
+ public:
+  virtual bool WaitForFrontendMessage() = 0;
+  virtual void OnMessage(const v8_inspector::StringView message) = 0;


Is there any reason why this can’t be const v8_inspector::StringView&?

addaleax · 2017-02-21T16:23:02Z

src/inspector_agent.h

+
+  void Connect(InspectorSessionDelegate* delegate);
+  void Disconnect();
+  void Dispatch(const v8_inspector::StringView message);


(ditto for const v8_inspector::StringView&)

addaleax · 2017-02-21T16:26:31Z

src/inspector_io.cc

+using v8_inspector::StringView;
+
+template<typename Transport>
+using IoAndTransport = std::pair<Transport*, InspectorIo*>;


nit: I’m tempted to say this might better be called TransportAndIo. 😄

Right... :) Thanks, I fixed it!

eugeneo · 2017-02-21T23:31:18Z

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

I do not see ARM failures through the Hudson - I think GitHub integration might be glitchy.

jkrems · 2017-03-16T00:23:39Z

Tried this change against node-inspect and it worked as expected.

jasnell · 2017-03-22T17:56:29Z

What's the status on this?

eugeneo · 2017-03-22T18:10:06Z

@jasnell I am not sure. Code-wise it is ready for the review. A decision needs to be made when the old debugger stops handling the signal - is this something that needs to be discussed by @nodejs/diagnostics?

jkrems · 2017-03-22T18:24:10Z

@joshgav Can we pull this into tomorrow's meeting? I think it boils down to a unified "master plan" for doing the switch and when to land what and where.

joshgav · 2017-03-22T18:35:31Z

@jkrems sure, just added label.

eugeneo · 2017-03-22T19:01:09Z

@jkrems, @joshgav - how can I join the meeting? I tried looking through the https://github.com/nodejs/diagnostics but did not find details about the upcoming meeting.

jkrems · 2017-03-22T19:08:13Z

@eugeneo This is the meta issue for the meeting: nodejs/diagnostics#89

bnoordhuis

Mea culpa, looks like I reviewed this back in February but forgot to submit the comments. Re-reviewed; needs a rebase but the conflicts look minor.

bnoordhuis · 2017-03-29T15:10:27Z

src/inspector_agent.cc

-  bool waiting_;
-};
+static int GetDebugSignalHandlerMappingName(DWORD pid, wchar_t* buf,
+    size_t buf_len) {


Can you line up the arguments?

bnoordhuis · 2017-03-29T15:14:43Z

src/inspector_agent.cc

    inspector_->contextCreated(info);
  }

  void runMessageLoopOnPause(int context_group_id) override {
+    CHECK(channel_);


Can you write this as CHECK_NE(channel, nullptr)?

bnoordhuis · 2017-03-29T15:15:12Z

src/inspector_agent.cc

-  void connectFrontend() {
-    session_ = inspector_->connect(1, new ChannelImpl(agent_), StringView());
+  void connectFrontend(InspectorSessionDelegate* delegate) {
+    CHECK(!channel_);


Likewise but with CHECK_EQ. I'll stop pointing it out.

bnoordhuis · 2017-03-29T15:18:20Z

src/inspector_agent.cc

+  }
+
+  InspectorSessionDelegate* delegate() {
+    if (!channel_)


Prefer channel_ == nullptr over bool coercion.

bnoordhuis · 2017-03-29T15:33:31Z

src/inspector_agent.cc

-  int err = uv_thread_join(&thread_);
-  CHECK_EQ(err, 0);
-  delete inspector_;
+  if (io_)


Likewise. I'll stop pointing it out.

bnoordhuis · 2017-03-29T15:36:49Z

src/inspector_agent.cc

-std::string InspectorAgentDelegate::GetTargetTitle(const std::string& id) {
-  return script_name_.empty() ? GetProcessTitle() : script_name_;
+InspectorSessionDelegate* Agent::delegate() {
+  return impl->client()->delegate();


Can you explain why you sometimes CHECK(impl->client()) and sometimes not?

bnoordhuis · 2017-03-29T15:39:13Z

src/inspector_io.cc

+                           OneByteString(env->isolate(), str),                \
+                           var,                                               \
+                           v8::ReadOnly).FromJust();                          \
+  } while (0)


Unused macro?

bnoordhuis · 2017-03-29T15:40:11Z

src/inspector_io.cc

+  if (!script_name_.empty()) {
+    uv_fs_t req;
+    if (0 == uv_fs_realpath(&loop, &req, script_name_.c_str(), nullptr))
+      script_path = std::string(reinterpret_cast<char*>(req.ptr));


static_cast?

bnoordhuis · 2017-03-29T15:41:59Z

src/inspector_io.cc

+  if (!server.Start(&loop)) {
+    state_ = State::kError;  // Safe, main thread is waiting on semaphore
+    uv_close(reinterpret_cast<uv_handle_t*>(&io_thread_req_), nullptr);
+    uv_loop_close(&loop);


Can you CHECK_EQ(0, uv_loop_close(&loop))? I'm pretty sure this won't work on account of the closing handle.

I changed the code to spin until the callback is called.

bnoordhuis · 2017-03-29T15:52:37Z

src/node_debug_options.cc

@@ -141,7 +141,8 @@ int DebugOptions::port() const {
  int port = port_;
  if (port < 0) {
 #if HAVE_INSPECTOR
-    port = inspector_enabled_ ? default_inspector_port : default_debugger_port;
+    port = (debugger_enabled_ && !inspector_enabled_) ? default_debugger_port
+                                                      : default_inspector_port;


Can you make this an if statement for legibility? You could also simplify it to this:

if (port < 0) { port = default_debugger_port; #if HAVE_INSPECTOR if (inspector_enabled_) port = default_inspector_port; #endif // HAVE_INSPECTOR }

eugeneo · 2017-03-29T19:13:27Z

Thank you for the review. Please take another look.

eugeneo · 2017-03-31T18:26:33Z

I did a rebase to account for the latest changes. Please review.

jkrems · 2017-04-03T21:51:38Z

Looks like there's one stray lint error:

src/inspector_io.h:119:  Lines should be <= 80 characters long  [whitespace/line_length] [2]

eugeneo · 2017-04-03T23:00:50Z

@jkrems thanks for pointing that out, I fixed it.

eugeneo · 2017-04-03T23:23:04Z

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

bnoordhuis

LGTM sans some final comments. Have you checked if ./configure --without-inspector still builds?

bnoordhuis · 2017-04-04T08:45:29Z

src/inspector_agent.cc


 class AgentImpl {
 public:
  explicit AgentImpl(node::Environment* env);
+  ~AgentImpl() { }


Out of curiosity, what is the purpose of this destructor?

bnoordhuis · 2017-04-04T08:45:50Z

src/inspector_agent.cc

  void WaitForFrontendMessage();
  void NotifyMessageReceived();
+  bool StartIoThread();
+  static void InspectorWrapConsoleCall(
+    const v8::FunctionCallbackInfo<Value>& args);


Four space indent.

bnoordhuis · 2017-04-04T08:48:15Z

src/inspector_agent.cc

+    Local<Value> config_value = args->Get(context, 2).ToLocalChecked();
+    CHECK(config_value->IsObject());
+    Local<Object> config_object = config_value.As<Object>();
+    Local<String> in_call_key = OneByteString(isolate, "in_call");


You could use FIXED_ONE_BYTE_STRING here.

Done, did search/replace for all invocations

bnoordhuis · 2017-04-04T08:51:00Z

src/inspector_agent.cc

+  static_cast<void>(node_method.As<Function>()->Call(context,
+                                                     info.Holder(),
+                                                     call_args.size(),
+                                                     call_args.data()));
  if (try_catch.HasCaught())
    try_catch.ReThrow();


Is there a point to creating the TryCatch when it just rethrows the exception?

bnoordhuis · 2017-04-04T08:51:47Z

src/inspector_agent.cc

+  Local<Object> process = parent_env_->process_object();
+  Local<Object> inspector = Object::New(parent_env_->isolate());
+  process->DefineOwnProperty(parent_env_->context(),
+                             OneByteString(parent_env_->isolate(), "inspector"),


You could use FIXED_ONE_BYTE_STRING here.

bnoordhuis · 2017-04-04T08:57:00Z

src/inspector_io.cc

+}
+
+bool InspectorIo::IsStarted() {
+  return !!platform_;


platform_ != nullptr?

bnoordhuis · 2017-04-04T08:58:46Z

src/inspector_io.cc

+                            InspectorIo::MainThreadAsyncCb));
+  uv_unref(reinterpret_cast<uv_handle_t*>(&main_thread_req_));
+  CHECK_EQ(0, uv_sem_init(&start_sem_, 0));
+  memset(&io_thread_req_, 0, sizeof(io_thread_req_));


Not wrong but you could also io_thread_req_() in the initializer list.

bnoordhuis · 2017-04-04T09:01:00Z

src/inspector_io.cc

+      StringView message = std::get<2>(task)->string();
+      switch (std::get<0>(task)) {
+      case InspectorAction::kStartSession:
+        CHECK(!session_delegate_);


bnoordhuis · 2017-04-04T09:01:23Z

src/inspector_io.cc

+        parent_env_->inspector_agent()->Connect(session_delegate_.get());
+        break;
+      case InspectorAction::kEndSession:
+        CHECK(session_delegate_);


bnoordhuis · 2017-04-04T09:03:39Z

src/node.cc

@@ -270,7 +270,7 @@ static struct {
  bool StartInspector(Environment *env, const char* script_path,
                      const node::DebugOptions& options) {
    env->ThrowError("Node compiled with NODE_USE_V8_PLATFORM=0");
-    return false;  // make compiler happy
+    return true;  // make compiler happy


The change suggests it's not just about the compiler?

I removed outdated comment.

eugeneo · 2017-04-04T20:20:14Z

@bnoordhuis I moved the _debugProcess code back to node.cc so the node built with --without-inspector would still be able to send signal/do the weird Windows stuff to another Node instance. What do you think?

bnoordhuis · 2017-04-04T20:32:41Z

@eugeneo Did you forget to push? I still see RegisterDebugSignalHandler() and friends in inspector_agent.cc, not node.cc.

(Happy to hear I'm not the only one who thinks the Windows code is weird. I'm not even sure why it works.)

eugeneo · 2017-04-04T20:39:03Z

Signal handler remains there, it is only the code that sends the signal that was moved back. (I see CI failures, looking into them)

eugeneo · 2017-04-05T00:08:23Z

CI is passing: https://ci.nodejs.org/job/node-test-pull-request/7217/

eugeneo · 2017-04-05T21:16:54Z

CI: https://ci.nodejs.org/job/node-test-pull-request/7219/ - OS X is passing, even though its status is not properly reported.

eugeneo · 2017-04-06T16:13:08Z

Landed as 7599b0e

nodejs-github-bot added build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v4.x inspector Issues and PRs related to the V8 inspector protocol labels Feb 16, 2017

ofrobots added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 17, 2017

jkrems mentioned this pull request Feb 17, 2017

Switch the CLI debugger to V8 inspector #11421

Closed

14 tasks

ofrobots mentioned this pull request Feb 17, 2017

inspector: make debug an alias for inspect #11441

Merged

addaleax reviewed Feb 21, 2017

View reviewed changes

eugeneo mentioned this pull request Feb 28, 2017

Breakpoints can't be set by a debugger (Chrome or VS Code) while the Node program is running #11589

Closed

ofrobots added this to the 8.0.0 milestone Mar 9, 2017

ofrobots mentioned this pull request Mar 9, 2017

Move V8 inspector to stable #11770

Closed

jkrems mentioned this pull request Mar 16, 2017

Support for debugging a pid nodejs/node-inspect#37

Merged

ofrobots mentioned this pull request Mar 16, 2017

Find an alternative to V8's Debug Mirror API for util.inspect #11875

Closed

TimothyGu added dont-land-on-v6.x labels Mar 16, 2017

jkrems mentioned this pull request Mar 22, 2017

Can't put running Node process into debug mode microsoft/vscode-node-debug2#15

Closed

joshgav added the diag-agenda Issues and PRs to discuss during the meetings of the diagnostics working group. label Mar 22, 2017

joshgav mentioned this pull request Mar 23, 2017

Diag WG Meeting - 2017-03 nodejs/diagnostics#89

Closed

ofrobots mentioned this pull request Mar 24, 2017

doc: add meeting minutes 2017-03-23 nodejs/diagnostics#93

Closed

bnoordhuis requested changes Mar 29, 2017

View reviewed changes

MylesBorins mentioned this pull request Apr 3, 2017

Remove Legacy Debugger for 8.0.0 nodejs/CTC#94

Closed

jkrems mentioned this pull request Apr 3, 2017

src: Remove support for --debug #12197

Closed

4 tasks

bnoordhuis approved these changes Apr 4, 2017

View reviewed changes

jkrems mentioned this pull request Apr 4, 2017

[WIP] deps: Update node-inspect to v1.11.0 #12214

Closed

3 tasks

bnoordhuis approved these changes Apr 5, 2017

View reviewed changes

ofrobots mentioned this pull request Apr 5, 2017

doc: deprecate vm.runInDebugContext #12243

Closed

4 tasks

eugeneo closed this Apr 6, 2017

eugeneo deleted the inspector-signal branch April 6, 2017 16:12

eugeneo merged commit 7599b0e into nodejs:master Apr 6, 2017

jkrems mentioned this pull request Apr 12, 2017

deps: update node-inspect to v1.11.2 #12363

Closed

4 tasks

refack mentioned this pull request Apr 23, 2017

regression: 3rd party debuggers are incompatible with node8 nighlies #12364

Closed

addaleax removed the diag-agenda Issues and PRs to discuss during the meetings of the diagnostics working group. label May 10, 2017

jasnell mentioned this pull request May 11, 2017

8.0.0 Release Proposal #12220

Closed

refack mentioned this pull request Jun 11, 2017

refactor test/inspector #13622

Closed

debug: activate inspector with _debugProcess #11431

debug: activate inspector with _debugProcess #11431

Conversation

eugeneo commented Feb 16, 2017

eugeneo commented Feb 16, 2017

eugeneo commented Feb 17, 2017

ofrobots commented Feb 17, 2017

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugeneo commented Feb 21, 2017

jkrems commented Mar 16, 2017

jasnell commented Mar 22, 2017

eugeneo commented Mar 22, 2017

jkrems commented Mar 22, 2017 • edited Loading

joshgav commented Mar 22, 2017 • edited Loading

eugeneo commented Mar 22, 2017

jkrems commented Mar 22, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugeneo commented Mar 29, 2017

eugeneo commented Mar 31, 2017

jkrems commented Apr 3, 2017

eugeneo commented Apr 3, 2017

eugeneo commented Apr 3, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugeneo commented Apr 4, 2017

bnoordhuis commented Apr 4, 2017

eugeneo commented Apr 4, 2017

eugeneo commented Apr 5, 2017

jkrems commented Mar 22, 2017 •

edited

Loading

joshgav commented Mar 22, 2017 •

edited

Loading