fix: window.open causing occasional Node.js crashes

patches/chromium/fix_harden_blink_scriptstate_maybefrom.patch

@@ -40,13 +40,14 @@ accessing uninitialized lower indexes can return garbage values that cannot be n
 Refer to v8::EmbedderDataSlot::store_aligned_pointer for context.
 
 diff --git a/gin/public/gin_embedders.h b/gin/public/gin_embedders.h
-index 8d7c5631fd8f1499c67384286f0e3c4037673b32..6a7491bc27334f6d1b1175eaa472c888e2b35b5e 100644
+index 8d7c5631fd8f1499c67384286f0e3c4037673b32..99b2e2f63be8a46c5546dd53bc9b05e8c54e857c 100644
 --- a/gin/public/gin_embedders.h
 +++ b/gin/public/gin_embedders.h
-@@ -18,6 +18,7 @@ namespace gin {
+@@ -18,6 +18,8 @@ namespace gin {
  enum GinEmbedder : uint16_t {
    kEmbedderNativeGin,
    kEmbedderBlink,
++  kEmbedderElectron,
 +  kEmbedderBlinkTag,
    kEmbedderPDFium,
    kEmbedderFuchsia,

shell/common/node_bindings.cc

@@ -33,7 +33,6 @@
 #include "shell/common/gin_helper/locker.h"
 #include "shell/common/gin_helper/microtasks_scope.h"
 #include "shell/common/mac/main_application_bundle.h"
-#include "shell/common/node_includes.h"
 #include "third_party/blink/renderer/bindings/core/v8/v8_initializer.h"  // nogncheck
 #include "third_party/electron_node/src/debug_utils.h"
 
@@ -518,8 +517,9 @@ 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 = node::CreateIsolateData(isolate, uv_loop_, platform);
+  context->SetAlignedPointerInEmbedderData(kElectronContextEmbedderDataIndex,
+                                           static_cast<void*>(isolate_data));
 
   node::Environment* env;
   uint64_t flags = node::EnvironmentFlags::kDefaultFlags |
@@ -550,7 +550,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

@@ -13,21 +13,24 @@
 #include "base/memory/raw_ptr.h"
 #include "base/memory/raw_ptr_exclusion.h"
 #include "base/memory/weak_ptr.h"
+#include "gin/public/context_holder.h"
+#include "gin/public/gin_embedders.h"
+#include "shell/common/node_includes.h"
 #include "uv.h"  // NOLINT(build/include_directory)
 #include "v8/include/v8.h"
 
 namespace base {
 class SingleThreadTaskRunner;
 }
 
-namespace node {
-class Environment;
-class MultiIsolatePlatform;
-class IsolateData;
-}  // namespace node
-
 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
@@ -108,11 +111,24 @@ 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(kElectronContextEmbedderDataIndex,
+                                             nullptr);
+  }
+
+  node::IsolateData* isolate_data(v8::Local<v8::Context> context) const {
+    if (context->GetNumberOfEmbedderDataFields() <=
+        kElectronContextEmbedderDataIndex) {
+      return nullptr;
+    }
+    auto* isolate_data = static_cast<node::IsolateData*>(
+        context->GetAlignedPointerFromEmbedderData(
+            kElectronContextEmbedderDataIndex));
+    CHECK(isolate_data);
+    CHECK(isolate_data->event_loop());
+    return isolate_data;
   }
-  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

@@ -6,6 +6,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"
@@ -144,8 +145,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,18 @@ 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);
+  if (node_bindings_->uv_env() == nullptr) {
+    node::FreeIsolateData(node_bindings_->isolate_data(env->context()));
+    node_bindings_->clear_isolate_data(env->context());
+  }
+
   microtask_queue->set_microtasks_policy(old_policy);
 }
 

spec/chromium-spec.ts

@@ -1111,9 +1111,31 @@
           return { action: 'allow' };
         });
       });
       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);
+        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 });