fix: window.open causing occasional Node.js crashes

shell/common/node_bindings.cc

@@ -518,8 +518,8 @@ node::Environment* NodeBindings::CreateEnvironment(
 
   args.insert(args.begin() + 1, init_script);
 
-  if (!isolate_data_)
-    isolate_data_ = node::CreateIsolateData(isolate, uv_loop_, platform);
+  if (!isolate_data())
+    set_isolate_data(node::CreateIsolateData(isolate, uv_loop_, platform));
 
   node::Environment* env;
   uint64_t flags = node::EnvironmentFlags::kDefaultFlags |

shell/common/node_bindings.h

@@ -108,10 +108,10 @@ class NodeBindings {
   // Notify embed thread to start polling after environment is loaded.
   void StartPolling();
 
-  // Gets/sets the per isolate data.
   void set_isolate_data(node::IsolateData* isolate_data) {
     isolate_data_ = isolate_data;
   }
+
   node::IsolateData* isolate_data() const { return isolate_data_; }
 
   // Gets/sets the environment to wrap uv loop.
@@ -172,7 +172,7 @@ class NodeBindings {
   // Semaphore to wait for main loop in the embed thread.
   uv_sem_t embed_sem_;
 
-  // Environment that to wrap the uv loop.
+  // Environment that wraps the uv loop.
   raw_ptr<node::Environment> uv_env_ = nullptr;
 
   // Isolate data used in creating the environment

shell/renderer/electron_renderer_client.cc

@@ -18,6 +18,7 @@
 #include "shell/renderer/electron_render_frame_observer.h"
 #include "shell/renderer/web_worker_observer.h"
 #include "third_party/blink/public/common/web_preferences/web_preferences.h"
+#include "third_party/blink/public/web/blink.h"
 #include "third_party/blink/public/web/web_document.h"
 #include "third_party/blink/public/web/web_local_frame.h"
 #include "third_party/blink/renderer/core/execution_context/execution_context.h"  // nogncheck
@@ -30,7 +31,30 @@ ElectronRendererClient::ElectronRendererClient()
       electron_bindings_(
           std::make_unique<ElectronBindings>(node_bindings_->uv_loop())) {}
 
-ElectronRendererClient::~ElectronRendererClient() = default;
+ElectronRendererClient::~ElectronRendererClient() {
+  if (!env_)
+    return;
+
+  // Destroying the node environment will also run the uv loop,
+  // Node.js expects `kExplicit` microtasks policy and will run microtasks
+  // checkpoints after every call into JavaScript. Since we use a different
+  // policy in the renderer - switch to `kExplicit` and then drop back to the
+  // previous policy value.
+  v8::Local<v8::Context> context = env_->context();
+  v8::MicrotaskQueue* microtask_queue = context->GetMicrotaskQueue();
+  auto old_policy = microtask_queue->microtasks_policy();
+  DCHECK_EQ(microtask_queue->GetMicrotasksScopeDepth(), 0);
+  microtask_queue->set_microtasks_policy(v8::MicrotasksPolicy::kExplicit);
+
+  node::FreeEnvironment(env_);
+  node::FreeIsolateData(node_bindings_->isolate_data());
+  node_bindings_->set_isolate_data(nullptr);
+
+  microtask_queue->set_microtasks_policy(old_policy);
+
+  // ElectronBindings is tracking node environments.
+  electron_bindings_->EnvironmentDestroyed(env_);
+}
 
 void ElectronRendererClient::RenderFrameCreated(
     content::RenderFrame* render_frame) {
@@ -41,117 +65,95 @@ void ElectronRendererClient::RenderFrameCreated(
 void ElectronRendererClient::RunScriptsAtDocumentStart(
     content::RenderFrame* render_frame) {
   RendererClientBase::RunScriptsAtDocumentStart(render_frame);
+
   // Inform the document start phase.
-  v8::HandleScope handle_scope(v8::Isolate::GetCurrent());
-  node::Environment* env = GetEnvironment(render_frame);
+  auto* isolate = v8::Isolate::GetCurrent();
+  v8::HandleScope handle_scope(isolate);
+  node::Environment* env = GetEnvironment(render_frame, isolate);
   if (env)
-    gin_helper::EmitEvent(env->isolate(), env->process_object(),
-                          "document-start");
+    gin_helper::EmitEvent(isolate, env->process_object(), "document-start");
 }
 
 void ElectronRendererClient::RunScriptsAtDocumentEnd(
     content::RenderFrame* render_frame) {
   RendererClientBase::RunScriptsAtDocumentEnd(render_frame);
+
   // Inform the document end phase.
-  v8::HandleScope handle_scope(v8::Isolate::GetCurrent());
-  node::Environment* env = GetEnvironment(render_frame);
+  auto* isolate = v8::Isolate::GetCurrent();
+  v8::HandleScope handle_scope(isolate);
+  node::Environment* env = GetEnvironment(render_frame, isolate);
   if (env)
-    gin_helper::EmitEvent(env->isolate(), env->process_object(),
-                          "document-end");
+    gin_helper::EmitEvent(isolate, env->process_object(), "document-end");
 }
 
 void ElectronRendererClient::DidCreateScriptContext(
-    v8::Handle<v8::Context> renderer_context,
+    v8::Handle<v8::Context> context,
     content::RenderFrame* render_frame) {
   // TODO(zcbenz): Do not create Node environment if node integration is not
   // enabled.
 
   // Only load Node.js if we are a main frame or a devtools extension
   // unless Node.js support has been explicitly enabled for subframes.
-  if (!ShouldLoadPreload(renderer_context, render_frame))
+  if (!ShouldLoadPreload(context, render_frame))
     return;
 
   injected_frames_.insert(render_frame);
 
   if (!node_integration_initialized_) {
     node_integration_initialized_ = true;
-    node_bindings_->Initialize(renderer_context);
+    node_bindings_->Initialize(context);
     node_bindings_->PrepareEmbedThread();
   }
 
   // Setup node tracing controller.
   if (!node::tracing::TraceEventHelper::GetAgent())
     node::tracing::TraceEventHelper::SetAgent(node::CreateAgent());
 
-  // Setup node environment for each window.
-  v8::Maybe<bool> initialized = node::InitializeContext(renderer_context);
+  v8::Maybe<bool> initialized = node::InitializeContext(context);
   CHECK(!initialized.IsNothing() && initialized.FromJust());
 
-  node::Environment* env =
-      node_bindings_->CreateEnvironment(renderer_context, nullptr);
+  // If DidCreateScriptContext is called and we've already created a Node.js
+  // Environment, then we're in the same process with a new V8::Context. We
+  // should assign the existing Environment to the new V8::Context.
+  if (env_) {
+    env_->AssignToContext(context, nullptr, node::ContextInfo(""));
+    return;
+  }
+
+  env_ = node_bindings_->CreateEnvironment(context, nullptr);
 
   // If we have disabled the site instance overrides we should prevent loading
   // any non-context aware native module.
-  env->options()->force_context_aware = true;
+  env_->options()->force_context_aware = true;
 
   // We do not want to crash the renderer process on unhandled rejections.
-  env->options()->unhandled_rejections = "warn-with-error-code";
-
-  environments_.insert(env);
+  env_->options()->unhandled_rejections = "warn-with-error-code";
 
   // Add Electron extended APIs.
-  electron_bindings_->BindTo(env->isolate(), env->process_object());
-  gin_helper::Dictionary process_dict(env->isolate(), env->process_object());
-  BindProcess(env->isolate(), &process_dict, render_frame);
+  electron_bindings_->BindTo(env_->isolate(), env_->process_object());
+  gin_helper::Dictionary process_dict(env_->isolate(), env_->process_object());
+  BindProcess(env_->isolate(), &process_dict, render_frame);
 
-  // Load everything.
-  node_bindings_->LoadEnvironment(env);
+  // Load the Environment.
+  node_bindings_->LoadEnvironment(env_);
 
-  if (node_bindings_->uv_env() == nullptr) {
-    // Make uv loop being wrapped by window context.
-    node_bindings_->set_uv_env(env);
+  // Make uv loop being wrapped by window context.
+  node_bindings_->set_uv_env(env_);
 
-    // Give the node loop a run to make sure everything is ready.
-    node_bindings_->StartPolling();
-  }
+  // Give the node loop a run to make sure everything is ready.
+  node_bindings_->StartPolling();
 }
 
 void ElectronRendererClient::WillReleaseScriptContext(
     v8::Handle<v8::Context> context,
     content::RenderFrame* render_frame) {
-  if (injected_frames_.erase(render_frame) == 0)
+  auto* env = node::Environment::GetCurrent(context);
+  if (injected_frames_.erase(render_frame) == 0 || !env)
     return;
 
-  node::Environment* env = node::Environment::GetCurrent(context);
-  if (environments_.erase(env) == 0)
-    return;
-
-  gin_helper::EmitEvent(env->isolate(), env->process_object(), "exit");
-
-  // The main frame may be replaced.
-  if (env == node_bindings_->uv_env())
-    node_bindings_->set_uv_env(nullptr);
-
-  // Destroying the node environment will also run the uv loop,
-  // Node.js expects `kExplicit` microtasks policy and will run microtasks
-  // checkpoints after every call into JavaScript. Since we use a different
-  // policy in the renderer - switch to `kExplicit` and then drop back to the
-  // previous policy value.
-  v8::MicrotaskQueue* microtask_queue = context->GetMicrotaskQueue();
-  auto old_policy = microtask_queue->microtasks_policy();
-  DCHECK_EQ(microtask_queue->GetMicrotasksScopeDepth(), 0);
-  microtask_queue->set_microtasks_policy(v8::MicrotasksPolicy::kExplicit);
-
-  node::FreeEnvironment(env);
-  if (node_bindings_->uv_env() == nullptr) {
-    node::FreeIsolateData(node_bindings_->isolate_data());
-    node_bindings_->set_isolate_data(nullptr);
-  }
-
-  microtask_queue->set_microtasks_policy(old_policy);
+  gin_helper::EmitEvent(env_->isolate(), env_->process_object(), "exit");
 
-  // ElectronBindings is tracking node environments.
-  electron_bindings_->EnvironmentDestroyed(env);
+  env->UntrackContext(context);
 }
 
 void ElectronRendererClient::WorkerScriptReadyForEvaluationOnWorkerThread(
@@ -193,14 +195,13 @@ void ElectronRendererClient::WillDestroyWorkerContextOnWorkerThread(
 }
 
 node::Environment* ElectronRendererClient::GetEnvironment(
-    content::RenderFrame* render_frame) const {
+    content::RenderFrame* render_frame,
+    v8::Isolate* isolate) const {
   if (!base::Contains(injected_frames_, render_frame))
     return nullptr;
-  v8::HandleScope handle_scope(v8::Isolate::GetCurrent());
-  auto context =
-      GetContext(render_frame->GetWebFrame(), v8::Isolate::GetCurrent());
-  node::Environment* env = node::Environment::GetCurrent(context);
-  return base::Contains(environments_, env) ? env : nullptr;
+
+  auto* env = node::Environment::GetCurrent(isolate);
+  return (env && env == env_) ? env : nullptr;
 }
 
 }  // namespace electron

shell/renderer/electron_renderer_client.h

@@ -44,7 +44,8 @@ class ElectronRendererClient : public RendererClientBase {
   void WillDestroyWorkerContextOnWorkerThread(
       v8::Local<v8::Context> context) override;
 
-  node::Environment* GetEnvironment(content::RenderFrame* frame) const;
+  node::Environment* GetEnvironment(content::RenderFrame* frame,
+                                    v8::Isolate* isolate) const;
 
   // Whether the node integration has been initialized.
   bool node_integration_initialized_ = false;
@@ -55,7 +56,9 @@ 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_;
+  node::Environment* env_;
+
+  std::set<v8::Handle<v8::Context>> contexts_;
 
   // 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

@@ -45,14 +45,17 @@ WebWorkerObserver::~WebWorkerObserver() {
   // Destroying the node environment will also run the uv loop,
   // Node.js expects `kExplicit` microtasks policy and will run microtasks
   // checkpoints after every call into JavaScript. Since we use a different
-  // policy in the renderer - switch to `kExplicit`
-  v8::MicrotaskQueue* microtask_queue =
-      node_bindings_->uv_env()->context()->GetMicrotaskQueue();
+  // policy in the renderer - switch to `kExplicit` and then drop back to the
+  // previous policy value.
+  auto* env = node_bindings_->uv_env();
+  v8::MicrotaskQueue* microtask_queue = env->context()->GetMicrotaskQueue();
   auto old_policy = microtask_queue->microtasks_policy();
   DCHECK_EQ(microtask_queue->GetMicrotasksScopeDepth(), 0);
   microtask_queue->set_microtasks_policy(v8::MicrotasksPolicy::kExplicit);
-  node::FreeEnvironment(node_bindings_->uv_env());
+
+  node::FreeEnvironment(env);
   node::FreeIsolateData(node_bindings_->isolate_data());
+
   microtask_queue->set_microtasks_policy(old_policy);
 }
 

spec/chromium-spec.ts

@@ -1114,6 +1114,29 @@ describe('chromium features', () => {
       expect(frameName).to.equal('__proto__');
     });
 
+    it('works when used in conjunction with the vm module', async () => {
+      const w = new BrowserWindow({
+        show: false,
+        webPreferences: {
+          nodeIntegration: true,
+          contextIsolation: false
+        }
+      });
+
+      await w.loadFile(path.resolve(__dirname, 'fixtures', 'blank.html'));
+
+      const { contextObject } = await w.webContents.executeJavaScript(`(async () => {
+        const vm = require('node:vm');
+        const contextObject = { count: 1, type: 'gecko' };
+        window.open('');
+        vm.runInNewContext('count += 1; type = "chameleon";', contextObject);
+        console.log(contextObject);
+        return { contextObject };
+      })()`);
+
+      expect(contextObject).to.deep.equal({ count: 2, type: 'chameleon' });
+    });
+
     // FIXME(nornagon): I'm not sure this ... ever was correct?
     xit('inherit options of parent window', async () => {
       const w = new BrowserWindow({ show: false, width: 123, height: 456 });