inspector: reimplement JS binding

- Group all relevant methods/states into a C++ class.
- Uses internal fields instead of the less efficient v8::External for
  storing the pointer to the C++ object.
- Use AsyncWrap to allow instrumenting callback states.

Backport-PR-URL: #16071
PR-URL: #15643
Refs: #13503
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

Author: TimothyGu

lib/inspector.js

@@ -2,9 +2,9 @@
 
 const EventEmitter = require('events');
 const util = require('util');
-const { connect, open, url } = process.binding('inspector');
+const { Connection, open, url } = process.binding('inspector');
 
-if (!connect)
+if (!Connection)
   throw new Error('Inspector is not available');
 
 const connectionSymbol = Symbol('connectionProperty');
@@ -24,7 +24,7 @@ class Session extends EventEmitter {
     if (this[connectionSymbol])
       throw new Error('Already connected');
     this[connectionSymbol] =
-        connect((message) => this[onMessageSymbol](message));
+        new Connection((message) => this[onMessageSymbol](message));
   }
 
   [onMessageSymbol](message) {

src/async-wrap.h

@@ -72,9 +72,17 @@ namespace node {
 #define NODE_ASYNC_CRYPTO_PROVIDER_TYPES(V)
 #endif  // HAVE_OPENSSL
 
+#if HAVE_INSPECTOR
+#define NODE_ASYNC_INSPECTOR_PROVIDER_TYPES(V)                                \
+  V(INSPECTORJSBINDING)
+#else
+#define NODE_ASYNC_INSPECTOR_PROVIDER_TYPES(V)
+#endif  // HAVE_INSPECTOR
+
 #define NODE_ASYNC_PROVIDER_TYPES(V)                                          \
   NODE_ASYNC_NON_CRYPTO_PROVIDER_TYPES(V)                                     \
-  NODE_ASYNC_CRYPTO_PROVIDER_TYPES(V)
+  NODE_ASYNC_CRYPTO_PROVIDER_TYPES(V)                                         \
+  NODE_ASYNC_INSPECTOR_PROVIDER_TYPES(V)
 
 class Environment;
 

src/env.h

@@ -89,7 +89,6 @@ class ModuleWrap;
   V(arrow_message_private_symbol, "node:arrowMessage")                        \
   V(contextify_context_private_symbol, "node:contextify:context")             \
   V(contextify_global_private_symbol, "node:contextify:global")               \
-  V(inspector_delegate_private_symbol, "node:inspector:delegate")             \
   V(decorated_private_symbol, "node:decorated")                               \
   V(npn_buffer_private_symbol, "node:npnBuffer")                              \
   V(processed_private_symbol, "node:processed")                               \

src/inspector_agent.cc

@@ -1,6 +1,8 @@
 #include "inspector_agent.h"
 
 #include "inspector_io.h"
+#include "base-object.h"
+#include "base-object-inl.h"
 #include "node_internals.h"
 #include "v8-inspector.h"
 #include "v8-platform.h"
@@ -26,9 +28,9 @@ using node::FatalError;
 using v8::Array;
 using v8::Boolean;
 using v8::Context;
-using v8::External;
 using v8::Function;
 using v8::FunctionCallbackInfo;
+using v8::FunctionTemplate;
 using v8::HandleScope;
 using v8::Integer;
 using v8::Isolate;
@@ -191,143 +193,117 @@ static int StartDebugSignalHandler() {
 }
 #endif  // _WIN32
 
-class JsBindingsSessionDelegate : public InspectorSessionDelegate {
+class JSBindingsConnection : public AsyncWrap {
  public:
-  JsBindingsSessionDelegate(Environment* env,
-                            Local<Object> session,
-                            Local<Object> receiver,
-                            Local<Function> callback)
-                            : env_(env),
-                              session_(env->isolate(), session),
-                              receiver_(env->isolate(), receiver),
-                              callback_(env->isolate(), callback) {
-    session_.SetWeak(this, JsBindingsSessionDelegate::Release,
-                     v8::WeakCallbackType::kParameter);
-  }
-
-  ~JsBindingsSessionDelegate() override {
-    session_.Reset();
-    receiver_.Reset();
-    callback_.Reset();
-  }
+  class JSBindingsSessionDelegate : public InspectorSessionDelegate {
+   public:
+    JSBindingsSessionDelegate(Environment* env,
+                              JSBindingsConnection* connection)
+                              : env_(env),
+                                connection_(connection) {
+    }
 
-  bool WaitForFrontendMessageWhilePaused() override {
-    return false;
-  }
+    bool WaitForFrontendMessageWhilePaused() override {
+      return false;
+    }
 
-  void SendMessageToFrontend(const v8_inspector::StringView& message) override {
-    Isolate* isolate = env_->isolate();
-    v8::HandleScope handle_scope(isolate);
-    Context::Scope context_scope(env_->context());
-    MaybeLocal<String> v8string =
-        String::NewFromTwoByte(isolate, message.characters16(),
-                               NewStringType::kNormal, message.length());
-    Local<Value> argument = v8string.ToLocalChecked().As<Value>();
-    Local<Function> callback = callback_.Get(isolate);
-    Local<Object> receiver = receiver_.Get(isolate);
-    callback->Call(env_->context(), receiver, 1, &argument)
-        .FromMaybe(Local<Value>());
-  }
+    void SendMessageToFrontend(const v8_inspector::StringView& message)
+        override {
+      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> argument = v8string.ToLocalChecked().As<Value>();
+      connection_->OnMessage(argument);
+    }
 
-  void Disconnect() {
-    Agent* agent = env_->inspector_agent();
-    if (agent->delegate() == this)
-      agent->Disconnect();
-  }
+    void Disconnect() {
+      Agent* agent = env_->inspector_agent();
+      if (agent->delegate() == this)
+        agent->Disconnect();
+    }
 
- private:
-  static void Release(
-      const v8::WeakCallbackInfo<JsBindingsSessionDelegate>& info) {
-    info.SetSecondPassCallback(ReleaseSecondPass);
-    info.GetParameter()->session_.Reset();
+   private:
+    Environment* env_;
+    JSBindingsConnection* connection_;
+  };
+
+  JSBindingsConnection(Environment* env,
+                       Local<Object> wrap,
+                       Local<Function> callback)
+                       : AsyncWrap(env, wrap, PROVIDER_INSPECTORJSBINDING),
+                         delegate_(env, this),
+                         callback_(env->isolate(), callback) {
+    Wrap(wrap, this);
+
+    Agent* inspector = env->inspector_agent();
+    if (inspector->delegate() != nullptr) {
+      env->ThrowTypeError("Session is already attached");
+      return;
+    }
+    inspector->Connect(&delegate_);
   }
 
-  static void ReleaseSecondPass(
-      const v8::WeakCallbackInfo<JsBindingsSessionDelegate>& info) {
-    JsBindingsSessionDelegate* delegate = info.GetParameter();
-    delegate->Disconnect();
-    delete delegate;
+  ~JSBindingsConnection() override {
+    callback_.Reset();
   }
 
-  Environment* env_;
-  Persistent<Object> session_;
-  Persistent<Object> receiver_;
-  Persistent<Function> callback_;
-};
+  void OnMessage(Local<Value> value) {
+    MakeCallback(callback_.Get(env()->isolate()), 1, &value);
+  }
 
-void SetDelegate(Environment* env, Local<Object> inspector,
-                 JsBindingsSessionDelegate* delegate) {
-  inspector->SetPrivate(env->context(),
-                        env->inspector_delegate_private_symbol(),
-                        v8::External::New(env->isolate(), delegate));
-}
+  void CheckIsCurrent() {
+    Agent* inspector = env()->inspector_agent();
+    CHECK_EQ(&delegate_, inspector->delegate());
+  }
 
-Maybe<JsBindingsSessionDelegate*> GetDelegate(
-    const FunctionCallbackInfo<Value>& info) {
-  Environment* env = Environment::GetCurrent(info);
-  Local<Value> delegate;
-  MaybeLocal<Value> maybe_delegate =
-      info.This()->GetPrivate(env->context(),
-                              env->inspector_delegate_private_symbol());
-
-  if (maybe_delegate.ToLocal(&delegate)) {
-    CHECK(delegate->IsExternal());
-    void* value = delegate.As<External>()->Value();
-    if (value != nullptr) {
-      return v8::Just(static_cast<JsBindingsSessionDelegate*>(value));
+  static void New(const FunctionCallbackInfo<Value>& info) {
+    Environment* env = Environment::GetCurrent(info);
+    if (!info[0]->IsFunction()) {
+      env->ThrowTypeError("Message callback is required");
+      return;
     }
+    Local<Function> callback = info[0].As<Function>();
+    new JSBindingsConnection(env, info.This(), callback);
   }
-  env->ThrowError("Inspector is not connected");
-  return v8::Nothing<JsBindingsSessionDelegate*>();
-}
 
-void Dispatch(const FunctionCallbackInfo<Value>& info) {
-  Environment* env = Environment::GetCurrent(info);
-  if (!info[0]->IsString()) {
-    env->ThrowError("Inspector message must be a string");
-    return;
+  void Disconnect() {
+    delegate_.Disconnect();
+    if (!persistent().IsEmpty()) {
+      ClearWrap(object());
+      persistent().Reset();
+    }
+    delete this;
   }
-  Maybe<JsBindingsSessionDelegate*> maybe_delegate = GetDelegate(info);
-  if (maybe_delegate.IsNothing())
-    return;
-  Agent* inspector = env->inspector_agent();
-  CHECK_EQ(maybe_delegate.ToChecked(), inspector->delegate());
-  inspector->Dispatch(ToProtocolString(env->isolate(), info[0])->string());
-}
 
-void Disconnect(const FunctionCallbackInfo<Value>& info) {
-  Environment* env = Environment::GetCurrent(info);
-  Maybe<JsBindingsSessionDelegate*> delegate = GetDelegate(info);
-  if (delegate.IsNothing()) {
-    return;
+  static void Disconnect(const FunctionCallbackInfo<Value>& info) {
+    JSBindingsConnection* session;
+    ASSIGN_OR_RETURN_UNWRAP(&session, info.Holder());
+    session->Disconnect();
   }
-  delegate.ToChecked()->Disconnect();
-  SetDelegate(env, info.This(), nullptr);
-  delete delegate.ToChecked();
-}
 
-void ConnectJSBindingsSession(const FunctionCallbackInfo<Value>& info) {
-  Environment* env = Environment::GetCurrent(info);
-  if (!info[0]->IsFunction()) {
-    env->ThrowError("Message callback is required");
-    return;
-  }
-  Agent* inspector = env->inspector_agent();
-  if (inspector->delegate() != nullptr) {
-    env->ThrowError("Session is already attached");
-    return;
+  static void Dispatch(const FunctionCallbackInfo<Value>& info) {
+    Environment* env = Environment::GetCurrent(info);
+    JSBindingsConnection* session;
+    ASSIGN_OR_RETURN_UNWRAP(&session, info.Holder());
+    if (!info[0]->IsString()) {
+      env->ThrowTypeError("Inspector message must be a string");
+      return;
+    }
+
+    session->CheckIsCurrent();
+    Agent* inspector = env->inspector_agent();
+    inspector->Dispatch(ToProtocolString(env->isolate(), info[0])->string());
   }
-  Local<Object> session = Object::New(env->isolate());
-  env->SetMethod(session, "dispatch", Dispatch);
-  env->SetMethod(session, "disconnect", Disconnect);
-  info.GetReturnValue().Set(session);
 
-  JsBindingsSessionDelegate* delegate =
-      new JsBindingsSessionDelegate(env, session, info.Holder(),
-                                    info[0].As<Function>());
-  inspector->Connect(delegate);
-  SetDelegate(env, session, delegate);
-}
+  size_t self_size() const override { return sizeof(*this); }
+
+ private:
+  JSBindingsSessionDelegate delegate_;
+  Persistent<Function> callback_;
+};
 
 void InspectorConsoleCall(const v8::FunctionCallbackInfo<Value>& info) {
   Isolate* isolate = info.GetIsolate();
@@ -973,7 +949,6 @@ void Agent::InitInspector(Local<Object> target, Local<Value> unused,
   env->SetMethod(target, "addCommandLineAPI", AddCommandLineAPI);
   if (agent->debug_options_.wait_for_connect())
     env->SetMethod(target, "callAndPauseOnStart", CallAndPauseOnStart);
-  env->SetMethod(target, "connect", ConnectJSBindingsSession);
   env->SetMethod(target, "open", Open);
   env->SetMethod(target, "url", Url);
 
@@ -987,6 +962,16 @@ void Agent::InitInspector(Local<Object> target, Local<Value> unused,
 
   env->SetMethod(target, "registerAsyncHook", RegisterAsyncHookWrapper);
   env->SetMethod(target, "isEnabled", IsEnabled);
+
+  auto conn_str = FIXED_ONE_BYTE_STRING(env->isolate(), "Connection");
+  Local<FunctionTemplate> tmpl =
+      env->NewFunctionTemplate(JSBindingsConnection::New);
+  tmpl->InstanceTemplate()->SetInternalFieldCount(1);
+  tmpl->SetClassName(conn_str);
+  AsyncWrap::AddWrapMethods(env, tmpl);
+  env->SetProtoMethod(tmpl, "dispatch", JSBindingsConnection::Dispatch);
+  env->SetProtoMethod(tmpl, "disconnect", JSBindingsConnection::Disconnect);
+  target->Set(env->context(), conn_str, tmpl->GetFunction()).ToChecked();
 }
 
 void Agent::RequestIoThreadStart() {

test/sequential/test-async-wrap-getasyncid.js

@@ -273,3 +273,10 @@ if (common.hasCrypto) { // eslint-disable-line crypto-check
   handle.send(req, [Buffer.alloc(1)], 1, req.port, req.address, true);
   testInitialized(req, 'SendWrap');
 }
+
+if (process.config.variables.v8_enable_inspector !== 0) {
+  const binding = process.binding('inspector');
+  const handle = new binding.Connection(() => {});
+  testInitialized(handle, 'Connection');
+  handle.disconnect();
+}