fix: microtasks policy in CreateEnvironment #29531
Conversation
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
|
@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. |
|
I should have probably used |
|
Done. |
|
@indutny no problem from me, thanks for asking. |
spec-main/fixtures/crash-cases/setimmediate-window-open-crash/index.html
Show resolved
Hide resolved
spec-main/fixtures/crash-cases/setimmediate-window-open-crash/index.html
Outdated
Show resolved
Hide resolved
spec-main/fixtures/crash-cases/setimmediate-window-open-crash/index.html
Outdated
Show resolved
Hide resolved
shell/common/node_bindings.cc
Outdated
| // Match Blink's behavior by allowing microtasks invocation to be controlled | ||
| // by MicrotasksScope objects. | ||
| is.policy = v8::MicrotasksPolicy::kScoped; | ||
| is.policy = context->GetIsolate()->GetMicrotasksPolicy(); |
There was a problem hiding this comment.
This comment is now incorrect, right? Is the new behavior safe? Why was this the way it was before?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Changed the comment to reflect new behavior.
…index.html Co-authored-by: Jeremy Rose <nornagon@nornagon.net>
…index.html Co-authored-by: Jeremy Rose <nornagon@nornagon.net>
|
@nornagon kindly reminding about it. Thanks! |
| // 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(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ack. Thanks for writing a much better comment than I was capable of 😅
|
Release Notes Persisted
|
|
I have automatically backported this PR to "12-x-y", please check out #29807 |
|
I have automatically backported this PR to "13-x-y", please check out #29808 |
|
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>
Description of Change
Microtasks policy should not be updated for the renderer because
NodeBindings::CreateEnvironmentmight be entered with or withoutUvRunOnce()on stack. One of the examples of such calls iswindow.open()which is possible to invoke whileuv_run()is stillrunning (e.g. with
setImmediate()).All in all, it doesn't matter that much which policy we use since
v8::MicrotasksScopehas a check for the policy in its destructor andno commits will be made if the policy is
kExplicit. It is important,however, to not change the policy in the middle of
UvRunOnce()so weshould respect whatever we currently have and move on.
Fix: #29463
cc @nornagon @codebytere @deepak1556
Checklist
npm testpassesRelease Notes
Notes: fix crashes in debug builds caused by microtasks policy mismatch