Skip to content

Commit

Permalink
fix: pending-microtasks-on-exit bug that caused stderr test flake
Browse files Browse the repository at this point in the history
  • Loading branch information
ckerr authored and codebytere committed Oct 3, 2023
1 parent 9d0e6d0 commit cf48ad7
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 11 deletions.
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 @@ namespace electron {

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

0 comments on commit cf48ad7

Please sign in to comment.