diff --git a/shell/renderer/electron_renderer_client.cc b/shell/renderer/electron_renderer_client.cc index 50b5cde56e..bb1fd5f748 100644 --- a/shell/renderer/electron_renderer_client.cc +++ b/shell/renderer/electron_renderer_client.cc @@ -168,7 +168,12 @@ void ElectronRendererClient::WorkerScriptReadyForEvaluationOnWorkerThread( // that have a different value for nodeIntegrationInWorker if (base::CommandLine::ForCurrentProcess()->HasSwitch( switches::kNodeIntegrationInWorker)) { - WebWorkerObserver::GetCurrent()->WorkerScriptReadyForEvaluation(context); + // WorkerScriptReadyForEvaluationOnWorkerThread can be invoked multiple + // times for the same thread, so we need to create a new observer each time + // this happens. We use a ThreadLocalOwnedPointer to ensure that the old + // observer for a given thread gets destructed when swapping with the new + // observer in WebWorkerObserver::Create. + WebWorkerObserver::Create()->WorkerScriptReadyForEvaluation(context); } } @@ -182,7 +187,9 @@ void ElectronRendererClient::WillDestroyWorkerContextOnWorkerThread( // with webPreferences that have a different value for nodeIntegrationInWorker if (base::CommandLine::ForCurrentProcess()->HasSwitch( switches::kNodeIntegrationInWorker)) { - WebWorkerObserver::GetCurrent()->ContextWillDestroy(context); + auto* current = WebWorkerObserver::GetCurrent(); + if (current) + current->ContextWillDestroy(context); } } diff --git a/shell/renderer/web_worker_observer.cc b/shell/renderer/web_worker_observer.cc index 7cf9c1cd69..2ee0d387cc 100644 --- a/shell/renderer/web_worker_observer.cc +++ b/shell/renderer/web_worker_observer.cc @@ -4,7 +4,9 @@ #include "shell/renderer/web_worker_observer.h" -#include "base/lazy_instance.h" +#include + +#include "base/no_destructor.h" #include "base/threading/thread_local.h" #include "shell/common/api/electron_bindings.h" #include "shell/common/gin_helper/event_emitter_caller.h" @@ -15,28 +17,31 @@ namespace electron { namespace { -static base::LazyInstance< - base::ThreadLocalPointer>::DestructorAtExit lazy_tls = - LAZY_INSTANCE_INITIALIZER; +static base::NoDestructor> + lazy_tls; } // namespace // static WebWorkerObserver* WebWorkerObserver::GetCurrent() { - WebWorkerObserver* self = lazy_tls.Pointer()->Get(); - return self ? self : new WebWorkerObserver; + return lazy_tls->Get(); +} + +// static +WebWorkerObserver* WebWorkerObserver::Create() { + auto obs = std::make_unique(); + auto* obs_raw = obs.get(); + lazy_tls->Set(std::move(obs)); + return obs_raw; } WebWorkerObserver::WebWorkerObserver() : node_bindings_( NodeBindings::Create(NodeBindings::BrowserEnvironment::kWorker)), electron_bindings_( - std::make_unique(node_bindings_->uv_loop())) { - lazy_tls.Pointer()->Set(this); -} + std::make_unique(node_bindings_->uv_loop())) {} WebWorkerObserver::~WebWorkerObserver() { - lazy_tls.Pointer()->Set(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 @@ -90,7 +95,8 @@ void WebWorkerObserver::ContextWillDestroy(v8::Local context) { if (env) gin_helper::EmitEvent(env->isolate(), env->process_object(), "exit"); - delete this; + if (lazy_tls->Get()) + lazy_tls->Set(nullptr); } } // namespace electron diff --git a/shell/renderer/web_worker_observer.h b/shell/renderer/web_worker_observer.h index 0b05644fa0..bba7ae7e61 100644 --- a/shell/renderer/web_worker_observer.h +++ b/shell/renderer/web_worker_observer.h @@ -17,8 +17,13 @@ class NodeBindings; // Watches for WebWorker and insert node integration to it. class WebWorkerObserver { public: + WebWorkerObserver(); + ~WebWorkerObserver(); + // Returns the WebWorkerObserver for current worker thread. static WebWorkerObserver* GetCurrent(); + // Creates a new WebWorkerObserver for a given context. + static WebWorkerObserver* Create(); // disable copy WebWorkerObserver(const WebWorkerObserver&) = delete; @@ -28,9 +33,6 @@ class WebWorkerObserver { void ContextWillDestroy(v8::Local context); private: - WebWorkerObserver(); - ~WebWorkerObserver(); - std::unique_ptr node_bindings_; std::unique_ptr electron_bindings_; }; diff --git a/spec/fixtures/crash-cases/worker-multiple-destroy/index.html b/spec/fixtures/crash-cases/worker-multiple-destroy/index.html new file mode 100644 index 0000000000..131b48850d --- /dev/null +++ b/spec/fixtures/crash-cases/worker-multiple-destroy/index.html @@ -0,0 +1,22 @@ + + + + + + + + + diff --git a/spec/fixtures/crash-cases/worker-multiple-destroy/index.js b/spec/fixtures/crash-cases/worker-multiple-destroy/index.js new file mode 100644 index 0000000000..382212f918 --- /dev/null +++ b/spec/fixtures/crash-cases/worker-multiple-destroy/index.js @@ -0,0 +1,38 @@ +const { app, BrowserWindow } = require('electron'); + +async function createWindow () { + const mainWindow = new BrowserWindow({ + webPreferences: { + nodeIntegrationInWorker: true + } + }); + + let loads = 1; + mainWindow.webContents.on('did-finish-load', async () => { + if (loads === 2) { + process.exit(0); + } else { + loads++; + await mainWindow.webContents.executeJavaScript('addPaintWorklet()'); + await mainWindow.webContents.executeJavaScript('location.reload()'); + } + }); + + mainWindow.webContents.on('render-process-gone', () => { + process.exit(1); + }); + + await mainWindow.loadFile('index.html'); +} + +app.whenReady().then(() => { + createWindow(); + + app.on('activate', () => { + if (BrowserWindow.getAllWindows().length === 0) createWindow(); + }); +}); + +app.on('window-all-closed', () => { + if (process.platform !== 'darwin') app.quit(); +}); diff --git a/spec/fixtures/crash-cases/worker-multiple-destroy/worklet.js b/spec/fixtures/crash-cases/worker-multiple-destroy/worklet.js new file mode 100644 index 0000000000..61e24a0602 --- /dev/null +++ b/spec/fixtures/crash-cases/worker-multiple-destroy/worklet.js @@ -0,0 +1,18 @@ +class CheckerboardPainter { + paint (ctx, geom, properties) { + const colors = ['red', 'green', 'blue']; + const size = 32; + for (let y = 0; y < (geom.height / size); y++) { + for (let x = 0; x < (geom.width / size); x++) { + const color = colors[(x + y) % colors.length]; + ctx.beginPath(); + ctx.fillStyle = color; + ctx.rect(x * size, y * size, size, size); + ctx.fill(); + } + } + } +} + +// eslint-disable-next-line no-undef +registerPaint('checkerboard', CheckerboardPainter);