From a9dda01a07ba9a47f57df36967193bc4230d621a Mon Sep 17 00:00:00 2001 From: deepak1556 Date: Tue, 15 Mar 2022 21:20:23 +0900 Subject: [PATCH 1/2] fix: crash when destroying node env with pending promises --- shell/renderer/electron_renderer_client.cc | 19 ++++++++++++------- shell/renderer/web_worker_observer.cc | 9 +++++++-- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/shell/renderer/electron_renderer_client.cc b/shell/renderer/electron_renderer_client.cc index f24c8073935fe..2c80080172c1f 100644 --- 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 index 287be9ead13f0..ca929bf9e4bb9 100644 --- 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()); } From f4fa04ae7aa9aabc5d2533b87e9a6798574e368b Mon Sep 17 00:00:00 2001 From: deepak1556 Date: Wed, 16 Mar 2022 16:19:03 +0900 Subject: [PATCH 2/2] chore: add spec --- .../fs-promises-renderer-crash/index.html | 17 +++++++++++ .../fs-promises-renderer-crash/index.js | 28 +++++++++++++++++++ 2 files changed, 45 insertions(+) create mode 100644 spec-main/fixtures/crash-cases/fs-promises-renderer-crash/index.html create mode 100644 spec-main/fixtures/crash-cases/fs-promises-renderer-crash/index.js 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 new file mode 100644 index 0000000000000..9721dac6ed821 --- /dev/null +++ b/spec-main/fixtures/crash-cases/fs-promises-renderer-crash/index.html @@ -0,0 +1,17 @@ + + + + + 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 new file mode 100644 index 0000000000000..4c74ba39f74d6 --- /dev/null +++ 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')); + } + }); +});