fix: window.open causing occasional Node.js crashes

shell/common/node_bindings.cc

@@ -43,6 +43,8 @@
 #include "shell/common/crash_keys.h"
 #endif
 
+#define NODE_CONTEXT_BLINK_EMBEDDER_DATA_INDEX 12
+
 #define ELECTRON_BROWSER_BINDINGS(V)     \
   V(electron_browser_app)                \
   V(electron_browser_auto_updater)       \
@@ -517,8 +519,13 @@ node::Environment* NodeBindings::CreateEnvironment(
 
   args.insert(args.begin() + 1, init_script);
 
-  if (!isolate_data_)
-    isolate_data_ = node::CreateIsolateData(isolate, uv_loop_, platform);
+  auto* isolate_data = context->GetAlignedPointerFromEmbedderData(
+      ELECTRON_CONTEXT_EMBEDDER_DATA_INDEX);
+  if (!isolate_data) {
+    isolate_data = node::CreateIsolateData(isolate, uv_loop_, platform);
+    context->SetAlignedPointerInEmbedderData(
+        ELECTRON_CONTEXT_EMBEDDER_DATA_INDEX, static_cast<void*>(isolate_data));
+  }
 
   node::Environment* env;
   uint64_t flags = node::EnvironmentFlags::kDefaultFlags |
@@ -549,7 +556,7 @@ node::Environment* NodeBindings::CreateEnvironment(
   {
     v8::TryCatch try_catch(isolate);
     env = node::CreateEnvironment(
-        isolate_data_, context, args, exec_args,
+        static_cast<node::IsolateData*>(isolate_data), context, args, exec_args,
         static_cast<node::EnvironmentFlags::Flags>(flags));
 
     if (try_catch.HasCaught()) {

shell/common/node_bindings.h

@@ -28,6 +28,12 @@ class IsolateData;
 
 namespace electron {
 
+// Choose a reasonable unique index that's higher than any Blink uses
+// and thus unlikley to collide with an existing index.
+#ifndef ELECTRON_CONTEXT_EMBEDDER_DATA_INDEX
+#define ELECTRON_CONTEXT_EMBEDDER_DATA_INDEX 12
+#endif
+
 // 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
@@ -108,11 +114,17 @@ 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;
+  // Clears the PerIsolateData.
+  void clear_isolate_data(v8::Local<v8::Context> context) {
+    context->SetAlignedPointerInEmbedderData(
+        ELECTRON_CONTEXT_EMBEDDER_DATA_INDEX, nullptr);
+  }
+
+  node::IsolateData* isolate_data(v8::Local<v8::Context> context) const {
+    return static_cast<node::IsolateData*>(
+        context->GetAlignedPointerFromEmbedderData(
+            ELECTRON_CONTEXT_EMBEDDER_DATA_INDEX));
   }
-  node::IsolateData* isolate_data() const { return isolate_data_; }
 
   // Gets/sets the environment to wrap uv loop.
   void set_uv_env(node::Environment* env) { uv_env_ = env; }

shell/renderer/electron_renderer_client.cc

@@ -8,6 +8,7 @@
 
 #include "base/command_line.h"
 #include "base/containers/contains.h"
+#include "base/debug/stack_trace.h"
 #include "content/public/renderer/render_frame.h"
 #include "electron/buildflags/buildflags.h"
 #include "net/http/http_request_headers.h"
@@ -146,8 +147,8 @@ void ElectronRendererClient::WillReleaseScriptContext(
 
   node::FreeEnvironment(env);
   if (node_bindings_->uv_env() == nullptr) {
-    node::FreeIsolateData(node_bindings_->isolate_data());
-    node_bindings_->set_isolate_data(nullptr);
+    node::FreeIsolateData(node_bindings_->isolate_data(context));
+    node_bindings_->clear_isolate_data(context);
   }
 
   microtask_queue->set_microtasks_policy(old_policy);

shell/renderer/web_worker_observer.cc

@@ -46,13 +46,13 @@ WebWorkerObserver::~WebWorkerObserver() {
   // 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();
+  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::FreeIsolateData(node_bindings_->isolate_data());
+  node::FreeEnvironment(env);
+  node::FreeIsolateData(node_bindings_->isolate_data(env->context()));
   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 });