From 35969939a1fe795d21c9e11ab2bd08b596b9fe64 Mon Sep 17 00:00:00 2001 From: Charles Kerr Date: Wed, 23 Aug 2023 08:56:16 -0500 Subject: [PATCH] refactor: node::Environment self-cleanup (#39604) * chore: savepoint * chore: turn raw_ptr tests back off --- shell/browser/electron_browser_main_parts.cc | 17 +++++----- shell/browser/electron_browser_main_parts.h | 7 ++-- shell/browser/javascript_environment.cc | 8 ----- shell/browser/javascript_environment.h | 16 --------- shell/common/node_bindings.cc | 21 ++++++++++-- shell/common/node_bindings.h | 34 +++++++++----------- shell/renderer/electron_renderer_client.cc | 20 +++++++----- shell/renderer/electron_renderer_client.h | 2 +- shell/renderer/web_worker_observer.cc | 16 +++++---- shell/renderer/web_worker_observer.h | 8 +++++ shell/services/node/node_service.cc | 20 ++++++------ shell/services/node/node_service.h | 9 ++++-- 12 files changed, 95 insertions(+), 83 deletions(-) diff --git a/shell/browser/electron_browser_main_parts.cc b/shell/browser/electron_browser_main_parts.cc index 61c0b3df0106..1bfdb82e440a 100644 --- a/shell/browser/electron_browser_main_parts.cc +++ b/shell/browser/electron_browser_main_parts.cc @@ -266,26 +266,25 @@ void ElectronBrowserMainParts::PostEarlyInitialization() { node_bindings_->Initialize(js_env_->isolate()->GetCurrentContext()); // Create the global environment. - node::Environment* env = node_bindings_->CreateEnvironment( + node_env_ = node_bindings_->CreateEnvironment( js_env_->isolate()->GetCurrentContext(), js_env_->platform()); - node_env_ = std::make_unique(env); - env->set_trace_sync_io(env->options()->trace_sync_io); + node_env_->set_trace_sync_io(node_env_->options()->trace_sync_io); // We do not want to crash the main process on unhandled rejections. - env->options()->unhandled_rejections = "warn-with-error-code"; + node_env_->options()->unhandled_rejections = "warn-with-error-code"; // Add Electron extended APIs. - electron_bindings_->BindTo(js_env_->isolate(), env->process_object()); + electron_bindings_->BindTo(js_env_->isolate(), node_env_->process_object()); // Create explicit microtasks runner. js_env_->CreateMicrotasksRunner(); // Wrap the uv loop with global env. - node_bindings_->set_uv_env(env); + node_bindings_->set_uv_env(node_env_.get()); // Load everything. - node_bindings_->LoadEnvironment(env); + node_bindings_->LoadEnvironment(node_env_.get()); // We already initialized the feature list in PreEarlyInitialization(), but // the user JS script would not have had a chance to alter the command-line @@ -627,9 +626,9 @@ void ElectronBrowserMainParts::PostMainMessageLoopRun() { // Destroy node platform after all destructors_ are executed, as they may // invoke Node/V8 APIs inside them. - node_env_->env()->set_trace_sync_io(false); + node_env_->set_trace_sync_io(false); js_env_->DestroyMicrotasksRunner(); - node::Stop(node_env_->env(), node::StopFlags::kDoNotTerminateIsolate); + node::Stop(node_env_.get(), node::StopFlags::kDoNotTerminateIsolate); node_env_.reset(); auto default_context_key = ElectronBrowserContext::PartitionKey("", false); diff --git a/shell/browser/electron_browser_main_parts.h b/shell/browser/electron_browser_main_parts.h index 69f4c32cd967..0e49e8a66d20 100644 --- a/shell/browser/electron_browser_main_parts.h +++ b/shell/browser/electron_browser_main_parts.h @@ -37,6 +37,10 @@ class Screen; } #endif +namespace node { +class Environment; +} + namespace ui { class LinuxUiGetter; } @@ -47,7 +51,6 @@ class Browser; class ElectronBindings; class JavascriptEnvironment; class NodeBindings; -class NodeEnvironment; #if BUILDFLAG(ENABLE_ELECTRON_EXTENSIONS) class ElectronExtensionsClient; @@ -166,7 +169,7 @@ class ElectronBrowserMainParts : public content::BrowserMainParts { std::unique_ptr js_env_; // depends-on: js_env_'s isolate - std::unique_ptr node_env_; + std::shared_ptr node_env_; // depends-on: js_env_'s isolate std::unique_ptr browser_; diff --git a/shell/browser/javascript_environment.cc b/shell/browser/javascript_environment.cc index c176a589c721..98c6e12cac49 100644 --- a/shell/browser/javascript_environment.cc +++ b/shell/browser/javascript_environment.cc @@ -340,12 +340,4 @@ void JavascriptEnvironment::DestroyMicrotasksRunner() { base::CurrentThread::Get()->RemoveTaskObserver(microtasks_runner_.get()); } -NodeEnvironment::NodeEnvironment(node::Environment* env) : env_(env) {} - -NodeEnvironment::~NodeEnvironment() { - auto* isolate_data = env_->isolate_data(); - node::FreeEnvironment(env_); - node::FreeIsolateData(isolate_data); -} - } // namespace electron diff --git a/shell/browser/javascript_environment.h b/shell/browser/javascript_environment.h index bb38e3594592..937b0f976415 100644 --- a/shell/browser/javascript_environment.h +++ b/shell/browser/javascript_environment.h @@ -54,22 +54,6 @@ class JavascriptEnvironment { std::unique_ptr microtasks_runner_; }; -// Manage the Node Environment automatically. -class NodeEnvironment { - public: - explicit NodeEnvironment(node::Environment* env); - ~NodeEnvironment(); - - // disable copy - NodeEnvironment(const NodeEnvironment&) = delete; - NodeEnvironment& operator=(const NodeEnvironment&) = delete; - - node::Environment* env() { return env_; } - - private: - raw_ptr env_; -}; - } // namespace electron #endif // ELECTRON_SHELL_BROWSER_JAVASCRIPT_ENVIRONMENT_H_ diff --git a/shell/common/node_bindings.cc b/shell/common/node_bindings.cc index 4790b00e535d..afa86048f333 100644 --- a/shell/common/node_bindings.cc +++ b/shell/common/node_bindings.cc @@ -477,7 +477,7 @@ void NodeBindings::Initialize(v8::Local context) { g_is_initialized = true; } -node::Environment* NodeBindings::CreateEnvironment( +std::shared_ptr NodeBindings::CreateEnvironment( v8::Handle context, node::MultiIsolatePlatform* platform, std::vector args, @@ -644,10 +644,25 @@ node::Environment* NodeBindings::CreateEnvironment( base::PathService::Get(content::CHILD_PROCESS_EXE, &helper_exec_path); process.Set("helperExecPath", helper_exec_path); - return env; + auto env_deleter = [isolate, isolate_data, + context = v8::Global{isolate, context}]( + node::Environment* nenv) mutable { + // When `isolate_data` was created above, a pointer to it was kept + // in context's embedder_data[kElectronContextEmbedderDataIndex]. + // Since we're about to free `isolate_data`, clear that entry + v8::HandleScope handle_scope{isolate}; + context.Get(isolate)->SetAlignedPointerInEmbedderData( + kElectronContextEmbedderDataIndex, nullptr); + context.Reset(); + + node::FreeEnvironment(nenv); + node::FreeIsolateData(isolate_data); + }; + + return {env, std::move(env_deleter)}; } -node::Environment* NodeBindings::CreateEnvironment( +std::shared_ptr NodeBindings::CreateEnvironment( v8::Handle context, node::MultiIsolatePlatform* platform) { #if BUILDFLAG(IS_WIN) diff --git a/shell/common/node_bindings.h b/shell/common/node_bindings.h index 9182f5440b2a..c6e2a56f3810 100644 --- a/shell/common/node_bindings.h +++ b/shell/common/node_bindings.h @@ -5,6 +5,7 @@ #ifndef ELECTRON_SHELL_COMMON_NODE_BINDINGS_H_ #define ELECTRON_SHELL_COMMON_NODE_BINDINGS_H_ +#include #include #include #include @@ -25,12 +26,6 @@ class SingleThreadTaskRunner; namespace electron { -// Choose a reasonable unique index that's higher than any Blink uses -// and thus unlikely to collide with an existing index. -static constexpr int kElectronContextEmbedderDataIndex = - static_cast(gin::kPerContextDataStartIndex) + - static_cast(gin::kEmbedderElectron); - // A helper class to manage uv_handle_t types, e.g. uv_async_t. // // As per the uv docs: "uv_close() MUST be called on each handle before @@ -95,12 +90,15 @@ class NodeBindings { std::vector ParseNodeCliFlags(); // Create the environment and load node.js. - node::Environment* CreateEnvironment(v8::Handle context, - node::MultiIsolatePlatform* platform, - std::vector args, - std::vector exec_args); - node::Environment* CreateEnvironment(v8::Handle context, - node::MultiIsolatePlatform* platform); + std::shared_ptr CreateEnvironment( + v8::Handle context, + node::MultiIsolatePlatform* platform, + std::vector args, + std::vector exec_args); + + std::shared_ptr CreateEnvironment( + v8::Handle context, + node::MultiIsolatePlatform* platform); // Load node.js in the environment. void LoadEnvironment(node::Environment* env); @@ -111,12 +109,6 @@ class NodeBindings { // Notify embed thread to start polling after environment is loaded. void StartPolling(); - // Clears the PerIsolateData. - void clear_isolate_data(v8::Local context) { - context->SetAlignedPointerInEmbedderData(kElectronContextEmbedderDataIndex, - nullptr); - } - node::IsolateData* isolate_data(v8::Local context) const { if (context->GetNumberOfEmbedderDataFields() <= kElectronContextEmbedderDataIndex) { @@ -167,6 +159,12 @@ class NodeBindings { raw_ptr uv_loop_; private: + // Choose a reasonable unique index that's higher than any Blink uses + // and thus unlikely to collide with an existing index. + static constexpr int kElectronContextEmbedderDataIndex = + static_cast(gin::kPerContextDataStartIndex) + + static_cast(gin::kEmbedderElectron); + // Thread to poll uv events. static void EmbedThreadRunner(void* arg); diff --git a/shell/renderer/electron_renderer_client.cc b/shell/renderer/electron_renderer_client.cc index 04b9b0e03bb9..d33527ff847c 100644 --- a/shell/renderer/electron_renderer_client.cc +++ b/shell/renderer/electron_renderer_client.cc @@ -88,7 +88,7 @@ void ElectronRendererClient::DidCreateScriptContext( v8::Maybe initialized = node::InitializeContext(renderer_context); CHECK(!initialized.IsNothing() && initialized.FromJust()); - node::Environment* env = + std::shared_ptr env = node_bindings_->CreateEnvironment(renderer_context, nullptr); // If we have disabled the site instance overrides we should prevent loading @@ -106,11 +106,11 @@ void ElectronRendererClient::DidCreateScriptContext( BindProcess(env->isolate(), &process_dict, render_frame); // Load everything. - node_bindings_->LoadEnvironment(env); + node_bindings_->LoadEnvironment(env.get()); if (node_bindings_->uv_env() == nullptr) { // Make uv loop being wrapped by window context. - node_bindings_->set_uv_env(env); + node_bindings_->set_uv_env(env.get()); // Give the node loop a run to make sure everything is ready. node_bindings_->StartPolling(); @@ -124,7 +124,9 @@ void ElectronRendererClient::WillReleaseScriptContext( return; node::Environment* env = node::Environment::GetCurrent(context); - if (environments_.erase(env) == 0) + const auto iter = base::ranges::find_if( + environments_, [env](auto& item) { return env == item.get(); }); + if (iter == environments_.end()) return; gin_helper::EmitEvent(env->isolate(), env->process_object(), "exit"); @@ -143,9 +145,7 @@ void ElectronRendererClient::WillReleaseScriptContext( DCHECK_EQ(microtask_queue->GetMicrotasksScopeDepth(), 0); microtask_queue->set_microtasks_policy(v8::MicrotasksPolicy::kExplicit); - node::FreeEnvironment(env); - node::FreeIsolateData(node_bindings_->isolate_data(context)); - node_bindings_->clear_isolate_data(context); + environments_.erase(iter); microtask_queue->set_microtasks_policy(old_policy); @@ -201,7 +201,11 @@ node::Environment* ElectronRendererClient::GetEnvironment( auto context = GetContext(render_frame->GetWebFrame(), v8::Isolate::GetCurrent()); node::Environment* env = node::Environment::GetCurrent(context); - return base::Contains(environments_, env) ? env : nullptr; + + return base::Contains(environments_, env, + [](auto const& item) { return item.get(); }) + ? env + : nullptr; } } // namespace electron diff --git a/shell/renderer/electron_renderer_client.h b/shell/renderer/electron_renderer_client.h index c93d51e75938..1e8318c4caeb 100644 --- a/shell/renderer/electron_renderer_client.h +++ b/shell/renderer/electron_renderer_client.h @@ -55,7 +55,7 @@ class ElectronRendererClient : public RendererClientBase { // The node::Environment::GetCurrent API does not return nullptr when it // is called for a context without node::Environment, so we have to keep // a book of the environments created. - std::set environments_; + std::set> environments_; // Getting main script context from web frame would lazily initializes // its script context. Doing so in a web page without scripts would trigger diff --git a/shell/renderer/web_worker_observer.cc b/shell/renderer/web_worker_observer.cc index d738a10acfa8..ff2f9bbda888 100644 --- a/shell/renderer/web_worker_observer.cc +++ b/shell/renderer/web_worker_observer.cc @@ -6,7 +6,9 @@ #include +#include "base/containers/cxx20_erase_set.h" #include "base/no_destructor.h" +#include "base/ranges/algorithm.h" #include "base/threading/thread_local.h" #include "shell/common/api/electron_bindings.h" #include "shell/common/gin_helper/event_emitter_caller.h" @@ -61,20 +63,23 @@ void WebWorkerObserver::WorkerScriptReadyForEvaluation( // Setup node environment for each window. v8::Maybe initialized = node::InitializeContext(worker_context); CHECK(!initialized.IsNothing() && initialized.FromJust()); - node::Environment* env = + std::shared_ptr env = node_bindings_->CreateEnvironment(worker_context, nullptr); // Add Electron extended APIs. electron_bindings_->BindTo(env->isolate(), env->process_object()); // Load everything. - node_bindings_->LoadEnvironment(env); + node_bindings_->LoadEnvironment(env.get()); // Make uv loop being wrapped by window context. - node_bindings_->set_uv_env(env); + node_bindings_->set_uv_env(env.get()); // Give the node loop a run to make sure everything is ready. node_bindings_->StartPolling(); + + // Keep the environment alive until we free it in ContextWillDestroy() + environments_.insert(std::move(env)); } void WebWorkerObserver::ContextWillDestroy(v8::Local context) { @@ -91,9 +96,8 @@ void WebWorkerObserver::ContextWillDestroy(v8::Local context) { DCHECK_EQ(microtask_queue->GetMicrotasksScopeDepth(), 0); microtask_queue->set_microtasks_policy(v8::MicrotasksPolicy::kExplicit); - node::FreeEnvironment(env); - node::FreeIsolateData(node_bindings_->isolate_data(context)); - node_bindings_->clear_isolate_data(context); + base::EraseIf(environments_, + [env](auto const& item) { return item.get() == env; }); microtask_queue->set_microtasks_policy(old_policy); diff --git a/shell/renderer/web_worker_observer.h b/shell/renderer/web_worker_observer.h index bba7ae7e619e..897052a3c62d 100644 --- a/shell/renderer/web_worker_observer.h +++ b/shell/renderer/web_worker_observer.h @@ -6,9 +6,16 @@ #define ELECTRON_SHELL_RENDERER_WEB_WORKER_OBSERVER_H_ #include +#include #include "v8/include/v8.h" +namespace node { + +class Environment; + +} // namespace node + namespace electron { class ElectronBindings; @@ -35,6 +42,7 @@ class WebWorkerObserver { private: std::unique_ptr node_bindings_; std::unique_ptr electron_bindings_; + std::set> environments_; }; } // namespace electron diff --git a/shell/services/node/node_service.cc b/shell/services/node/node_service.cc index 8aa7a4a61882..ba86b8d27aca 100644 --- a/shell/services/node/node_service.cc +++ b/shell/services/node/node_service.cc @@ -30,9 +30,9 @@ NodeService::NodeService( NodeService::~NodeService() { if (!node_env_stopped_) { - node_env_->env()->set_trace_sync_io(false); + node_env_->set_trace_sync_io(false); js_env_->DestroyMicrotasksRunner(); - node::Stop(node_env_->env(), node::StopFlags::kDoNotTerminateIsolate); + node::Stop(node_env_.get(), node::StopFlags::kDoNotTerminateIsolate); } } @@ -57,13 +57,12 @@ void NodeService::Initialize(node::mojom::NodeServiceParamsPtr params) { #endif // Create the global environment. - node::Environment* env = node_bindings_->CreateEnvironment( + node_env_ = node_bindings_->CreateEnvironment( js_env_->isolate()->GetCurrentContext(), js_env_->platform(), params->args, params->exec_args); - node_env_ = std::make_unique(env); node::SetProcessExitHandler( - env, [this](node::Environment* env, int exit_code) { + node_env_.get(), [this](node::Environment* env, int exit_code) { // Destroy node platform. env->set_trace_sync_io(false); js_env_->DestroyMicrotasksRunner(); @@ -72,20 +71,21 @@ void NodeService::Initialize(node::mojom::NodeServiceParamsPtr params) { receiver_.ResetWithReason(exit_code, ""); }); - env->set_trace_sync_io(env->options()->trace_sync_io); + node_env_->set_trace_sync_io(node_env_->options()->trace_sync_io); // Add Electron extended APIs. - electron_bindings_->BindTo(env->isolate(), env->process_object()); + electron_bindings_->BindTo(node_env_->isolate(), node_env_->process_object()); // Add entry script to process object. - gin_helper::Dictionary process(env->isolate(), env->process_object()); + gin_helper::Dictionary process(node_env_->isolate(), + node_env_->process_object()); process.SetHidden("_serviceStartupScript", params->script); // Setup microtask runner. js_env_->CreateMicrotasksRunner(); // Wrap the uv loop with global env. - node_bindings_->set_uv_env(env); + node_bindings_->set_uv_env(node_env_.get()); // LoadEnvironment should be called after setting up // JavaScriptEnvironment including the microtask runner @@ -94,7 +94,7 @@ void NodeService::Initialize(node::mojom::NodeServiceParamsPtr params) { // the exit handler set above will be triggered and it expects // both Node Env and JavaScriptEnviroment are setup to perform // a clean shutdown of this process. - node_bindings_->LoadEnvironment(env); + node_bindings_->LoadEnvironment(node_env_.get()); // Run entry script. node_bindings_->PrepareEmbedThread(); diff --git a/shell/services/node/node_service.h b/shell/services/node/node_service.h index 96839775958a..ba5e95caf656 100644 --- a/shell/services/node/node_service.h +++ b/shell/services/node/node_service.h @@ -11,12 +11,17 @@ #include "mojo/public/cpp/bindings/receiver.h" #include "shell/services/node/public/mojom/node_service.mojom.h" +namespace node { + +class Environment; + +} // namespace node + namespace electron { class ElectronBindings; class JavascriptEnvironment; class NodeBindings; -class NodeEnvironment; class NodeService : public node::mojom::NodeService { public: @@ -35,7 +40,7 @@ class NodeService : public node::mojom::NodeService { std::unique_ptr js_env_; std::unique_ptr node_bindings_; std::unique_ptr electron_bindings_; - std::unique_ptr node_env_; + std::shared_ptr node_env_; mojo::Receiver receiver_{this}; };