Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: perform microtask checkpoint before MicrotaskRunner destruction #38828

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions shell/browser/javascript_environment.cc
Original file line number Diff line number Diff line change
Expand Up @@ -333,11 +333,13 @@ void JavascriptEnvironment::CreateMicrotasksRunner() {

void JavascriptEnvironment::DestroyMicrotasksRunner() {
DCHECK(microtasks_runner_);
microtasks_runner_->PerformCheckpoint();
{
v8::HandleScope scope(isolate_);
gin_helper::CleanedUpAtExit::DoCleanup();
}
base::CurrentThread::Get()->RemoveTaskObserver(microtasks_runner_.get());
microtasks_runner_.reset();
}

} // namespace electron
25 changes: 14 additions & 11 deletions shell/browser/microtasks_runner.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,25 +14,28 @@

MicrotasksRunner::MicrotasksRunner(v8::Isolate* isolate) : isolate_(isolate) {}

void MicrotasksRunner::WillProcessTask(const base::PendingTask& pending_task,
bool was_blocked_or_low_priority) {}

void MicrotasksRunner::DidProcessTask(const base::PendingTask& pending_task) {
v8::Isolate::Scope scope(isolate_);
void MicrotasksRunner::PerformCheckpoint() {
// In the browser process we follow Node.js microtask policy of kExplicit
// and let the MicrotaskRunner which is a task observer for chromium UI thread
// scheduler run the microtask checkpoint. This worked fine because Node.js
// also runs microtasks through its task queue, but after
// https://github.com/electron/electron/issues/20013 Node.js now performs its
// own microtask checkpoint and it may happen is some situations that there is
// contention for performing checkpoint between Node.js and chromium, ending
// up Node.js dealying its callbacks. To fix this, now we always lets Node.js
// up Node.js delaying its callbacks. To fix this, now we always let Node.js

Check failure on line 25 in shell/browser/microtasks_runner.cc

View check run for this annotation

trop / Backportable? - 29-x-y

shell/browser/microtasks_runner.cc#L25

Patch Conflict
Raw output
++<<<<<<< HEAD
 +  // up Node.js delaying its callbacks. To fix this, now we always lets Node.js
++=======
+   // up Node.js delaying its callbacks. To fix this, now we always let Node.js
++>>>>>>> fix: pending-microtasks-on-exit bug that caused stderr test flake

Check failure on line 25 in shell/browser/microtasks_runner.cc

View check run for this annotation

trop / Backportable? - 29-x-y

shell/browser/microtasks_runner.cc#L25

Patch Conflict
Raw output
++<<<<<<< HEAD
 +  // up Node.js delaying its callbacks. To fix this, now we always lets Node.js
++=======
+   // up Node.js delaying its callbacks. To fix this, now we always let Node.js
++>>>>>>> fix: pending-microtasks-on-exit bug that caused stderr test flake

Check failure on line 25 in shell/browser/microtasks_runner.cc

View check run for this annotation

trop / Backportable? - 29-x-y

shell/browser/microtasks_runner.cc#L25

Patch Conflict
Raw output
++<<<<<<< HEAD
 +  // up Node.js delaying its callbacks. To fix this, now we always lets Node.js
++=======
+   // up Node.js delaying its callbacks. To fix this, now we always let Node.js
++>>>>>>> fix: pending-microtasks-on-exit bug that caused stderr test flake

Check failure on line 25 in shell/browser/microtasks_runner.cc

View check run for this annotation

trop / Backportable? - 29-x-y

shell/browser/microtasks_runner.cc#L25

Patch Conflict
Raw output
++<<<<<<< HEAD
 +  // up Node.js delaying its callbacks. To fix this, now we always lets Node.js
++=======
+   // up Node.js delaying its callbacks. To fix this, now we always let Node.js
++>>>>>>> fix: pending-microtasks-on-exit bug that caused stderr test flake

Check failure on line 25 in shell/browser/microtasks_runner.cc

View check run for this annotation

trop / Backportable? - 30-x-y

shell/browser/microtasks_runner.cc#L25

Patch Conflict
Raw output
++<<<<<<< HEAD
 +  // up Node.js delaying its callbacks. To fix this, now we always lets Node.js
++=======
+   // up Node.js delaying its callbacks. To fix this, now we always let Node.js
++>>>>>>> fix: pending-microtasks-on-exit bug that caused stderr test flake

Check failure on line 25 in shell/browser/microtasks_runner.cc

View check run for this annotation

trop / Backportable? - 30-x-y

shell/browser/microtasks_runner.cc#L25

Patch Conflict
Raw output
++<<<<<<< HEAD
 +  // up Node.js delaying its callbacks. To fix this, now we always lets Node.js
++=======
+   // up Node.js delaying its callbacks. To fix this, now we always let Node.js
++>>>>>>> fix: pending-microtasks-on-exit bug that caused stderr test flake

Check failure on line 25 in shell/browser/microtasks_runner.cc

View check run for this annotation

trop / Backportable? - 30-x-y

shell/browser/microtasks_runner.cc#L25

Patch Conflict
Raw output
++<<<<<<< HEAD
 +  // up Node.js delaying its callbacks. To fix this, now we always lets Node.js
++=======
+   // up Node.js delaying its callbacks. To fix this, now we always let Node.js
++>>>>>>> fix: pending-microtasks-on-exit bug that caused stderr test flake

Check failure on line 25 in shell/browser/microtasks_runner.cc

View check run for this annotation

trop / Backportable? - 31-x-y

shell/browser/microtasks_runner.cc#L25

Patch Conflict
Raw output
++<<<<<<< HEAD
 +  // up Node.js delaying its callbacks. To fix this, now we always lets Node.js
++=======
+   // up Node.js delaying its callbacks. To fix this, now we always let Node.js
++>>>>>>> fix: pending-microtasks-on-exit bug that caused stderr test flake
// handle the checkpoint in the browser process.
{
v8::HandleScope handle_scope(isolate_);
node::CallbackScope microtasks_scope(isolate_, v8::Object::New(isolate_),
{0, 0});
}
// node::CallbackScope performs the checkpoint when it goes out of scope.
v8::Isolate::Scope scope(isolate_);
v8::HandleScope handle_scope(isolate_);
node::CallbackScope microtasks_scope(isolate_, v8::Object::New(isolate_),
{0, 0});
}

void MicrotasksRunner::WillProcessTask(const base::PendingTask& pending_task,
bool was_blocked_or_low_priority) {}

void MicrotasksRunner::DidProcessTask(const base::PendingTask& pending_task) {
PerformCheckpoint();
}

} // namespace electron
2 changes: 2 additions & 0 deletions shell/browser/microtasks_runner.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ class MicrotasksRunner : public base::TaskObserver {
public:
explicit MicrotasksRunner(v8::Isolate* isolate);

void PerformCheckpoint();

// base::TaskObserver
void WillProcessTask(const base::PendingTask& pending_task,
bool was_blocked_or_low_priority) override;
Expand Down