fix: crash when destroying node env with pending promises (electron#3…

…3280) * fix: crash when destroying node env with pending promises * chore: add spec
solarwindscloud · Feb 22, 2023 · 76eefbb · 76eefbb
1 parent 9c7c31f
commit 76eefbb
Show file tree

Hide file tree

Showing 4 changed files with 64 additions and 9 deletions.
diff --git a/shell/renderer/electron_renderer_client.cc b/shell/renderer/electron_renderer_client.cc
@@ -151,17 +151,22 @@ void ElectronRendererClient::WillReleaseScriptContext(
   if (env == node_bindings_->uv_env())
     node_bindings_->set_uv_env(nullptr);
 
-  // Destroy the node environment.  We only do this if node support has been
-  // enabled for sub-frames to avoid a change-of-behavior / introduce crashes
-  // for existing users.
-  // We also do this if we have disable electron site instance overrides to
-  // avoid memory leaks
-  auto prefs = render_frame->GetBlinkPreferences();
-  gin_helper::MicrotasksScope microtasks_scope(env->isolate());
+  // 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` and then drop back to the
+  // previous policy value.
+  v8::Isolate* isolate = context->GetIsolate();
+  auto old_policy = isolate->GetMicrotasksPolicy();
+  DCHECK_EQ(v8::MicrotasksScope::GetCurrentDepth(isolate), 0);
+  isolate->SetMicrotasksPolicy(v8::MicrotasksPolicy::kExplicit);
+
   node::FreeEnvironment(env);
   if (env == node_bindings_->uv_env())
     node::FreeIsolateData(node_bindings_->isolate_data());
 
+  isolate->SetMicrotasksPolicy(old_policy);
+
   // ElectronBindings is tracking node environments.
   electron_bindings_->EnvironmentDestroyed(env);
 }

diff --git a/shell/renderer/web_worker_observer.cc b/shell/renderer/web_worker_observer.cc
@@ -37,8 +37,13 @@ WebWorkerObserver::WebWorkerObserver()
 
 WebWorkerObserver::~WebWorkerObserver() {
   lazy_tls.Pointer()->Set(nullptr);
-  gin_helper::MicrotasksScope microtasks_scope(
-      node_bindings_->uv_env()->isolate());
+  // 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::Isolate* isolate = node_bindings_->uv_env()->isolate();
+  DCHECK_EQ(v8::MicrotasksScope::GetCurrentDepth(isolate), 0);
+  isolate->SetMicrotasksPolicy(v8::MicrotasksPolicy::kExplicit);
   node::FreeEnvironment(node_bindings_->uv_env());
   node::FreeIsolateData(node_bindings_->isolate_data());
 }

diff --git a/spec-main/fixtures/crash-cases/fs-promises-renderer-crash/index.html b/spec-main/fixtures/crash-cases/fs-promises-renderer-crash/index.html
@@ -0,0 +1,17 @@
+<html>
+<body>
+<script>
+const fs = require('fs');
+const { ipcRenderer } = require('electron');
+
+async function readFile(path) {
+    await fs.promises.readFile(path);
+}
+
+ipcRenderer.on('reload', (_, path) => {
+    readFile(path);
+    window.location.reload();
+});
+</script>
+</body>
+</html>
diff --git a/spec-main/fixtures/crash-cases/fs-promises-renderer-crash/index.js b/spec-main/fixtures/crash-cases/fs-promises-renderer-crash/index.js
@@ -0,0 +1,28 @@
+const { app, BrowserWindow, ipcMain } = require('electron');
+const path = require('path');
+
+app.whenReady().then(() => {
+  let reloadCount = 0;
+  const win = new BrowserWindow({
+    show: false,
+    webPreferences: {
+      nodeIntegration: true,
+      contextIsolation: false
+    }
+  });
+
+  win.loadFile('index.html');
+
+  win.webContents.on('render-process-gone', () => {
+    process.exit(1);
+  });
+
+  win.webContents.on('did-finish-load', () => {
+    if (reloadCount > 2) {
+      setImmediate(() => app.quit());
+    } else {
+      reloadCount += 1;
+      win.webContents.send('reload', path.join(__dirname, '..', '..', 'cat.pdf'));
+    }
+  });
+});