From 1fe12a6adebd6f3cfef00eb22afd449d574977e2 Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Tue, 13 Jun 2023 12:16:14 +0200 Subject: [PATCH] fix: window.open causing occasional Node.js crashes --- shell/common/node_bindings.cc | 13 +++++++++--- shell/common/node_bindings.h | 20 +++++++++++++++---- shell/renderer/electron_renderer_client.cc | 5 +++-- shell/renderer/web_worker_observer.cc | 8 ++++---- spec/chromium-spec.ts | 23 ++++++++++++++++++++++ 5 files changed, 56 insertions(+), 13 deletions(-) diff --git a/shell/common/node_bindings.cc b/shell/common/node_bindings.cc index 36a06d1ed454c..c3afe1c8ccf97 100644 --- a/shell/common/node_bindings.cc +++ b/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(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(isolate_data), context, args, exec_args, static_cast(flags)); if (try_catch.HasCaught()) { diff --git a/shell/common/node_bindings.h b/shell/common/node_bindings.h index f8df9cb90876d..26b70495def00 100644 --- a/shell/common/node_bindings.h +++ b/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 context) { + context->SetAlignedPointerInEmbedderData( + ELECTRON_CONTEXT_EMBEDDER_DATA_INDEX, nullptr); + } + + node::IsolateData* isolate_data(v8::Local context) const { + return static_cast( + 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; } diff --git a/shell/renderer/electron_renderer_client.cc b/shell/renderer/electron_renderer_client.cc index caab3b88d5642..d245db5231655 100644 --- a/shell/renderer/electron_renderer_client.cc +++ b/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); diff --git a/shell/renderer/web_worker_observer.cc b/shell/renderer/web_worker_observer.cc index 2ee0d387cc5b8..e6d808a2e7abd 100644 --- a/shell/renderer/web_worker_observer.cc +++ b/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); } diff --git a/spec/chromium-spec.ts b/spec/chromium-spec.ts index ceffda8549cee..4c01de6538e51 100644 --- a/spec/chromium-spec.ts +++ b/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 });