inspector: do not add 'inspector' property

Eugene Ostroukhov · Eugene Ostroukhov · commit 3f48ab304209 · 2017-05-04T10:40:24.000-07:00
'inspector' property is not an official API and should not be published on process object, where the user may discover it. This change was extracted from nodejs#12263 that will be focused on creating JS bindings. PR-URL: nodejs#12656 Reviewed-By: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
diff --git a/lib/internal/bootstrap_node.js b/lib/internal/bootstrap_node.js
@@ -254,52 +254,41 @@
   }
 
   function setupGlobalConsole() {
-    var inspectorConsole;
-    var wrapConsoleCall;
-    if (process.inspector) {
-      inspectorConsole = global.console;
-      wrapConsoleCall = process.inspector.wrapConsoleCall;
-      delete process.inspector.wrapConsoleCall;
-      if (Object.keys(process.inspector).length === 0)
-        delete process.inspector;
-    }
-    var console;
+    const originalConsole = global.console;
+    let console;
     Object.defineProperty(global, 'console', {
       configurable: true,
       enumerable: true,
       get: function() {
         if (!console) {
-          console = NativeModule.require('console');
-          installInspectorConsoleIfNeeded(console,
-                                          inspectorConsole,
-                                          wrapConsoleCall);
+          console = installInspectorConsole(originalConsole);
         }
         return console;
       }
     });
   }
 
-  function installInspectorConsoleIfNeeded(console,
-                                           inspectorConsole,
-                                           wrapConsoleCall) {
-    if (!inspectorConsole)
-      return;
+  function installInspectorConsole(globalConsole) {
+    const wrappedConsole = NativeModule.require('console');
+    const inspector = process.binding('inspector');
     const config = {};
-    for (const key of Object.keys(console)) {
-      if (!inspectorConsole.hasOwnProperty(key))
+    for (const key of Object.keys(wrappedConsole)) {
+      if (!globalConsole.hasOwnProperty(key))
         continue;
-      // If node console has the same method as inspector console,
+      // If global console has the same method as inspector console,
       // then wrap these two methods into one. Native wrapper will preserve
       // the original stack.
-      console[key] = wrapConsoleCall(inspectorConsole[key],
-                                     console[key],
-                                     config);
+      wrappedConsole[key] = inspector.consoleCall.bind(wrappedConsole,
+                                                       globalConsole[key],
+                                                       wrappedConsole[key],
+                                                       config);
     }
-    for (const key of Object.keys(inspectorConsole)) {
-      if (console.hasOwnProperty(key))
+    for (const key of Object.keys(globalConsole)) {
+      if (wrappedConsole.hasOwnProperty(key))
         continue;
-      console[key] = inspectorConsole[key];
+      wrappedConsole[key] = globalConsole[key];
     }
+    return wrappedConsole;
   }
 
   function setupProcessFatal() {
diff --git a/lib/module.js b/lib/module.js
@@ -472,19 +472,6 @@ function tryModuleLoad(module, filename) {
   }
 }
 
-function getInspectorCallWrapper() {
-  var inspector = process.inspector;
-  if (!inspector || !inspector.callAndPauseOnStart) {
-    return null;
-  }
-  var wrapper = inspector.callAndPauseOnStart.bind(inspector);
-  delete inspector.callAndPauseOnStart;
-  if (Object.keys(process.inspector).length === 0) {
-    delete process.inspector;
-  }
-  return wrapper;
-}
-
 Module._resolveFilename = function(request, parent, isMain) {
   if (NativeModule.nonInternalExists(request)) {
     return request;
@@ -563,7 +550,7 @@ Module.prototype._compile = function(content, filename) {
     // Set breakpoint on module start
     if (filename === resolvedArgv) {
       delete process._debugWaitConnect;
-      inspectorWrapper = getInspectorCallWrapper();
+      inspectorWrapper = process.binding('inspector').callAndPauseOnStart;
       if (!inspectorWrapper) {
         const Debug = vm.runInDebugContext('Debug');
         Debug.setBreakPoint(compiledWrapper, 0, 0);
diff --git a/src/env.h b/src/env.h
@@ -72,6 +72,7 @@ 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
@@ -154,8 +154,61 @@ static int RegisterDebugSignalHandler() {
   return 0;
 }
 #endif  // _WIN32
-}  // namespace
 
+void InspectorConsoleCall(const v8::FunctionCallbackInfo<Value>& info) {
+  Isolate* isolate = info.GetIsolate();
+  HandleScope handle_scope(isolate);
+  Local<Context> context = isolate->GetCurrentContext();
+  CHECK_LT(2, info.Length());
+  std::vector<Local<Value>> call_args;
+  for (int i = 3; i < info.Length(); ++i) {
+    call_args.push_back(info[i]);
+  }
+  Environment* env = Environment::GetCurrent(isolate);
+  if (env->inspector_agent()->enabled()) {
+    Local<Value> inspector_method = info[0];
+    CHECK(inspector_method->IsFunction());
+    Local<Value> config_value = info[2];
+    CHECK(config_value->IsObject());
+    Local<Object> config_object = config_value.As<Object>();
+    Local<String> in_call_key = FIXED_ONE_BYTE_STRING(isolate, "in_call");
+    if (!config_object->Has(context, in_call_key).FromMaybe(false)) {
+      CHECK(config_object->Set(context,
+                               in_call_key,
+                               v8::True(isolate)).FromJust());
+      CHECK(!inspector_method.As<Function>()->Call(context,
+                                                   info.Holder(),
+                                                   call_args.size(),
+                                                   call_args.data()).IsEmpty());
+    }
+    CHECK(config_object->Delete(context, in_call_key).FromJust());
+  }
+
+  Local<Value> node_method = info[1];
+  CHECK(node_method->IsFunction());
+  static_cast<void>(node_method.As<Function>()->Call(context,
+                                                     info.Holder(),
+                                                     call_args.size(),
+                                                     call_args.data()));
+}
+
+void CallAndPauseOnStart(
+    const v8::FunctionCallbackInfo<v8::Value>& args) {
+  Environment* env = Environment::GetCurrent(args);
+  CHECK_GT(args.Length(), 1);
+  CHECK(args[0]->IsFunction());
+  std::vector<v8::Local<v8::Value>> call_args;
+  for (int i = 2; i < args.Length(); i++) {
+    call_args.push_back(args[i]);
+  }
+
+  env->inspector_agent()->PauseOnNextJavascriptStatement("Break on start");
+  v8::MaybeLocal<v8::Value> retval =
+      args[0].As<v8::Function>()->Call(env->context(), args[1],
+                                       call_args.size(), call_args.data());
+  args.GetReturnValue().Set(retval.ToLocalChecked());
+}
+}  // namespace
 
 // Used in NodeInspectorClient::currentTimeMS() below.
 const int NANOS_PER_MSEC = 1000000;
@@ -263,12 +316,6 @@ class NodeInspectorClient : public v8_inspector::V8InspectorClient {
     channel_->dispatchProtocolMessage(message);
   }
 
-  void schedulePauseOnNextStatement(const std::string& reason) {
-    if (channel_ != nullptr) {
-      channel_->schedulePauseOnNextStatement(reason);
-    }
-  }
-
   Local<Context> ensureDefaultContextInGroup(int contextGroupId) override {
     return env_->context();
   }
@@ -302,10 +349,8 @@ class NodeInspectorClient : public v8_inspector::V8InspectorClient {
         script_id);
   }
 
-  InspectorSessionDelegate* delegate() {
-    if (channel_ == nullptr)
-      return nullptr;
-    return channel_->delegate();
+  ChannelImpl* channel() {
+    return channel_.get();
   }
 
  private:
@@ -320,98 +365,22 @@ class NodeInspectorClient : public v8_inspector::V8InspectorClient {
 Agent::Agent(Environment* env) : parent_env_(env),
                                  inspector_(nullptr),
                                  platform_(nullptr),
-                                 inspector_console_(false) {}
+                                 enabled_(false) {}
 
-// Header has unique_ptr to some incomplete types - this definition tells
-// the compiler to figure out destruction here, were those types are complete
+// Destructor needs to be defined here in implementation file as the header
+// does not have full definition of some classes.
 Agent::~Agent() {
 }
 
-// static
-void Agent::InspectorConsoleCall(const v8::FunctionCallbackInfo<Value>& info) {
-  Isolate* isolate = info.GetIsolate();
-  Local<Context> context = isolate->GetCurrentContext();
-
-  CHECK(info.Data()->IsArray());
-  Local<v8::Array> args = info.Data().As<v8::Array>();
-  CHECK_EQ(args->Length(), 3);
-
-  std::vector<Local<Value>> call_args(info.Length());
-  for (int i = 0; i < info.Length(); ++i) {
-    call_args[i] = info[i];
-  }
-
-  Environment* env = Environment::GetCurrent(isolate);
-  if (env->inspector_agent()->inspector_console_) {
-    Local<Value> inspector_method = args->Get(context, 0).ToLocalChecked();
-    CHECK(inspector_method->IsFunction());
-    Local<Value> config_value = args->Get(context, 2).ToLocalChecked();
-    CHECK(config_value->IsObject());
-    Local<Object> config_object = config_value.As<Object>();
-    Local<String> in_call_key = FIXED_ONE_BYTE_STRING(isolate, "in_call");
-    if (!config_object->Has(context, in_call_key).FromMaybe(false)) {
-      CHECK(config_object->Set(context,
-                               in_call_key,
-                               v8::True(isolate)).FromJust());
-      CHECK(!inspector_method.As<Function>()->Call(context,
-                                                   info.Holder(),
-                                                   call_args.size(),
-                                                   call_args.data()).IsEmpty());
-    }
-    CHECK(config_object->Delete(context, in_call_key).FromJust());
-  }
-
-  Local<Value> node_method =
-      args->Get(context, 1).ToLocalChecked();
-  CHECK(node_method->IsFunction());
-  static_cast<void>(node_method.As<Function>()->Call(context,
-                                                     info.Holder(),
-                                                     call_args.size(),
-                                                     call_args.data()));
-}
-
-// static
-void Agent::InspectorWrapConsoleCall(const FunctionCallbackInfo<Value>& info) {
-  Environment* env = Environment::GetCurrent(info);
-  if (info.Length() != 3 || !info[0]->IsFunction() ||
-      !info[1]->IsFunction() || !info[2]->IsObject()) {
-    return env->ThrowError("inspector.wrapConsoleCall takes exactly 3 "
-        "arguments: two functions and an object.");
-  }
-
-  Local<v8::Array> array = v8::Array::New(env->isolate(), info.Length());
-  CHECK(array->Set(env->context(), 0, info[0]).FromJust());
-  CHECK(array->Set(env->context(), 1, info[1]).FromJust());
-  CHECK(array->Set(env->context(), 2, info[2]).FromJust());
-  info.GetReturnValue().Set(Function::New(env->context(),
-                                          InspectorConsoleCall,
-                                          array).ToLocalChecked());
-}
-
 bool Agent::Start(v8::Platform* platform, const char* path,
                   const DebugOptions& options) {
   path_ = path == nullptr ? "" : path;
   debug_options_ = options;
-  inspector_console_ = false;
   inspector_ =
       std::unique_ptr<NodeInspectorClient>(
           new NodeInspectorClient(parent_env_, platform));
   platform_ = platform;
-  Local<Object> process = parent_env_->process_object();
-  Local<Object> inspector = Object::New(parent_env_->isolate());
-  Local<String> name =
-      FIXED_ONE_BYTE_STRING(parent_env_->isolate(), "inspector");
-  process->DefineOwnProperty(parent_env_->context(),
-                             name,
-                             inspector,
-                             v8::ReadOnly).FromJust();
-  parent_env_->SetMethod(inspector, "wrapConsoleCall",
-                         InspectorWrapConsoleCall);
   if (options.inspector_enabled()) {
-    if (options.wait_for_connect()) {
-      parent_env_->SetMethod(inspector, "callAndPauseOnStart",
-                             CallAndPauseOnStart);
-    }
     return StartIoThread();
   } else {
     CHECK_EQ(0, uv_async_init(uv_default_loop(),
@@ -431,7 +400,7 @@ bool Agent::StartIoThread() {
 
   CHECK_NE(inspector_, nullptr);
 
-  inspector_console_ = true;
+  enabled_ = true;
   io_ = std::unique_ptr<InspectorIo>(
       new InspectorIo(parent_env_, platform_, path_, debug_options_));
   if (!io_->Start()) {
@@ -469,7 +438,7 @@ void Agent::Stop() {
 }
 
 void Agent::Connect(InspectorSessionDelegate* delegate) {
-  inspector_console_ = true;
+  enabled_ = true;
   inspector_->connectFrontend(delegate);
 }
 
@@ -481,26 +450,6 @@ bool Agent::IsStarted() {
   return !!inspector_;
 }
 
-// static
-void Agent::CallAndPauseOnStart(
-    const v8::FunctionCallbackInfo<v8::Value>& args) {
-  Environment* env = Environment::GetCurrent(args);
-  CHECK_GT(args.Length(), 1);
-  CHECK(args[0]->IsFunction());
-  std::vector<v8::Local<v8::Value>> call_args;
-  for (int i = 2; i < args.Length(); i++) {
-    call_args.push_back(args[i]);
-  }
-
-  Agent* agent = env->inspector_agent();
-  agent->inspector_->schedulePauseOnNextStatement("Break on start");
-
-  v8::MaybeLocal<v8::Value> retval =
-      args[0].As<v8::Function>()->Call(env->context(), args[1],
-                                       call_args.size(), call_args.data());
-  args.GetReturnValue().Set(retval.ToLocalChecked());
-}
-
 void Agent::WaitForDisconnect() {
   if (io_ != nullptr) {
     io_->WaitForDisconnect();
@@ -529,5 +478,23 @@ void Agent::RunMessageLoop() {
   inspector_->runMessageLoopOnPause(CONTEXT_GROUP_ID);
 }
 
+void Agent::PauseOnNextJavascriptStatement(const std::string& reason) {
+  ChannelImpl* channel = inspector_->channel();
+  if (channel != nullptr)
+    channel->schedulePauseOnNextStatement(reason);
+}
+
+// static
+void Agent::InitJSBindings(Local<Object> target, Local<Value> unused,
+                           Local<Context> context, void* priv) {
+  Environment* env = Environment::GetCurrent(context);
+  Agent* agent = env->inspector_agent();
+  env->SetMethod(target, "consoleCall", InspectorConsoleCall);
+  if (agent->debug_options_.wait_for_connect())
+    env->SetMethod(target, "callAndPauseOnStart", CallAndPauseOnStart);
+}
 }  // namespace inspector
 }  // namespace node
+
+NODE_MODULE_CONTEXT_AWARE_BUILTIN(inspector,
+                                  node::inspector::Agent::InitJSBindings);
diff --git a/src/inspector_agent.h b/src/inspector_agent.h
diff --git a/src/node.cc b/src/node.cc
