fix: microtasks policy in CreateEnvironment by indutny · Pull Request #29531 · electron/electron

indutny · 2021-06-03T20:05:57Z

Description of Change

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

cc @nornagon @codebytere @deepak1556

Checklist

PR description included and stakeholders cc'd
npm test passes
tests are changed or added
relevant documentation is changed or added
PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: fix crashes in debug builds caused by microtasks policy mismatch

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: electron#29463

indutny · 2021-06-03T20:08:01Z

@nornagon I've used your test, but didn't ask for permission. Please let me know if this is not okay with you, and I'll write something else in its place.

indutny · 2021-06-03T20:13:56Z

I should have probably used crash-cases for the test. Let me follow-up on this real quick.

indutny-signal · 2021-06-03T20:21:00Z

Done.

nornagon · 2021-06-03T20:37:28Z

@indutny no problem from me, thanks for asking.

spec-main/fixtures/crash-cases/setimmediate-window-open-crash/index.html

nornagon · 2021-06-03T21:00:59Z

shell/common/node_bindings.cc

    // Match Blink's behavior by allowing microtasks invocation to be controlled
    // by MicrotasksScope objects.
-    is.policy = v8::MicrotasksPolicy::kScoped;
+    is.policy = context->GetIsolate()->GetMicrotasksPolicy();


This comment is now incorrect, right? Is the new behavior safe? Why was this the way it was before?

It was introduced in 103fea5 originally by @zcbenz . @zcbenz could you take a look at this PR?

I believe it is safe to do it the way because everything outside of UvRunOnce() would have scoped policy, and all C++ code invokes from UvRunOnce() will have kExplicit. This PR doesn't change this behavior at all, but just makes sure that we don't change back to kScoped while we are in UvRunOnce().

As stated in the PR description, using kExplicit with MicrotasksScope is safe because of this check in v8.

Changed the comment to reflect new behavior.

It was introduced in 103fea5 originally by @zcbenz . @zcbenz could you take a look at this PR?

I don't quite remember the background of the change, but we have a test for the original issue so as long as the tests are passing there should be no problem.

…index.html Co-authored-by: Jeremy Rose <nornagon@nornagon.net>

indutny · 2021-06-09T07:23:40Z

@nornagon kindly reminding about it. Thanks!

nornagon

LGTM with updated comment.

nornagon · 2021-06-15T17:25:24Z

shell/common/node_bindings.cc

+    // by MicrotasksScope objects when queued outside of `UvRunOnce()` call, but
+    // use explicit microtasks policy while inside `UvRunOnce()` for better
+    // compatibility with Node.js and better performance.
+    is.policy = context->GetIsolate()->GetMicrotasksPolicy();


I think the comment here could be a little clearer, let's go with something like:

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.

Ack. Thanks for writing a much better comment than I was capable of 😅

release-clerk · 2021-06-21T05:06:20Z

Release Notes Persisted

fix crashes in debug builds caused by microtasks policy mismatch

trop · 2021-06-21T05:06:50Z

I have automatically backported this PR to "12-x-y", please check out #29807

trop · 2021-06-21T05:06:56Z

I have automatically backported this PR to "13-x-y", please check out #29808

trop · 2021-06-21T05:06:59Z

I have automatically backported this PR to "14-x-y", please check out #29809

* 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: electron#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>

electron-cation bot added the new-pr 🌱 PR opened recently label Jun 3, 2021

indutny mentioned this pull request Jun 3, 2021

Microtasks policy incorrect, leading to DCHECK, when calling window.open from setImmediate #29463

Closed

Move test to a better place

8d6e988

nornagon suggested changes Jun 3, 2021

View reviewed changes

indutny-signal and others added 4 commits June 3, 2021 15:47

Update spec-main/fixtures/crash-cases/setimmediate-window-open-crash/…

036036e

…index.html Co-authored-by: Jeremy Rose <nornagon@nornagon.net>

Update spec-main/fixtures/crash-cases/setimmediate-window-open-crash/…

8ff9cbf

…index.html Co-authored-by: Jeremy Rose <nornagon@nornagon.net>

simplify crash-case

2cc2332

comment

15f692a

deepak1556 approved these changes Jun 4, 2021

View reviewed changes

indutny requested a review from nornagon June 4, 2021 06:06

codebytere approved these changes Jun 4, 2021

View reviewed changes

codebytere added target/12-x-y semver/patch backwards-compatible bug fixes labels Jun 4, 2021

electron-cation bot removed the new-pr 🌱 PR opened recently label Jun 4, 2021

nornagon approved these changes Jun 15, 2021

View reviewed changes

fix comment

bbe2028

zcbenz merged commit d4a1b41 into electron:main Jun 21, 2021

This was referenced Jun 21, 2021

fix: microtasks policy in CreateEnvironment #29807

Merged

fix: microtasks policy in CreateEnvironment #29808

Merged

fix: microtasks policy in CreateEnvironment #29809

Merged

trop bot removed the target/12-x-y label Jun 21, 2021

trop bot added the in-flight/12-x-y label Jun 21, 2021

trop bot added in-flight/13-x-y and removed target/13-x-y labels Jun 21, 2021

Conversation

indutny commented Jun 3, 2021

Description of Change

Checklist

Release Notes

Uh oh!

indutny commented Jun 3, 2021

Uh oh!

indutny commented Jun 3, 2021

Uh oh!

indutny-signal commented Jun 3, 2021

Uh oh!

nornagon commented Jun 3, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nornagon Jun 3, 2021

Choose a reason for hiding this comment

Uh oh!

indutny-signal Jun 3, 2021

Choose a reason for hiding this comment

Uh oh!

indutny-signal Jun 3, 2021

Choose a reason for hiding this comment

Uh oh!

zcbenz Jun 4, 2021

Choose a reason for hiding this comment

Uh oh!

indutny commented Jun 9, 2021

Uh oh!

nornagon left a comment

Choose a reason for hiding this comment

Uh oh!

nornagon Jun 15, 2021

Choose a reason for hiding this comment

Uh oh!

indutny-signal Jun 16, 2021

Choose a reason for hiding this comment

Uh oh!

release-clerk bot commented Jun 21, 2021

Uh oh!

trop bot commented Jun 21, 2021

Uh oh!

trop bot commented Jun 21, 2021

Uh oh!

trop bot commented Jun 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants