inspector: added NodeRuntime domain

alexkozy · Aleksei Koziatinskii · commit 74a3c97eced3 · 2019-05-07T10:57:22.000-07:00
Historically Node process sends Runtime.executionContextDestroyed
with main context as argument when it is finished.
This approach has some disadvantages. V8 prevents running some
protocol command on destroyed contexts, e.g. Runtime.evaluate
will return an error or Debugger.enable won't return a list of
scripts.
Both command might be useful for different tools, e.g. tool runs
Profiler.startPreciseCoverage and at the end of node process it
would like to get list of all scripts to match data to source code.
Or some tooling frontend would like to provide capabilities to run
commands in console when node process is finished to allow user to
inspect state of the program at exit.
This PR adds new domain: NodeRuntime. When this domain is enabled
by at least one client, node will send
NodeRuntime.waitingForDebuggerToDisconnect event instead of
Runtime.executionContextDestroyed. Based on this signal any protocol
client can capture all required information and then disconnect its
session.
diff --git a/src/inspector/node_inspector.gypi b/src/inspector/node_inspector.gypi
@@ -9,6 +9,8 @@
       '<(SHARED_INTERMEDIATE_DIR)/src/node/inspector/protocol/NodeWorker.h',
       '<(SHARED_INTERMEDIATE_DIR)/src/node/inspector/protocol/NodeTracing.cpp',
       '<(SHARED_INTERMEDIATE_DIR)/src/node/inspector/protocol/NodeTracing.h',
+      '<(SHARED_INTERMEDIATE_DIR)/src/node/inspector/protocol/NodeRuntime.cpp',
+      '<(SHARED_INTERMEDIATE_DIR)/src/node/inspector/protocol/NodeRuntime.h',
     ],
     'node_protocol_files': [
       '<(protocol_tool_path)/lib/Allocator_h.template',
@@ -55,6 +57,8 @@
     '../../src/inspector/main_thread_interface.h',
     '../../src/inspector/node_string.cc',
     '../../src/inspector/node_string.h',
+    '../../src/inspector/runtime_agent.cc',
+    '../../src/inspector/runtime_agent.h',
     '../../src/inspector/tracing_agent.cc',
     '../../src/inspector/tracing_agent.h',
     '../../src/inspector/worker_agent.cc',
diff --git a/src/inspector/node_protocol.pdl b/src/inspector/node_protocol.pdl
@@ -92,3 +92,12 @@ experimental domain NodeWorker
       # Identifier of a session which sends a message.
       SessionID sessionId
       string message
+
+experimental domain NodeRuntime
+  command enable
+  command disable
+
+  # Event is issued when Node process is finished and waiting for debugger to disconnect.
+  # When domain is enabled client should listen for this event instead of `Runtime.executionContextDestroyed`
+  # to check wether node process is finished.
+  event waitingForDebuggerToDisconnect
diff --git a/src/inspector/runtime_agent.cc b/src/inspector/runtime_agent.cc
@@ -0,0 +1,43 @@
+#include "runtime_agent.h"
+
+#include "env-inl.h"
+#include "inspector_agent.h"
+
+namespace node {
+namespace inspector {
+namespace protocol {
+
+RuntimeAgent::RuntimeAgent(Environment* env)
+  : enabled_(false)
+  , env_(env) {}
+
+void RuntimeAgent::Wire(UberDispatcher* dispatcher) {
+  frontend_.reset(new NodeRuntime::Frontend(dispatcher->channel()));
+  NodeRuntime::Dispatcher::wire(dispatcher, this);
+}
+
+DispatchResponse RuntimeAgent::enable() {
+  if (!env_->owns_process_state()) {
+    return DispatchResponse::Error(
+        "NodeRuntime domain can only be used through main thread sessions");
+  }
+  enabled_ = true;
+  return DispatchResponse::OK();
+}
+
+DispatchResponse RuntimeAgent::disable() {
+  if (!env_->owns_process_state()) {
+    return DispatchResponse::Error(
+        "NodeRuntime domain can only be used through main thread sessions");
+  }
+  enabled_ = false;
+  return DispatchResponse::OK();
+}
+
+bool RuntimeAgent::reportWaitingForDebuggerToDisconnect() {
+  if (enabled_) frontend_->waitingForDebuggerToDisconnect();
+  return enabled_;
+}
+}  // namespace protocol
+}  // namespace inspector
+}  // namespace node
diff --git a/src/inspector/runtime_agent.h b/src/inspector/runtime_agent.h
@@ -0,0 +1,33 @@
+#ifndef SRC_INSPECTOR_RUNTIME_AGENT_H_
+#define SRC_INSPECTOR_RUNTIME_AGENT_H_
+
+#include "node/inspector/protocol/NodeRuntime.h"
+#include "v8.h"
+
+namespace node {
+class Environment;
+
+namespace inspector {
+namespace protocol {
+
+class RuntimeAgent : public NodeRuntime::Backend {
+ public:
+  explicit RuntimeAgent(Environment* env);
+
+  void Wire(UberDispatcher* dispatcher);
+
+  DispatchResponse enable() override;
+  DispatchResponse disable() override;
+
+  bool reportWaitingForDebuggerToDisconnect();
+
+ private:
+  std::shared_ptr<NodeRuntime::Frontend> frontend_;
+  bool enabled_;
+  Environment* env_;
+};
+}  // namespace protocol
+}  // namespace inspector
+}  // namespace node
+
+#endif  // SRC_INSPECTOR_RUNTIME_AGENT_H_
diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc
@@ -2,6 +2,7 @@
 
 #include "inspector/main_thread_interface.h"
 #include "inspector/node_string.h"
+#include "inspector/runtime_agent.h"
 #include "inspector/tracing_agent.h"
 #include "inspector/worker_agent.h"
 #include "inspector/worker_inspector.h"
@@ -228,13 +229,17 @@ class ChannelImpl final : public v8_inspector::V8Inspector::Channel,
     tracing_agent_->Wire(node_dispatcher_.get());
     worker_agent_ = std::make_unique<protocol::WorkerAgent>(worker_manager);
     worker_agent_->Wire(node_dispatcher_.get());
+    runtime_agent_.reset(new protocol::RuntimeAgent(env));
+    runtime_agent_->Wire(node_dispatcher_.get());
   }
 
   ~ChannelImpl() override {
     tracing_agent_->disable();
     tracing_agent_.reset();  // Dispose before the dispatchers
     worker_agent_->disable();
     worker_agent_.reset();  // Dispose before the dispatchers
+    runtime_agent_->disable();
+    runtime_agent_.reset();  // Dispose before the dispatchers
   }
 
   std::string dispatchProtocolMessage(const StringView& message) {
@@ -264,6 +269,10 @@ class ChannelImpl final : public v8_inspector::V8Inspector::Channel,
     return prevent_shutdown_;
   }
 
+  bool reportWaitingForDebuggerToDisconnect() {
+    return runtime_agent_->reportWaitingForDebuggerToDisconnect();
+  }
+
  private:
   void sendResponse(
       int callId,
@@ -303,6 +312,7 @@ class ChannelImpl final : public v8_inspector::V8Inspector::Channel,
     DCHECK(false);
   }
 
+  std::unique_ptr<protocol::RuntimeAgent> runtime_agent_;
   std::unique_ptr<protocol::TracingAgent> tracing_agent_;
   std::unique_ptr<protocol::WorkerAgent> worker_agent_;
   std::unique_ptr<InspectorSessionDelegate> delegate_;
@@ -610,6 +620,14 @@ class NodeInspectorClient : public V8InspectorClient {
     return false;
   }
 
+  bool reportWaitingForDebuggerToDisconnect() {
+    for (const auto& id_channel : channels_) {
+      if (id_channel.second->reportWaitingForDebuggerToDisconnect())
+        return true;
+    }
+    return false;
+  }
+
   std::shared_ptr<MainThreadHandle> getThreadHandle() {
     if (interface_ == nullptr) {
       interface_.reset(new MainThreadInterface(
@@ -779,7 +797,8 @@ void Agent::WaitForDisconnect() {
   }
   // TODO(addaleax): Maybe this should use an at-exit hook for the Environment
   // or something similar?
-  client_->contextDestroyed(parent_env_->context());
+  if (!client_->reportWaitingForDebuggerToDisconnect())
+    client_->contextDestroyed(parent_env_->context());
   if (io_ != nullptr) {
     io_->StopAcceptingNewConnections();
     client_->waitForIoShutdown();
diff --git a/test/common/inspector-helper.js b/test/common/inspector-helper.js
@@ -191,6 +191,10 @@ class InspectorSession {
     }
   }
 
+  unprocessedNotifications() {
+    return this._unprocessedNotifications;
+  }
+
   _sendMessage(message) {
     const msg = JSON.parse(JSON.stringify(message)); // Clone!
     msg.id = this._nextId++;
diff --git a/test/parallel/test-inspector-waiting-for-debugger-to-disconnect.js b/test/parallel/test-inspector-waiting-for-debugger-to-disconnect.js
@@ -0,0 +1,31 @@
+// Flags: --expose-internals
+'use strict';
+const common = require('../common');
+
+common.skipIfInspectorDisabled();
+
+const assert = require('assert');
+const { NodeInstance } = require('../common/inspector-helper.js');
+
+async function runTest() {
+  const child = new NodeInstance(['--inspect-brk=0', '-e', 'process.exit(55)']);
+  const session = await child.connectInspectorSession();
+  await session.send([
+    { method: 'Runtime.enable' },
+    { method: 'NodeRuntime.enable' },
+    { method: 'Runtime.runIfWaitingForDebugger' }]);
+  await session.waitForNotification((notification) => {
+    return notification.method === 'NodeRuntime.waitingForDebuggerToDisconnect';
+  });
+  const receivedExecutionContextDestroyed = session.unprocessedNotifications().some((notification) => {
+    return notification.method === 'Runtime.executionContextDestroyed' &&
+      notification.params.executionContextId === 1;
+  });
+  if (receivedExecutionContextDestroyed) {
+    assert.fail(`When NodeRuntime enabled, Runtime.executionContextDestroyed should not be sent`);
+  }
+  await session.disconnect();
+  assert.strictEqual((await child.expectShutdown()).exitCode, 55);
+}
+
+runTest();

Original file line number	Diff line number	Diff line change
`@@ -191,6 +191,10 @@ class InspectorSession {`
`191`	`191`	`}`
`192`	`192`	`}`
`193`	`193`
	`194`	`+ unprocessedNotifications() {`
	`195`	`+ return this._unprocessedNotifications;`
	`196`	`+ }`
	`197`	`+`
`194`	`198`	`_sendMessage(message) {`
`195`	`199`	`const msg = JSON.parse(JSON.stringify(message)); // Clone!`
`196`	`200`	`msg.id = this._nextId++;`