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>

11 files changed, 231 insertions, 194 deletions

diff --git a/lib/internal/bootstrap/pre_execution.js b/lib/internal/bootstrap/pre_execution.js
index 936dc4673c1..79bb5cc2916 100644
--- a/lib/internal/bootstrap/pre_execution.js
+++ b/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) => {
diff --git a/lib/internal/process/per_thread.js b/lib/internal/process/per_thread.js
index 85772aafd8c..a0d6b325960 100644
--- a/lib/internal/process/per_thread.js
+++ b/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();
     }
 
diff --git a/lib/internal/coverage-gen/with_profiler.js b/lib/internal/profiler.js
index 573d2c3712b..1042f126dd6 100644
--- a/lib/internal/coverage-gen/with_profiler.js
+++ b/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) {
diff --git a/node.gyp b/node.gyp
index abae8592f42..13fa0d01e17 100644
--- a/node.gyp
+++ b/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',
diff --git a/src/env.h b/src/env.h
index 206f125efe3..62ce57b0bc3 100644
--- a/src/env.h
+++ b/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,
diff --git a/src/inspector/node_inspector.gypi b/src/inspector/node_inspector.gypi
index a59727f191e..5191543ab73 100644
--- a/src/inspector/node_inspector.gypi
+++ b/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',
diff --git a/src/inspector_coverage.cc b/src/inspector_coverage.cc
deleted file mode 100644
index 8cbec586fba..00000000000
--- a/src/inspector_coverage.cc
+++ /dev/null
@@ -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)
diff --git a/src/inspector_profiler.cc b/src/inspector_profiler.cc
new file mode 100644
index 00000000000..942a1d148a2
--- /dev/null
+++ b/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)
diff --git a/src/node.cc b/src/node.cc
index 891d4d0b3ab..fcc8923ff47 100644
--- a/src/node.cc
+++ b/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
diff --git a/src/node_binding.cc b/src/node_binding.cc
index 7b87ba6bffd..9599cf956b9 100644
--- a/src/node_binding.cc
+++ b/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>
diff --git a/src/node_internals.h b/src/node_internals.h
index 212da7af281..2e492727bbc 100644
--- a/src/node_internals.h
+++ b/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