fix: crash on `WebWorkerObserver` script execution (#37081)

fix: crash on WebWorkerObserver script execution

Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <[email protected]>

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);
   }
 }
 

shell/renderer/web_worker_observer.cc

@@ -4,7 +4,9 @@
 
 #include "shell/renderer/web_worker_observer.h"
 
-#include "base/lazy_instance.h"
+#include <utility>
+
+#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<WebWorkerObserver>>::DestructorAtExit lazy_tls =
-    LAZY_INSTANCE_INITIALIZER;
+static base::NoDestructor<base::ThreadLocalOwnedPointer<WebWorkerObserver>>
+    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<WebWorkerObserver>();
+  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<ElectronBindings>(node_bindings_->uv_loop())) {
-  lazy_tls.Pointer()->Set(this);
-}
+          std::make_unique<ElectronBindings>(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<v8::Context> context) {
   if (env)
     gin_helper::EmitEvent(env->isolate(), env->process_object(), "exit");
 
-  delete this;
+  if (lazy_tls->Get())
+    lazy_tls->Set(nullptr);
 }
 
 }  // namespace electron

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<v8::Context> context);
 
  private:
-  WebWorkerObserver();
-  ~WebWorkerObserver();
-
   std::unique_ptr<NodeBindings> node_bindings_;
   std::unique_ptr<ElectronBindings> electron_bindings_;
 };

spec/fixtures/crash-cases/worker-multiple-destroy/index.html

@@ -0,0 +1,22 @@
+<html>
+
+<body>
+  <style>
+    textarea {
+      background-image: url(checkerboard);
+      background-image: paint(checkerboard);
+    }
+
+    @supports (background: paint(id)) {
+      background-image: paint(checkerboard);
+    }
+  </style>
+  <textarea></textarea>
+  <script>
+    const addPaintWorklet = async () => {
+      await CSS.paintWorklet.addModule('worklet.js');
+    }
+  </script>
+</body>
+
+</html>

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();
+});

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);