refactor: node::Environment self-cleanup (#39628)

* chore: savepoint

Co-authored-by: Charles Kerr <[email protected]>

* chore: turn raw_ptr tests back off

Co-authored-by: Charles Kerr <[email protected]>

---------

Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com>
Co-authored-by: Charles Kerr <[email protected]>

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<NodeEnvironment>(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);

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<JavascriptEnvironment> js_env_;
 
   // depends-on: js_env_'s isolate
-  std::unique_ptr<NodeEnvironment> node_env_;
+  std::shared_ptr<node::Environment> node_env_;
 
   // depends-on: js_env_'s isolate
   std::unique_ptr<Browser> browser_;

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

shell/browser/javascript_environment.h

@@ -54,22 +54,6 @@ class JavascriptEnvironment {
   std::unique_ptr<MicrotasksRunner> 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<node::Environment> env_;
-};
-
 }  // namespace electron
 
 #endif  // ELECTRON_SHELL_BROWSER_JAVASCRIPT_ENVIRONMENT_H_

shell/common/node_bindings.cc

@@ -477,7 +477,7 @@ void NodeBindings::Initialize(v8::Local<v8::Context> context) {
   g_is_initialized = true;
 }
 
-node::Environment* NodeBindings::CreateEnvironment(
+std::shared_ptr<node::Environment> NodeBindings::CreateEnvironment(
     v8::Handle<v8::Context> context,
     node::MultiIsolatePlatform* platform,
     std::vector<std::string> 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<v8::Context>{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<node::Environment> NodeBindings::CreateEnvironment(
     v8::Handle<v8::Context> context,
     node::MultiIsolatePlatform* platform) {
 #if BUILDFLAG(IS_WIN)

shell/common/node_bindings.h

@@ -5,6 +5,7 @@
 #ifndef ELECTRON_SHELL_COMMON_NODE_BINDINGS_H_
 #define ELECTRON_SHELL_COMMON_NODE_BINDINGS_H_
 
+#include <memory>
 #include <string>
 #include <type_traits>
 #include <vector>
@@ -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<int>(gin::kPerContextDataStartIndex) +
-    static_cast<int>(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<std::string> ParseNodeCliFlags();
 
   // Create the environment and load node.js.
-  node::Environment* CreateEnvironment(v8::Handle<v8::Context> context,
-                                       node::MultiIsolatePlatform* platform,
-                                       std::vector<std::string> args,
-                                       std::vector<std::string> exec_args);
-  node::Environment* CreateEnvironment(v8::Handle<v8::Context> context,
-                                       node::MultiIsolatePlatform* platform);
+  std::shared_ptr<node::Environment> CreateEnvironment(
+      v8::Handle<v8::Context> context,
+      node::MultiIsolatePlatform* platform,
+      std::vector<std::string> args,
+      std::vector<std::string> exec_args);
+
+  std::shared_ptr<node::Environment> CreateEnvironment(
+      v8::Handle<v8::Context> 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<v8::Context> context) {
-    context->SetAlignedPointerInEmbedderData(kElectronContextEmbedderDataIndex,
-                                             nullptr);
-  }
-
   node::IsolateData* isolate_data(v8::Local<v8::Context> context) const {
     if (context->GetNumberOfEmbedderDataFields() <=
         kElectronContextEmbedderDataIndex) {
@@ -167,6 +159,12 @@ class NodeBindings {
   raw_ptr<uv_loop_t> 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<int>(gin::kPerContextDataStartIndex) +
+      static_cast<int>(gin::kEmbedderElectron);
+
   // Thread to poll uv events.
   static void EmbedThreadRunner(void* arg);
 

shell/renderer/electron_renderer_client.cc

@@ -88,7 +88,7 @@ void ElectronRendererClient::DidCreateScriptContext(
   v8::Maybe<bool> initialized = node::InitializeContext(renderer_context);
   CHECK(!initialized.IsNothing() && initialized.FromJust());
 
-  node::Environment* env =
+  std::shared_ptr<node::Environment> 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

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<node::Environment*> environments_;
+  std::set<std::shared_ptr<node::Environment>> environments_;
 
   // Getting main script context from web frame would lazily initializes
   // its script context. Doing so in a web page without scripts would trigger

shell/renderer/web_worker_observer.cc

@@ -6,7 +6,9 @@
 
 #include <utility>
 
+#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<bool> initialized = node::InitializeContext(worker_context);
   CHECK(!initialized.IsNothing() && initialized.FromJust());
-  node::Environment* env =
+  std::shared_ptr<node::Environment> 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<v8::Context> context) {
@@ -91,9 +96,8 @@ void WebWorkerObserver::ContextWillDestroy(v8::Local<v8::Context> 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);
 

shell/renderer/web_worker_observer.h

@@ -6,9 +6,16 @@
 #define ELECTRON_SHELL_RENDERER_WEB_WORKER_OBSERVER_H_
 
 #include <memory>
+#include <set>
 
 #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<NodeBindings> node_bindings_;
   std::unique_ptr<ElectronBindings> electron_bindings_;
+  std::set<std::shared_ptr<node::Environment>> environments_;
 };
 
 }  // namespace electron

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<NodeEnvironment>(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();

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<JavascriptEnvironment> js_env_;
   std::unique_ptr<NodeBindings> node_bindings_;
   std::unique_ptr<ElectronBindings> electron_bindings_;
-  std::unique_ptr<NodeEnvironment> node_env_;
+  std::shared_ptr<node::Environment> node_env_;
   mojo::Receiver<node::mojom::NodeService> receiver_{this};
 };