fix: window.open causing occasional Node.js crashes (#38754)

* fix: window.open causing occasional Node.js crashes

* chore: always free isolate data

* chore: clear pending ticks in worker thread

* fix: UAF crash when creating WebWorkerObserver

---------

Co-authored-by: deepak1556 <hop2deep@gmail.com>

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_)
+  auto* isolate_data = node::CreateIsolateData(isolate, uv_loop_, platform);
-    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,6 +13,9 @@
 #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"
 
@@ -20,14 +23,14 @@ 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.
+  // Clears the PerIsolateData.
-  void set_isolate_data(node::IsolateData* isolate_data) {
+  void clear_isolate_data(v8::Local<v8::Context> context) {
-    isolate_data_ = isolate_data;
+    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"
@@ -143,10 +144,8 @@ void ElectronRendererClient::WillReleaseScriptContext(
   microtask_queue->set_microtasks_policy(v8::MicrotasksPolicy::kExplicit);
 
   node::FreeEnvironment(env);
-  if (node_bindings_->uv_env() == nullptr) {
+  node::FreeIsolateData(node_bindings_->isolate_data(context));
-    node::FreeIsolateData(node_bindings_->isolate_data());
+  node_bindings_->clear_isolate_data(context);
-    node_bindings_->set_isolate_data(nullptr);
-  }
 
   microtask_queue->set_microtasks_policy(old_policy);
 
@@ -159,19 +158,20 @@ void ElectronRendererClient::WorkerScriptReadyForEvaluationOnWorkerThread(
   // We do not create a Node.js environment in service or shared workers
   // owing to an inability to customize sandbox policies in these workers
   // given that they're run out-of-process.
+  // Also avoid creating a Node.js environment for worklet global scope
+  // created on the main thread.
   auto* ec = blink::ExecutionContext::From(context);
-  if (ec->IsServiceWorkerGlobalScope() || ec->IsSharedWorkerGlobalScope())
+  if (ec->IsServiceWorkerGlobalScope() || ec->IsSharedWorkerGlobalScope() ||
+      ec->IsMainThreadWorkletGlobalScope())
     return;
 
   // This won't be correct for in-process child windows with webPreferences
   // that have a different value for nodeIntegrationInWorker
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kNodeIntegrationInWorker)) {
-    // WorkerScriptReadyForEvaluationOnWorkerThread can be invoked multiple
+    auto* current = WebWorkerObserver::GetCurrent();
-    // times for the same thread, so we need to create a new observer each time
+    if (current)
-    // this happens. We use a ThreadLocalOwnedPointer to ensure that the old
+      return;
-    // observer for a given thread gets destructed when swapping with the new
-    // observer in WebWorkerObserver::Create.
     WebWorkerObserver::Create()->WorkerScriptReadyForEvaluation(context);
   }
 }
@@ -179,7 +179,8 @@ void ElectronRendererClient::WorkerScriptReadyForEvaluationOnWorkerThread(
 void ElectronRendererClient::WillDestroyWorkerContextOnWorkerThread(
     v8::Local<v8::Context> context) {
   auto* ec = blink::ExecutionContext::From(context);
-  if (ec->IsServiceWorkerGlobalScope() || ec->IsSharedWorkerGlobalScope())
+  if (ec->IsServiceWorkerGlobalScope() || ec->IsSharedWorkerGlobalScope() ||
+      ec->IsMainThreadWorkletGlobalScope())
     return;
 
   // TODO(loc): Note that this will not be correct for in-process child windows

shell/renderer/web_worker_observer.cc

@@ -41,20 +41,7 @@ WebWorkerObserver::WebWorkerObserver()
       electron_bindings_(
           std::make_unique<ElectronBindings>(node_bindings_->uv_loop())) {}
 
-WebWorkerObserver::~WebWorkerObserver() {
+WebWorkerObserver::~WebWorkerObserver() = default;
-  // 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();
-  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());
-  microtask_queue->set_microtasks_policy(old_policy);
-}
 
 void WebWorkerObserver::WorkerScriptReadyForEvaluation(
     v8::Local<v8::Context> worker_context) {
@@ -95,6 +82,24 @@ void WebWorkerObserver::ContextWillDestroy(v8::Local<v8::Context> context) {
   if (env)
     gin_helper::EmitEvent(env->isolate(), env->process_object(), "exit");
 
+  // 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 = 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(context));
+  node_bindings_->clear_isolate_data(context);
+
+  microtask_queue->set_microtasks_policy(old_policy);
+
+  // ElectronBindings is tracking node environments.
+  electron_bindings_->EnvironmentDestroyed(env);
+
   if (lazy_tls->Get())
     lazy_tls->Set(nullptr);
 }

spec/chromium-spec.ts

@@ -1114,6 +1114,28 @@ 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);
+        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 });