From 16057016ad205cb36112cebf3fde19f19edf4521 Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Tue, 22 Sep 2020 20:35:18 -0700 Subject: [PATCH] chore: remove custom Node.js debugger --- filenames.gni | 2 - patches/node/.patches | 3 +- ...reventing_initializeinspector_in_env.patch | 49 +++++++------- ...fix_build_and_expose_inspector_agent.patch | 65 ------------------- shell/app/node_main.cc | 13 +--- shell/browser/electron_browser_main_parts.cc | 6 -- shell/browser/electron_browser_main_parts.h | 2 - shell/browser/node_debugger.cc | 55 ---------------- shell/browser/node_debugger.h | 33 ---------- shell/common/node_bindings.cc | 9 ++- 10 files changed, 30 insertions(+), 207 deletions(-) delete mode 100644 patches/node/fix_build_and_expose_inspector_agent.patch delete mode 100644 shell/browser/node_debugger.cc delete mode 100644 shell/browser/node_debugger.h diff --git a/filenames.gni b/filenames.gni index b4041dca57552..65029260dbd57 100644 --- a/filenames.gni +++ b/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", diff --git a/patches/node/.patches b/patches/node/.patches index 8b0efdf999947..a991c7a68be3c 100644 --- a/patches/node/.patches +++ b/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 diff --git a/patches/node/fix_allow_preventing_initializeinspector_in_env.patch b/patches/node/fix_allow_preventing_initializeinspector_in_env.patch index 39cf4428a04ef..26f10ee076d44 100644 --- a/patches/node/fix_allow_preventing_initializeinspector_in_env.patch +++ b/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 -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 bc1ab0bfcf87936b8d8156172319283704b05da8..08e0da9ba08b933036623ae288b9b1f89efeed8b 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( @@ -37,45 +36,45 @@ 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 d051b422d7c5a51a8590abda382676d3f661b532..180e876f21f55826ea280b8d0f0d5b4ee8cf87b2 100644 --- a/src/node.h +++ b/src/node.h -@@ -424,7 +424,11 @@ enum Flags : uint64_t { +@@ -423,7 +423,11 @@ enum Flags : uint64_t { kNoRegisterESMLoader = 1 << 3, // Set this flag to make Node.js track "raw" file descriptors, i.e. managed // 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 diff --git a/patches/node/fix_build_and_expose_inspector_agent.patch b/patches/node/fix_build_and_expose_inspector_agent.patch deleted file mode 100644 index 01951bf382656..0000000000000 --- a/patches/node/fix_build_and_expose_inspector_agent.patch +++ /dev/null @@ -1,65 +0,0 @@ -From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 -From: Shelley Vohr -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 -@@ -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 - #include - #include diff --git a/shell/app/node_main.cc b/shell/app/node_main.cc index 33097d78be2f9..c133048e9cb9b 100644 --- a/shell/app/node_main.cc +++ b/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 args(argv, argv + argc); // NOLINT std::vector 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); diff --git a/shell/browser/electron_browser_main_parts.cc b/shell/browser/electron_browser_main_parts.cc index d27d3ad0f6d25..5b255a021faf9 100644 --- a/shell/browser/electron_browser_main_parts.cc +++ b/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(env); - // Enable support for v8 inspector - node_debugger_ = std::make_unique(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()); diff --git a/shell/browser/electron_browser_main_parts.h b/shell/browser/electron_browser_main_parts.h index c1fc16a709bf0..56733af33f60e 100644 --- a/shell/browser/electron_browser_main_parts.h +++ b/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 node_bindings_; std::unique_ptr electron_bindings_; std::unique_ptr node_env_; - std::unique_ptr node_debugger_; std::unique_ptr icon_manager_; std::unique_ptr field_trial_list_; diff --git a/shell/browser/node_debugger.cc b/shell/browser/node_debugger.cc deleted file mode 100644 index 538c95ab7b655..0000000000000 --- a/shell/browser/node_debugger.cc +++ /dev/null @@ -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 -#include -#include - -#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>( - 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 diff --git a/shell/browser/node_debugger.h b/shell/browser/node_debugger.h deleted file mode 100644 index e3a702d10e214..0000000000000 --- a/shell/browser/node_debugger.h +++ /dev/null @@ -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_ diff --git a/shell/common/node_bindings.cc b/shell/common/node_bindings.cc index fc2fd57bf9292..44d8d5f005821 100644 --- a/shell/common/node_bindings.cc +++ b/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); }