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 <nornagon@nornagon.net> * Update spec-main/fixtures/crash-cases/setimmediate-window-open-crash/index.html Co-authored-by: Jeremy Rose <nornagon@nornagon.net> * simplify crash-case * comment * fix comment Co-authored-by: Fedor Indutny <79877362+indutny-signal@users.noreply.github.com> Co-authored-by: Jeremy Rose <nornagon@nornagon.net> Co-authored-by: Fedor Indutny <indutny@signal.org>
2021-06-20 22:06:17 -07:00 · 2021-06-20 22:06:17 -07:00 · d4a1b41129
--- 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.
--- a/spec-main/fixtures/crash-cases/setimmediate-window-open-crash/index.html
+++ b/spec-main/fixtures/crash-cases/setimmediate-window-open-crash/index.html
@ -0,0 +1,21 @@
+<html>
+<body>
+<script>
+// `setImmediate` schedules a function to be run on the next UV tick, which
+//  ensures that `window.open` is run from `UvRunOnce()`.
+setImmediate(async () => {
+  if (!location.hash) {
+    const p = new Promise(resolve => {
+      window.addEventListener('message', resolve, { once: true });
+    });
+    window.open('#foo', '', 'nodeIntegration=no,show=no');
+    const e = await p;
+
+    window.close();
+  } else {
+    window.opener.postMessage('foo', '*');
+  }
+});
+</script>
+</body>
+</html>
--- a/spec-main/fixtures/crash-cases/setimmediate-window-open-crash/index.js
+++ 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();
+});