chore: remove custom Node.js debugger (#25587)

filenames.gni

@@ -256,8 +256,6 @@ filenames = {
     "shell/browser/net/web_request_api_interface.h",
     "shell/browser/network_hints_handler_impl.cc",
     "shell/browser/network_hints_handler_impl.h",
-    "shell/browser/node_debugger.cc",
-    "shell/browser/node_debugger.h",
     "shell/browser/notifications/linux/libnotify_notification.cc",
     "shell/browser/notifications/linux/libnotify_notification.h",
     "shell/browser/notifications/linux/notification_presenter_linux.cc",

patches/node/.patches

@@ -4,7 +4,6 @@ refactor_alter_child_process_fork_to_use_execute_script_with.patch
 feat_add_uv_loop_watcher_queue_code.patch
 feat_initialize_asar_support.patch
 expose_get_builtin_module_function.patch
-fix_build_and_expose_inspector_agent.patch
 fix_expose_internalcallbackscope.patch
 build_add_gn_build_files.patch
 fix_add_default_values_for_enable_lto_and_build_v8_with_gn_in.patch
@@ -30,5 +29,5 @@ update_tests_after_increasing_typed_array_size.patch
 feat_add_implementation_of_v8_platform_postjob.patch
 crypto_update_certdata_to_nss_3_56.patch
 fix_-wincompatible-pointer-types-discards-qualifiers_error.patch
-fix_allow_preventing_initializeinspector_in_env.patch
 fix_add_v8_enable_reverse_jsargs_defines_in_common_gypi.patch
+fix_allow_preventing_initializeinspector_in_env.patch

patches/node/fix_allow_preventing_initializeinspector_in_env.patch

@@ -1,18 +1,17 @@
 From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
 From: Shelley Vohr <[email protected]>
-Date: Tue, 1 Sep 2020 19:30:08 -0700
+Date: Tue, 22 Sep 2020 19:44:30 -0700
 Subject: fix: allow preventing InitializeInspector in env
 
 https://github.com/nodejs/node/commit/8c5ad1392f30cfe6b107e9bd85f4cb918ba04aab
 made it such that env->InitializeInspector was called in CreateEnvironment
-no matter what, which creates an issue for embedders seeking to manage
-the InspectorAgent themselves as Electron does. This adds a new
+no matter what, which creates an issue for Electron, as the V8 inspector
+already exists in the renderer process and therefore we only want to
+initialize it in the browser process. This adds a new
 EnvironmentFlags option which allows preventing that invocation.
 
-This will be upstreamed.
-
 diff --git a/src/api/environment.cc b/src/api/environment.cc
-index 4fa2d4175fb318f560b7b9a8ed8baa091d3ed6a2..7bd899c5933ac2e6178d650632699e19e85c00c9 100644
+index 4fa2d4175fb318f560b7b9a8ed8baa091d3ed6a2..cf6115d04ba3c184937c5db85c9d7ebc78ed3db7 100644
 --- a/src/api/environment.cc
 +++ b/src/api/environment.cc
 @@ -358,12 +358,14 @@ Environment* CreateEnvironment(
@@ -25,7 +24,7 @@ index 4fa2d4175fb318f560b7b9a8ed8baa091d3ed6a2..7bd899c5933ac2e6178d650632699e19
 -            inspector_parent_handle.get())->impl));
 -  } else {
 -    env->InitializeInspector({});
-+  if (!env->should_not_initialize_inspector()) {
++  if (env->should_initialize_inspector()) {
 +    if (inspector_parent_handle) {
 +      env->InitializeInspector(
 +          std::move(static_cast<InspectorParentHandleImpl*>(
@@ -37,34 +36,34 @@ index 4fa2d4175fb318f560b7b9a8ed8baa091d3ed6a2..7bd899c5933ac2e6178d650632699e19
  #endif
  
 diff --git a/src/env-inl.h b/src/env-inl.h
-index 623e9d4e429c03bb267539a318166f3ef3b9c501..8fc5f720764dd4ca536ae01ca78b2c7e3e9fd007 100644
+index 623e9d4e429c03bb267539a318166f3ef3b9c501..52a122a51049238547ff662bed1a10b346f3af00 100644
 --- a/src/env-inl.h
 +++ b/src/env-inl.h
-@@ -833,6 +833,10 @@ inline bool Environment::owns_inspector() const {
-   return flags_ & EnvironmentFlags::kOwnsInspector;
+@@ -837,6 +837,10 @@ inline bool Environment::tracks_unmanaged_fds() const {
+   return flags_ & EnvironmentFlags::kTrackUnmanagedFds;
  }
  
-+inline bool Environment::should_not_initialize_inspector() const {
-+  return flags_ & EnvironmentFlags::kNoInitializeInspector;
++inline bool Environment::should_initialize_inspector() const {
++  return (flags_ & EnvironmentFlags::kNoInitializeInspector) == 0;
 +}
 +
- inline bool Environment::tracks_unmanaged_fds() const {
-   return flags_ & EnvironmentFlags::kTrackUnmanagedFds;
+ bool Environment::filehandle_close_warning() const {
+   return emit_filehandle_warning_;
  }
 diff --git a/src/env.h b/src/env.h
-index 38d17f4e18aa38fde2c2f59a9816c8fb0f65fd51..4b9c2780f9736cb8bde60f40abb9aac9d53160a1 100644
+index 38d17f4e18aa38fde2c2f59a9816c8fb0f65fd51..4fe2eb3b7699efcab87c377743a955effbbfd9de 100644
 --- a/src/env.h
 +++ b/src/env.h
-@@ -1019,6 +1019,7 @@ class Environment : public MemoryRetainer {
- 
-   inline bool is_main_thread() const;
-   inline bool should_not_register_esm_loader() const;
-+  inline bool should_not_initialize_inspector() const;
+@@ -1022,6 +1022,7 @@ class Environment : public MemoryRetainer {
    inline bool owns_process_state() const;
    inline bool owns_inspector() const;
    inline bool tracks_unmanaged_fds() const;
++  inline bool should_initialize_inspector() const;
+   inline uint64_t thread_id() const;
+   inline worker::Worker* worker_context() const;
+   Environment* worker_parent_env() const;
 diff --git a/src/node.h b/src/node.h
-index 80acb3f9f04ef8e6c363cf31384af4037abeeb87..6b657f6941b8f96da08b6397e01e19a2763edf8f 100644
+index 80acb3f9f04ef8e6c363cf31384af4037abeeb87..22f037b0b26f39f9ce94c4a364b27cf204366cb9 100644
 --- a/src/node.h
 +++ b/src/node.h
 @@ -424,7 +424,11 @@ enum Flags : uint64_t {
@@ -73,9 +72,9 @@ index 80acb3f9f04ef8e6c363cf31384af4037abeeb87..6b657f6941b8f96da08b6397e01e19a2
    // by fs.open() and fs.close(), and close them during FreeEnvironment().
 -  kTrackUnmanagedFds = 1 << 4
 +  kTrackUnmanagedFds = 1 << 4,
-+  // This flag should be set to prevent InspectorAgent initialization from
-+  // within the environment. This is used by embedders who wish to manage the
-+  // InspectorAgent themselves.
++  // Controls whether or not the Environment should call InitializeInspector.
++  // This control is needed by embedders who may not want to initialize the V8
++  // inspector in situations where it already exists.
 +  kNoInitializeInspector = 1 << 5
  };
  }  // namespace EnvironmentFlags

patches/node/fix_build_and_expose_inspector_agent.patch

@@ -1,65 +0,0 @@
-From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
-From: Shelley Vohr <[email protected]>
-Date: Mon, 30 Jul 2018 15:18:11 -0700
-Subject: fix: build and expose inspector agent
-
-Node inspector initialization happens in a different start-up function in node.cc, which we don't call in Electron. This allows for us to use the inspector agent in electron/atom/browser/node_debugger.cc
-
-diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc
-index e6ab76abf56168041108972d54d741af988342b4..7de9d75e49a08625bbd37f5bfcee3f88c5fa978d 100644
---- a/src/inspector_agent.cc
-+++ b/src/inspector_agent.cc
-@@ -203,7 +203,7 @@ const int CONTEXT_GROUP_ID = 1;
- 
- std::string GetWorkerLabel(node::Environment* env) {
-   std::ostringstream result;
--  result << "Worker[" << env->thread_id() << "]";
-+  result << "Electron Worker[" << env->thread_id() << "]";
-   return result.str();
- }
- 
-@@ -458,7 +458,7 @@ class NodeInspectorClient : public V8InspectorClient {
-     client_ = V8Inspector::create(env->isolate(), this);
-     // TODO(bnoordhuis) Make name configurable from src/node.cc.
-     std::string name =
--        is_main_ ? GetHumanReadableProcessName() : GetWorkerLabel(env);
-+        is_main_ ? "Electron Main Context" : GetWorkerLabel(env);
-     ContextInfo info(name);
-     info.is_default = true;
-     contextCreated(env->context(), info);
-diff --git a/src/inspector_agent.h b/src/inspector_agent.h
-index efd090c49b4311bcf3d8b717d6e5c65553849aed..a508ddd43ce613441eae759cd6110b6cc15819e4 100644
---- a/src/inspector_agent.h
-+++ b/src/inspector_agent.h
-@@ -6,7 +6,9 @@
- #error("This header can only be used when inspector is enabled")
- #endif
- 
-+#include "node.h"
- #include "node_options.h"
-+#include "node_platform.h"
- #include "v8.h"
- 
- #include <cstddef>
-@@ -40,7 +42,7 @@ class InspectorSessionDelegate {
-                                      = 0;
- };
- 
--class Agent {
-+class NODE_EXTERN Agent {
-  public:
-   explicit Agent(node::Environment* env);
-   ~Agent();
-diff --git a/src/inspector_io.cc b/src/inspector_io.cc
-index d3bd1911214f83fbf841a91bf01072d4c12fe870..01b92d5b6b17015eb6037978b36ab20b7d2ad651 100644
---- a/src/inspector_io.cc
-+++ b/src/inspector_io.cc
-@@ -14,6 +14,8 @@
- #include "util-inl.h"
- #include "zlib.h"
- 
-+#include "libplatform/libplatform.h"
-+
- #include <deque>
- #include <cstring>
- #include <vector>

shell/app/node_main.cc

@@ -25,7 +25,6 @@
 #include "gin/v8_initializer.h"
 #include "shell/app/uv_task_runner.h"
 #include "shell/browser/javascript_environment.h"
-#include "shell/browser/node_debugger.h"
 #include "shell/common/api/electron_bindings.h"
 #include "shell/common/gin_helper/dictionary.h"
 #include "shell/common/node_bindings.h"
@@ -202,15 +201,11 @@ int NodeMain(int argc, char* argv[]) {
       isolate_data = node::CreateIsolateData(isolate, loop, gin_env.platform());
       CHECK_NE(nullptr, isolate_data);
 
-      uint64_t flags = node::EnvironmentFlags::kDefaultFlags |
-                       node::EnvironmentFlags::kNoInitializeInspector;
-
       std::vector<std::string> args(argv, argv + argc);  // NOLINT
       std::vector<std::string> exec_args(exec_argv,
                                          exec_argv + exec_argc);  // NOLINT
       env = node::CreateEnvironment(isolate_data, gin_env.context(), args,
-                                    exec_args,
-                                    (node::EnvironmentFlags::Flags)flags);
+                                    exec_args);
       CHECK_NE(nullptr, env);
 
       node::IsolateSettings is;
@@ -247,10 +242,6 @@ int NodeMain(int argc, char* argv[]) {
       }
     }
 
-    // Enable support for v8 inspector.
-    NodeDebugger node_debugger(env);
-    node_debugger.Start();
-
     // TODO(codebytere): we should try to handle this upstream.
     {
       v8::HandleScope scope(isolate);
@@ -288,8 +279,6 @@ int NodeMain(int argc, char* argv[]) {
           node::performance::NODE_PERFORMANCE_MILESTONE_LOOP_EXIT);
     }
 
-    node_debugger.Stop();
-
     env->set_trace_sync_io(false);
 
     exit_code = node::EmitExit(env);

shell/browser/electron_browser_main_parts.cc

@@ -41,7 +41,6 @@
 #include "shell/browser/feature_list.h"
 #include "shell/browser/javascript_environment.h"
 #include "shell/browser/media/media_capture_devices_dispatcher.h"
-#include "shell/browser/node_debugger.h"
 #include "shell/browser/ui/devtools_manager_delegate.h"
 #include "shell/common/api/electron_bindings.h"
 #include "shell/common/application_info.h"
@@ -322,10 +321,6 @@ void ElectronBrowserMainParts::PostEarlyInitialization() {
       js_env_->context(), js_env_->platform());
   node_env_ = std::make_unique<NodeEnvironment>(env);
 
-  // Enable support for v8 inspector
-  node_debugger_ = std::make_unique<NodeDebugger>(env);
-  node_debugger_->Start();
-
   env->set_trace_sync_io(env->options()->trace_sync_io);
 
   // Add Electron extended APIs.
@@ -550,7 +545,6 @@ void ElectronBrowserMainParts::PostMainMessageLoopRun() {
 
   // Destroy node platform after all destructors_ are executed, as they may
   // invoke Node/V8 APIs inside them.
-  node_debugger_->Stop();
   node_env_->env()->set_trace_sync_io(false);
   js_env_->OnMessageLoopDestroying();
   node::Stop(node_env_->env());

shell/browser/electron_browser_main_parts.h

@@ -42,7 +42,6 @@ class Browser;
 class ElectronBindings;
 class JavascriptEnvironment;
 class NodeBindings;
-class NodeDebugger;
 class NodeEnvironment;
 class BridgeTaskRunner;
 
@@ -151,7 +150,6 @@ class ElectronBrowserMainParts : public content::BrowserMainParts {
   std::unique_ptr<NodeBindings> node_bindings_;
   std::unique_ptr<ElectronBindings> electron_bindings_;
   std::unique_ptr<NodeEnvironment> node_env_;
-  std::unique_ptr<NodeDebugger> node_debugger_;
   std::unique_ptr<IconManager> icon_manager_;
   std::unique_ptr<base::FieldTrialList> field_trial_list_;
 

shell/browser/node_debugger.cc

@@ -1,55 +0,0 @@
-// Copyright (c) 2014 GitHub, Inc.
-// Use of this source code is governed by the MIT license that can be
-// found in the LICENSE file.
-
-#include "shell/browser/node_debugger.h"
-
-#include <memory>
-#include <string>
-#include <vector>
-
-#include "base/command_line.h"
-#include "base/logging.h"
-#include "base/strings/string_util.h"
-#include "base/strings/utf_string_conversions.h"
-#include "libplatform/libplatform.h"
-#include "shell/common/gin_helper/dictionary.h"
-#include "shell/common/node_includes.h"
-
-namespace electron {
-
-NodeDebugger::NodeDebugger(node::Environment* env) : env_(env) {}
-
-NodeDebugger::~NodeDebugger() = default;
-
-void NodeDebugger::Start() {
-  auto* inspector = env_->inspector_agent();
-  if (inspector == nullptr)
-    return;
-
-  // DebugOptions will already have been set by ProcessGlobalArgs,
-  // so just pull the ones from there to pass to the InspectorAgent
-  const auto debug_options = env_->options()->debug_options();
-  if (inspector->Start("" /* path */, debug_options,
-                       std::make_shared<node::ExclusiveAccess<node::HostPort>>(
-                           debug_options.host_port),
-                       true /* is_main */))
-    DCHECK(inspector->IsListening());
-
-  v8::HandleScope handle_scope(env_->isolate());
-  node::profiler::StartProfilers(env_);
-
-  if (inspector->options().break_node_first_line) {
-    inspector->PauseOnNextJavascriptStatement("Break at bootstrap");
-  }
-}
-
-void NodeDebugger::Stop() {
-  auto* inspector = env_->inspector_agent();
-  if (inspector && inspector->IsListening()) {
-    inspector->WaitForDisconnect();
-    inspector->Stop();
-  }
-}
-
-}  // namespace electron

shell/browser/node_debugger.h

@@ -1,33 +0,0 @@
-// Copyright (c) 2014 GitHub, Inc.
-// Use of this source code is governed by the MIT license that can be
-// found in the LICENSE file.
-
-#ifndef SHELL_BROWSER_NODE_DEBUGGER_H_
-#define SHELL_BROWSER_NODE_DEBUGGER_H_
-
-#include "base/macros.h"
-
-namespace node {
-class Environment;
-}  // namespace node
-
-namespace electron {
-
-// Add support for node's "--inspect" switch.
-class NodeDebugger {
- public:
-  explicit NodeDebugger(node::Environment* env);
-  ~NodeDebugger();
-
-  void Start();
-  void Stop();
-
- private:
-  node::Environment* env_;
-
-  DISALLOW_COPY_AND_ASSIGN(NodeDebugger);
-};
-
-}  // namespace electron
-
-#endif  // SHELL_BROWSER_NODE_DEBUGGER_H_

shell/common/node_bindings.cc

@@ -392,13 +392,13 @@ node::Environment* NodeBindings::CreateEnvironment(
       node::CreateIsolateData(context->GetIsolate(), uv_loop_, platform);
 
   node::Environment* env;
-  uint64_t flags = node::EnvironmentFlags::kDefaultFlags |
-                   node::EnvironmentFlags::kNoInitializeInspector;
   if (browser_env_ != BrowserEnvironment::BROWSER) {
     // Only one ESM loader can be registered per isolate -
     // in renderer processes this should be blink. We need to tell Node.js
     // not to register its handler (overriding blinks) in non-browser processes.
-    flags |= node::EnvironmentFlags::kNoRegisterESMLoader;
+    uint64_t flags = node::EnvironmentFlags::kDefaultFlags |
+                     node::EnvironmentFlags::kNoRegisterESMLoader |
+                     node::EnvironmentFlags::kNoInitializeInspector;
     v8::TryCatch try_catch(context->GetIsolate());
     env = node::CreateEnvironment(isolate_data_, context, args, exec_args,
                                   (node::EnvironmentFlags::Flags)flags);
@@ -411,8 +411,7 @@ node::Environment* NodeBindings::CreateEnvironment(
                  << process_type;
     }
   } else {
-    env = node::CreateEnvironment(isolate_data_, context, args, exec_args,
-                                  (node::EnvironmentFlags::Flags)flags);
+    env = node::CreateEnvironment(isolate_data_, context, args, exec_args);
     DCHECK(env);
   }