src: refactor coverage connection

- Refactor the C++ class to be resuable for other types of profiles
- Move the try-catch block around coverage collection callback
  to be inside the callback to silence potential JSON or write
  errors.
- Use Function::Call instead of MakeCallback to call the coverage
  message callback since it does not actually need async hook
  handling. This way we no longer needs to disable the async
  hooks when writing the coverage results.
- Renames `lib/internal/coverage-gen/with_profiler.js` to
  `lib/internal/profiler.js` because it is now the only way
  to generate coverage.

PR-URL: https://github.com/nodejs/node/pull/26513
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Coe <bencoe@gmail.com>

lib/internal/bootstrap/pre_execution.js

@@ -67,7 +67,7 @@ function setupCoverageHooks(dir) {
   const {
     writeCoverage,
     setCoverageDirectory
-  } = require('internal/coverage-gen/with_profiler');
+  } = require('internal/profiler');
   setCoverageDirectory(coverageDirectory);
   process.on('exit', writeCoverage);
   process.reallyExit = (code) => {

lib/internal/process/per_thread.js

@@ -158,7 +158,7 @@ function wrapProcessMethods(binding) {
   function kill(pid, sig) {
     var err;
     if (process.env.NODE_V8_COVERAGE) {
-      const { writeCoverage } = require('internal/coverage-gen/with_profiler');
+      const { writeCoverage } = require('internal/profiler');
       writeCoverage();
     }
 

lib/internal/profiler.js

@@ -21,23 +21,16 @@ function writeCoverage() {
   }
 
   const target = join(coverageDirectory, filename);
-  try {
-    disableAllAsyncHooks();
-    internalBinding('coverage').end((msg) => {
+  internalBinding('profiler').endCoverage((msg) => {
+    try {
       const coverageInfo = JSON.parse(msg).result;
       if (coverageInfo) {
         writeFileSync(target, JSON.stringify(coverageInfo));
       }
-    });
-  } catch (err) {
-    console.error(err);
-  }
-}
-
-function disableAllAsyncHooks() {
-  const { getHookArrays } = require('internal/async_hooks');
-  const [hooks_array] = getHookArrays();
-  hooks_array.forEach((hook) => { hook.disable(); });
+    } catch (err) {
+      console.error(err);
+    }
+  });
 }
 
 function setCoverageDirectory(dir) {

node.gyp

@@ -99,7 +99,6 @@
       'lib/internal/cluster/worker.js',
       'lib/internal/console/constructor.js',
       'lib/internal/console/global.js',
-      'lib/internal/coverage-gen/with_profiler.js',
       'lib/internal/crypto/certificate.js',
       'lib/internal/crypto/cipher.js',
       'lib/internal/crypto/diffiehellman.js',
@@ -168,6 +167,7 @@
       'lib/internal/process/worker_thread_only.js',
       'lib/internal/process/report.js',
       'lib/internal/process/task_queues.js',
+      'lib/internal/profiler.js',
       'lib/internal/querystring.js',
       'lib/internal/readline.js',
       'lib/internal/repl.js',

src/env.h

@@ -468,7 +468,7 @@ struct CompileFnEntry {
 #define DEBUG_CATEGORY_NAMES(V)                                                \
   NODE_ASYNC_PROVIDER_TYPES(V)                                                 \
   V(INSPECTOR_SERVER)                                                          \
-  V(COVERAGE)
+  V(INSPECTOR_PROFILER)
 
 enum class DebugCategory {
 #define V(name) name,

src/inspector/node_inspector.gypi

@@ -45,7 +45,7 @@
     '../../src/inspector_io.cc',
     '../../src/inspector_agent.h',
     '../../src/inspector_io.h',
-    '../../src/inspector_coverage.cc',
+    '../../src/inspector_profiler.cc',
     '../../src/inspector_js_api.cc',
     '../../src/inspector_socket.cc',
     '../../src/inspector_socket.h',

src/inspector_coverage.cc

@@ -1,168 +0,0 @@
-#include "base_object-inl.h"
-#include "debug_utils.h"
-#include "inspector_agent.h"
-#include "node_internals.h"
-#include "v8-inspector.h"
-
-namespace node {
-namespace coverage {
-
-using v8::Context;
-using v8::Function;
-using v8::FunctionCallbackInfo;
-using v8::HandleScope;
-using v8::Isolate;
-using v8::Local;
-using v8::MaybeLocal;
-using v8::NewStringType;
-using v8::Object;
-using v8::ObjectTemplate;
-using v8::String;
-using v8::Value;
-
-using v8_inspector::StringBuffer;
-using v8_inspector::StringView;
-
-std::unique_ptr<StringBuffer> ToProtocolString(Isolate* isolate,
-                                               Local<Value> value) {
-  TwoByteValue buffer(isolate, value);
-  return StringBuffer::create(StringView(*buffer, buffer.length()));
-}
-
-class V8CoverageConnection : public BaseObject {
- public:
-  class V8CoverageSessionDelegate : public inspector::InspectorSessionDelegate {
-   public:
-    explicit V8CoverageSessionDelegate(V8CoverageConnection* connection)
-        : connection_(connection) {}
-
-    void SendMessageToFrontend(
-        const v8_inspector::StringView& message) override {
-      Environment* env = connection_->env();
-      Local<Function> fn = connection_->env()->on_coverage_message_function();
-      bool ending = !fn.IsEmpty();
-      Debug(env,
-            DebugCategory::COVERAGE,
-            "Sending message to frontend, ending = %s\n",
-            ending ? "true" : "false");
-      if (!ending) {
-        return;
-      }
-      Isolate* isolate = env->isolate();
-
-      HandleScope handle_scope(isolate);
-      Context::Scope context_scope(env->context());
-      MaybeLocal<String> v8string =
-          String::NewFromTwoByte(isolate,
-                                 message.characters16(),
-                                 NewStringType::kNormal,
-                                 message.length());
-      Local<Value> args[] = {v8string.ToLocalChecked().As<Value>()};
-      USE(MakeCallback(isolate,
-                       connection_->object(),
-                       fn,
-                       arraysize(args),
-                       args,
-                       async_context{0, 0}));
-    }
-
-   private:
-    V8CoverageConnection* connection_;
-  };
-
-  SET_MEMORY_INFO_NAME(V8CoverageConnection)
-  SET_SELF_SIZE(V8CoverageConnection)
-
-  void MemoryInfo(MemoryTracker* tracker) const override {
-    tracker->TrackFieldWithSize(
-        "session", sizeof(*session_), "InspectorSession");
-  }
-
-  explicit V8CoverageConnection(Environment* env)
-      : BaseObject(env, env->coverage_connection()), session_(nullptr) {
-    inspector::Agent* inspector = env->inspector_agent();
-    std::unique_ptr<inspector::InspectorSession> session = inspector->Connect(
-        std::make_unique<V8CoverageSessionDelegate>(this), false);
-    session_ = std::move(session);
-    MakeWeak();
-  }
-
-  void Start() {
-    Debug(this->env(),
-          DebugCategory::COVERAGE,
-          "Sending Profiler.startPreciseCoverage\n");
-    Isolate* isolate = this->env()->isolate();
-    Local<Value> enable = FIXED_ONE_BYTE_STRING(
-        isolate, "{\"id\": 1, \"method\": \"Profiler.enable\"}");
-    Local<Value> start = FIXED_ONE_BYTE_STRING(
-        isolate,
-        "{"
-        "\"id\": 2,"
-        "\"method\": \"Profiler.startPreciseCoverage\","
-        "\"params\": {\"callCount\": true, \"detailed\": true}"
-        "}");
-    session_->Dispatch(ToProtocolString(isolate, enable)->string());
-    session_->Dispatch(ToProtocolString(isolate, start)->string());
-  }
-
-  void End() {
-    Debug(this->env(),
-          DebugCategory::COVERAGE,
-          "Sending Profiler.takePreciseCoverage\n");
-    Isolate* isolate = this->env()->isolate();
-    Local<Value> end =
-        FIXED_ONE_BYTE_STRING(isolate,
-                              "{"
-                              "\"id\": 3,"
-                              "\"method\": \"Profiler.takePreciseCoverage\""
-                              "}");
-    session_->Dispatch(ToProtocolString(isolate, end)->string());
-  }
-
-  friend class V8CoverageSessionDelegate;
-
- private:
-  std::unique_ptr<inspector::InspectorSession> session_;
-};
-
-bool StartCoverageCollection(Environment* env) {
-  HandleScope scope(env->isolate());
-
-  Local<ObjectTemplate> t = ObjectTemplate::New(env->isolate());
-  t->SetInternalFieldCount(1);
-  Local<Object> obj;
-  if (!t->NewInstance(env->context()).ToLocal(&obj)) {
-    return false;
-  }
-
-  obj->SetAlignedPointerInInternalField(0, nullptr);
-
-  CHECK(env->coverage_connection().IsEmpty());
-  env->set_coverage_connection(obj);
-  V8CoverageConnection* connection = new V8CoverageConnection(env);
-  connection->Start();
-  return true;
-}
-
-static void EndCoverageCollection(const FunctionCallbackInfo<Value>& args) {
-  Environment* env = Environment::GetCurrent(args);
-  CHECK(args[0]->IsFunction());
-  Debug(env, DebugCategory::COVERAGE, "Ending coverage collection\n");
-  env->set_on_coverage_message_function(args[0].As<Function>());
-  V8CoverageConnection* connection =
-      Unwrap<V8CoverageConnection>(env->coverage_connection());
-  CHECK_NOT_NULL(connection);
-  connection->End();
-}
-
-static void Initialize(Local<Object> target,
-                       Local<Value> unused,
-                       Local<Context> context,
-                       void* priv) {
-  Environment* env = Environment::GetCurrent(context);
-  env->SetMethod(target, "end", EndCoverageCollection);
-}
-}  // namespace coverage
-}  // namespace node
-
-NODE_MODULE_CONTEXT_AWARE_INTERNAL(coverage, node::coverage::Initialize)

src/inspector_profiler.cc

@@ -0,0 +1,214 @@
+#include "base_object-inl.h"
+#include "debug_utils.h"
+#include "inspector_agent.h"
+#include "node_internals.h"
+#include "v8-inspector.h"
+
+namespace node {
+namespace profiler {
+
+using v8::Context;
+using v8::EscapableHandleScope;
+using v8::Function;
+using v8::FunctionCallbackInfo;
+using v8::HandleScope;
+using v8::Isolate;
+using v8::Local;
+using v8::MaybeLocal;
+using v8::NewStringType;
+using v8::Object;
+using v8::ObjectTemplate;
+using v8::String;
+using v8::Value;
+
+using v8_inspector::StringBuffer;
+using v8_inspector::StringView;
+
+std::unique_ptr<StringBuffer> ToProtocolString(Isolate* isolate,
+                                               Local<Value> value) {
+  TwoByteValue buffer(isolate, value);
+  return StringBuffer::create(StringView(*buffer, buffer.length()));
+}
+
+class V8ProfilerConnection : public BaseObject {
+ public:
+  class V8ProfilerSessionDelegate
+      : public inspector::InspectorSessionDelegate {
+   public:
+    explicit V8ProfilerSessionDelegate(V8ProfilerConnection* connection)
+        : connection_(connection) {}
+
+    void SendMessageToFrontend(
+        const v8_inspector::StringView& message) override {
+      Environment* env = connection_->env();
+
+      Local<Function> fn = connection_->GetMessageCallback();
+      bool ending = !fn.IsEmpty();
+      Debug(env,
+            DebugCategory::INSPECTOR_PROFILER,
+            "Sending message to frontend, ending = %s\n",
+            ending ? "true" : "false");
+      if (!ending) {
+        return;
+      }
+      Isolate* isolate = env->isolate();
+
+      HandleScope handle_scope(isolate);
+      Context::Scope context_scope(env->context());
+      MaybeLocal<String> v8string =
+          String::NewFromTwoByte(isolate,
+                                 message.characters16(),
+                                 NewStringType::kNormal,
+                                 message.length());
+      Local<Value> args[] = {v8string.ToLocalChecked().As<Value>()};
+      USE(fn->Call(
+          env->context(), connection_->object(), arraysize(args), args));
+    }
+
+   private:
+    V8ProfilerConnection* connection_;
+  };
+
+  SET_MEMORY_INFO_NAME(V8ProfilerConnection)
+  SET_SELF_SIZE(V8ProfilerConnection)
+
+  void MemoryInfo(MemoryTracker* tracker) const override {
+    tracker->TrackFieldWithSize(
+        "session", sizeof(*session_), "InspectorSession");
+  }
+
+  explicit V8ProfilerConnection(Environment* env, Local<Object> obj)
+      : BaseObject(env, obj), session_(nullptr) {
+    inspector::Agent* inspector = env->inspector_agent();
+    std::unique_ptr<inspector::InspectorSession> session = inspector->Connect(
+        std::make_unique<V8ProfilerSessionDelegate>(this), false);
+    session_ = std::move(session);
+    MakeWeak();
+  }
+
+  void DispatchMessage(Isolate* isolate, Local<String> message) {
+    session_->Dispatch(ToProtocolString(isolate, message)->string());
+  }
+
+  static MaybeLocal<Object> CreateConnectionObject(Environment* env) {
+    Isolate* isolate = env->isolate();
+    Local<Context> context = env->context();
+    EscapableHandleScope scope(isolate);
+
+    Local<ObjectTemplate> t = ObjectTemplate::New(isolate);
+    t->SetInternalFieldCount(1);
+    Local<Object> obj;
+    if (!t->NewInstance(context).ToLocal(&obj)) {
+      return MaybeLocal<Object>();
+    }
+
+    obj->SetAlignedPointerInInternalField(0, nullptr);
+    return scope.Escape(obj);
+  }
+
+  void Start() {
+    SetConnection(object());
+    StartProfiling();
+  }
+
+  void End(Local<Function> callback) {
+    SetMessageCallback(callback);
+    EndProfiling();
+  }
+
+  // Override this to return a JS function that gets called with the message
+  // sent from the session.
+  virtual Local<Function> GetMessageCallback() = 0;
+  virtual void SetMessageCallback(Local<Function> callback) = 0;
+  // Use DispatchMessage() to dispatch necessary inspector messages
+  virtual void StartProfiling() = 0;
+  virtual void EndProfiling() = 0;
+  virtual void SetConnection(Local<Object> connection) = 0;
+
+ private:
+  std::unique_ptr<inspector::InspectorSession> session_;
+};
+
+class V8CoverageConnection : public V8ProfilerConnection {
+ public:
+  explicit V8CoverageConnection(Environment* env)
+      : V8ProfilerConnection(env,
+                             CreateConnectionObject(env).ToLocalChecked()) {}
+
+  Local<Function> GetMessageCallback() override {
+    return env()->on_coverage_message_function();
+  }
+
+  void SetMessageCallback(Local<Function> callback) override {
+    return env()->set_on_coverage_message_function(callback);
+  }
+
+  static V8ProfilerConnection* GetConnection(Environment* env) {
+    return Unwrap<V8CoverageConnection>(env->coverage_connection());
+  }
+
+  void SetConnection(Local<Object> obj) override {
+    env()->set_coverage_connection(obj);
+  }
+
+  void StartProfiling() override {
+    Debug(env(),
+          DebugCategory::INSPECTOR_PROFILER,
+          "Sending Profiler.startPreciseCoverage\n");
+    Isolate* isolate = env()->isolate();
+    Local<String> enable = FIXED_ONE_BYTE_STRING(
+        isolate, "{\"id\": 1, \"method\": \"Profiler.enable\"}");
+    Local<String> start = FIXED_ONE_BYTE_STRING(
+        isolate,
+        "{"
+        "\"id\": 2,"
+        "\"method\": \"Profiler.startPreciseCoverage\","
+        "\"params\": {\"callCount\": true, \"detailed\": true}"
+        "}");
+    DispatchMessage(isolate, enable);
+    DispatchMessage(isolate, start);
+  }
+
+  void EndProfiling() override {
+    Debug(env(),
+          DebugCategory::INSPECTOR_PROFILER,
+          "Sending Profiler.takePreciseCoverage\n");
+    Isolate* isolate = env()->isolate();
+    Local<String> end =
+        FIXED_ONE_BYTE_STRING(isolate,
+                              "{"
+                              "\"id\": 3,"
+                              "\"method\": \"Profiler.takePreciseCoverage\""
+                              "}");
+    DispatchMessage(isolate, end);
+  }
+
+ private:
+  std::unique_ptr<inspector::InspectorSession> session_;
+};
+
+void StartCoverageCollection(Environment* env) {
+  V8CoverageConnection* connection = new V8CoverageConnection(env);
+  connection->Start();
+}
+
+static void EndCoverageCollection(const FunctionCallbackInfo<Value>& args) {
+  Environment* env = Environment::GetCurrent(args);
+  CHECK(args[0]->IsFunction());
+  Debug(env, DebugCategory::INSPECTOR_PROFILER, "Ending coverage collection\n");
+  V8ProfilerConnection* connection = V8CoverageConnection::GetConnection(env);
+  CHECK_NOT_NULL(connection);
+  connection->End(args[0].As<Function>());
+}
+
+static void Initialize(Local<Object> target,
+                       Local<Value> unused,
+                       Local<Context> context,
+                       void* priv) {
+  Environment* env = Environment::GetCurrent(context);
+  env->SetMethod(target, "endCoverage", EndCoverageCollection);
+}
+}  // namespace profiler
+}  // namespace node
+
+NODE_MODULE_CONTEXT_AWARE_INTERNAL(profiler, node::profiler::Initialize)

src/node.cc

@@ -235,9 +235,7 @@ MaybeLocal<Value> RunBootstrapping(Environment* env) {
   bool rc = credentials::SafeGetenv("NODE_V8_COVERAGE", &coverage);
   if (rc && !coverage.empty()) {
 #if HAVE_INSPECTOR
-    if (!coverage::StartCoverageCollection(env)) {
-      return MaybeLocal<Value>();
-    }
+    profiler::StartCoverageCollection(env);
 #else
     fprintf(stderr, "NODE_V8_COVERAGE cannot be used without inspector");
 #endif  // HAVE_INSPECTOR

src/node_binding.cc

@@ -23,9 +23,9 @@
 #endif
 
 #if HAVE_INSPECTOR
-#define NODE_BUILTIN_COVERAGE_MODULES(V) V(coverage)
+#define NODE_BUILTIN_PROFILER_MODULES(V) V(profiler)
 #else
-#define NODE_BUILTIN_COVERAGE_MODULES(V)
+#define NODE_BUILTIN_PROFILER_MODULES(V)
 #endif
 
 // A list of built-in modules. In order to do module registration
@@ -85,7 +85,7 @@
   NODE_BUILTIN_OPENSSL_MODULES(V)                                              \
   NODE_BUILTIN_ICU_MODULES(V)                                                  \
   NODE_BUILTIN_REPORT_MODULES(V)                                               \
-  NODE_BUILTIN_COVERAGE_MODULES(V)
+  NODE_BUILTIN_PROFILER_MODULES(V)
 
 // This is used to load built-in modules. Instead of using
 // __attribute__((constructor)), we call the _register_<modname>

src/node_internals.h

@@ -294,8 +294,8 @@ void DefineZlibConstants(v8::Local<v8::Object> target);
 v8::MaybeLocal<v8::Value> RunBootstrapping(Environment* env);
 v8::MaybeLocal<v8::Value> StartExecution(Environment* env,
                                          const char* main_script_id);
-namespace coverage {
-bool StartCoverageCollection(Environment* env);
+namespace profiler {
+void StartCoverageCollection(Environment* env);
 }
 }  // namespace node