inspector: use BaseObject

Uses internal fields instead of the less efficient v8::External for storing the pointer to the C++ object. Refs: nodejs#13503
TimothyGu · Aug 9, 2017 · faa4eaa · faa4eaa
1 parent 11cb107
commit faa4eaa
Show file tree

Hide file tree

Showing 3 changed files with 80 additions and 69 deletions.
diff --git a/lib/inspector.js b/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');
@@ -23,8 +23,7 @@ class Session extends EventEmitter {
   connect() {
     if (this[connectionSymbol])
       throw new Error('Already connected');
-    this[connectionSymbol] =
-        connect((message) => this[onMessageSymbol](message));
+    this[connectionSymbol] = new Connection(this[onMessageSymbol], this);
   }
 
   [onMessageSymbol](message) {

diff --git a/src/env.h b/src/env.h
@@ -76,7 +76,6 @@ namespace node {
   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")                               \

diff --git a/src/inspector_agent.cc b/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 "env.h"
 #include "env-inl.h"
 #include "node.h"
@@ -27,6 +29,7 @@ using v8::Context;
 using v8::External;
 using v8::Function;
 using v8::FunctionCallbackInfo;
+using v8::FunctionTemplate;
 using v8::HandleScope;
 using v8::Isolate;
 using v8::Local;
@@ -250,79 +253,81 @@ class JsBindingsSessionDelegate : public InspectorSessionDelegate {
   Persistent<Function> callback_;
 };
 
-void SetDelegate(Environment* env, Local<Object> inspector,
-                 JsBindingsSessionDelegate* delegate) {
-  inspector->SetPrivate(env->context(),
-                        env->inspector_delegate_private_symbol(),
-                        v8::External::New(env->isolate(), 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));
+class JSBindingsConnection : public BaseObject {
+ public:
+  JSBindingsConnection(Environment* env,
+                       Local<Object> wrap,
+                       Local<Object> receiver,
+                       Local<Function> callback)
+                       : BaseObject(env, wrap) {
+    MakeWeak<JSBindingsConnection>(this);
+    Wrap(wrap, this);
+
+    Agent* inspector = env->inspector_agent();
+    if (inspector->delegate() != nullptr) {
+      env->ThrowTypeError("Session is already attached");
+      return;
     }
+    delegate_ = new JsBindingsSessionDelegate(env, wrap, receiver, callback);
+    inspector->Connect(delegate_);
   }
-  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;
+  ~JSBindingsConnection() override {
+    Disconnect();
   }
-  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;
+  JsBindingsSessionDelegate* delegate() {
+    return delegate_;
   }
-  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;
+  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>();
+    Local<Object> receiver =
+        info[1]->IsObject() ? info[1].As<Object>() : info.This();
+    new JSBindingsConnection(env, info.This(), receiver, callback);
   }
-  Agent* inspector = env->inspector_agent();
-  if (inspector->delegate() != nullptr) {
-    env->ThrowError("Session is already attached");
-    return;
+
+  void Disconnect() {
+    if (!delegate_)
+      return;
+    delegate_->Disconnect();
+    delete delegate_;
+    delegate_ = nullptr;
   }
-  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);
-}
+  static void Disconnect(const FunctionCallbackInfo<Value>& info) {
+    JSBindingsConnection* session;
+    ASSIGN_OR_RETURN_UNWRAP(&session, info.Holder());
+    session->Disconnect();
+  }
+
+  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;
+    }
+
+    auto delegate = session->delegate();
+    if (!delegate) {
+      env->ThrowTypeError("Inspector is not connected");
+      return;
+    }
+    Agent* inspector = env->inspector_agent();
+    CHECK_EQ(delegate, inspector->delegate());
+    inspector->Dispatch(ToProtocolString(env->isolate(), info[0])->string());
+  }
+
+ private:
+  JsBindingsSessionDelegate* delegate_;
+};
 
 void InspectorConsoleCall(const v8::FunctionCallbackInfo<Value>& info) {
   Isolate* isolate = info.GetIsolate();
@@ -825,9 +830,17 @@ 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);
+
+  auto conn_str = FIXED_ONE_BYTE_STRING(env->isolate(), "Connection");
+  Local<FunctionTemplate> tmpl =
+      env->NewFunctionTemplate(JSBindingsConnection::New);
+  tmpl->InstanceTemplate()->SetInternalFieldCount(1);
+  env->SetProtoMethod(tmpl, "dispatch", JSBindingsConnection::Dispatch);
+  env->SetProtoMethod(tmpl, "disconnect", JSBindingsConnection::Disconnect);
+  tmpl->SetClassName(conn_str);
+  target->Set(env->context(), conn_str, tmpl->GetFunction()).ToChecked();
 }
 
 void Agent::RequestIoThreadStart() {