From d4a1b411297ae7651441cf697b5251445c7a575c Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Sun, 20 Jun 2021 22:06:17 -0700 Subject: [PATCH] fix: microtasks policy in CreateEnvironment (#29531) * fix: microtasks policy in CreateEnvironment Microtasks policy should not be updated for the renderer because `NodeBindings::CreateEnvironment` might be entered with or without `UvRunOnce()` on stack. One of the examples of such calls is `window.open()` which is possible to invoke while `uv_run()` is still running (e.g. with `setImmediate()`). All in all, it doesn't matter that much which policy we use since `v8::MicrotasksScope` has a check for the policy in its destructor and no commits will be made if the policy is `kExplicit`. It is important, however, to not change the policy in the middle of `UvRunOnce()` so we should respect whatever we currently have and move on. Fix: #29463 * Move test to a better place * Update spec-main/fixtures/crash-cases/setimmediate-window-open-crash/index.html Co-authored-by: Jeremy Rose * Update spec-main/fixtures/crash-cases/setimmediate-window-open-crash/index.html Co-authored-by: Jeremy Rose * simplify crash-case * comment * fix comment Co-authored-by: Fedor Indutny <79877362+indutny-signal@users.noreply.github.com> Co-authored-by: Jeremy Rose Co-authored-by: Fedor Indutny --- shell/common/node_bindings.cc | 10 ++++++--- .../setimmediate-window-open-crash/index.html | 21 +++++++++++++++++++ .../setimmediate-window-open-crash/index.js | 21 +++++++++++++++++++ 3 files changed, 49 insertions(+), 3 deletions(-) create mode 100644 spec-main/fixtures/crash-cases/setimmediate-window-open-crash/index.html create mode 100644 spec-main/fixtures/crash-cases/setimmediate-window-open-crash/index.js diff --git a/shell/common/node_bindings.cc b/shell/common/node_bindings.cc index a1e46fd9a3ed..e1c6634306b7 100644 --- a/shell/common/node_bindings.cc +++ b/shell/common/node_bindings.cc @@ -482,9 +482,13 @@ node::Environment* NodeBindings::CreateEnvironment( // Node.js requires that microtask checkpoints be explicitly invoked. is.policy = v8::MicrotasksPolicy::kExplicit; } else { - // Match Blink's behavior by allowing microtasks invocation to be controlled - // by MicrotasksScope objects. - is.policy = v8::MicrotasksPolicy::kScoped; + // Blink expects the microtasks policy to be kScoped, but Node.js expects it + // to be kExplicit. In the renderer, there can be many contexts within the + // same isolate, so we don't want to change the existing policy here, which + // could be either kExplicit or kScoped depending on whether we're executing + // from within a Node.js or a Blink entrypoint. Instead, the policy is + // toggled to kExplicit when entering Node.js through UvRunOnce. + is.policy = context->GetIsolate()->GetMicrotasksPolicy(); // We do not want to use Node.js' message listener as it interferes with // Blink's. diff --git a/spec-main/fixtures/crash-cases/setimmediate-window-open-crash/index.html b/spec-main/fixtures/crash-cases/setimmediate-window-open-crash/index.html new file mode 100644 index 000000000000..9bb131aef454 --- /dev/null +++ b/spec-main/fixtures/crash-cases/setimmediate-window-open-crash/index.html @@ -0,0 +1,21 @@ + + + + + diff --git a/spec-main/fixtures/crash-cases/setimmediate-window-open-crash/index.js b/spec-main/fixtures/crash-cases/setimmediate-window-open-crash/index.js new file mode 100644 index 000000000000..2032f096c11c --- /dev/null +++ b/spec-main/fixtures/crash-cases/setimmediate-window-open-crash/index.js @@ -0,0 +1,21 @@ +const { app, BrowserWindow } = require('electron'); + +function createWindow () { + const mainWindow = new BrowserWindow({ + webPreferences: { + nodeIntegration: true, + contextIsolation: false, + nativeWindowOpen: true + } + }); + + mainWindow.on('close', () => { + app.quit(); + }); + + mainWindow.loadFile('index.html'); +} + +app.whenReady().then(() => { + createWindow(); +});