fix: let Node.js perform microtask checkpoint in the main process (#2…

…4131) (#24178)

* fix: let Node.js perform microtask checkpoint in the main process

* fix: don't specify v8::MicrotasksScope for explicit policy

* fix: remove checkpoint from some call-sites

We already perform checkpoint at the end of a task,
either through MicrotaskRunner or through NodeBindings.
There isn't a need to add them again when calling into JS
except when dealing with promises.

* fix: remove checkpoint from some call-sites

We already perform checkpoint at the end of a task,
either through MicrotaskRunner or through NodeBindings.
There isn't a need to add them again when calling into JS
except when dealing with promises.

* fix incorrect specs

* default constructor arguments are considered for explicit mark

* add regression spec

filenames.gni

@@ -512,6 +512,8 @@ filenames = {
     "shell/common/gin_helper/function_template.h",
     "shell/common/gin_helper/locker.cc",
     "shell/common/gin_helper/locker.h",
+    "shell/common/gin_helper/microtasks_scope.cc",
+    "shell/common/gin_helper/microtasks_scope.h",
     "shell/common/gin_helper/object_template_builder.cc",
     "shell/common/gin_helper/object_template_builder.h",
     "shell/common/gin_helper/persistent_dictionary.cc",

shell/browser/api/electron_api_url_loader.cc

@@ -134,8 +134,6 @@ class JSChunkedDataPipeGetter : public gin::Wrappable<JSChunkedDataPipeGetter>,
     data_producer_ = std::make_unique<mojo::DataPipeProducer>(std::move(pipe));
 
     v8::HandleScope handle_scope(isolate_);
-    v8::MicrotasksScope script_scope(isolate_,
-                                     v8::MicrotasksScope::kRunMicrotasks);
     auto maybe_wrapper = GetWrapper(isolate_);
     v8::Local<v8::Value> wrapper;
     if (!maybe_wrapper.ToLocal(&wrapper)) {

shell/browser/electron_browser_main_parts.h

@@ -90,6 +90,7 @@ class ElectronBrowserMainParts : public content::BrowserMainParts {
 
   Browser* browser() { return browser_.get(); }
   BrowserProcessImpl* browser_process() { return fake_browser_process_.get(); }
+  NodeEnvironment* node_env() { return node_env_.get(); }
 
  protected:
   // content::BrowserMainParts:

shell/browser/javascript_environment.h

@@ -58,6 +58,8 @@ class NodeEnvironment {
   explicit NodeEnvironment(node::Environment* env);
   ~NodeEnvironment();
 
+  node::Environment* env() { return env_; }
+
  private:
   node::Environment* env_;
 

shell/browser/microtasks_runner.cc

@@ -4,6 +4,10 @@
 // found in the LICENSE file.
 
 #include "shell/browser/microtasks_runner.h"
+
+#include "shell/browser/electron_browser_main_parts.h"
+#include "shell/browser/javascript_environment.h"
+#include "shell/common/node_includes.h"
 #include "v8/include/v8.h"
 
 namespace electron {
@@ -15,7 +19,21 @@ void MicrotasksRunner::WillProcessTask(const base::PendingTask& pending_task,
 
 void MicrotasksRunner::DidProcessTask(const base::PendingTask& pending_task) {
   v8::Isolate::Scope scope(isolate_);
-  v8::MicrotasksScope::PerformCheckpoint(isolate_);
+  // 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 micotask 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
+  // handle the checkpoint in the browser process.
+  {
+    auto* node_env = electron::ElectronBrowserMainParts::Get()->node_env();
+    node::InternalCallbackScope microtasks_scope(
+        node_env->env(), v8::Local<v8::Object>(), {0, 0},
+        node::InternalCallbackScope::kAllowEmptyResource);
+  }
 }
 
 }  // namespace electron

shell/common/api/electron_bindings.cc

@@ -25,6 +25,7 @@
 #include "shell/common/gin_converters/file_path_converter.h"
 #include "shell/common/gin_helper/dictionary.h"
 #include "shell/common/gin_helper/locker.h"
+#include "shell/common/gin_helper/microtasks_scope.h"
 #include "shell/common/gin_helper/promise.h"
 #include "shell/common/heap_snapshot.h"
 #include "shell/common/node_includes.h"
@@ -271,8 +272,7 @@ void ElectronBindings::DidReceiveMemoryDump(
   v8::Isolate* isolate = promise.isolate();
   gin_helper::Locker locker(isolate);
   v8::HandleScope handle_scope(isolate);
-  v8::MicrotasksScope script_scope(isolate,
-                                   v8::MicrotasksScope::kRunMicrotasks);
+  gin_helper::MicrotasksScope microtasks_scope(isolate, true);
   v8::Context::Scope context_scope(
       v8::Local<v8::Context>::New(isolate, context));
 

shell/common/gin_helper/callback.h

@@ -13,7 +13,7 @@
 #include "shell/common/gin_converters/std_converter.h"
 #include "shell/common/gin_helper/function_template.h"
 #include "shell/common/gin_helper/locker.h"
-
+#include "shell/common/gin_helper/microtasks_scope.h"
 // Implements safe convertions between JS functions and base::Callback.
 
 namespace gin_helper {
@@ -48,8 +48,7 @@ struct V8FunctionInvoker<v8::Local<v8::Value>(ArgTypes...)> {
     v8::EscapableHandleScope handle_scope(isolate);
     if (!function.IsAlive())
       return v8::Null(isolate);
-    v8::MicrotasksScope script_scope(isolate,
-                                     v8::MicrotasksScope::kRunMicrotasks);
+    gin_helper::MicrotasksScope microtasks_scope(isolate, true);
     v8::Local<v8::Function> holder = function.NewHandle(isolate);
     v8::Local<v8::Context> context = holder->CreationContext();
     v8::Context::Scope context_scope(context);
@@ -73,8 +72,7 @@ struct V8FunctionInvoker<void(ArgTypes...)> {
     v8::HandleScope handle_scope(isolate);
     if (!function.IsAlive())
       return;
-    v8::MicrotasksScope script_scope(isolate,
-                                     v8::MicrotasksScope::kRunMicrotasks);
+    gin_helper::MicrotasksScope microtasks_scope(isolate, true);
     v8::Local<v8::Function> holder = function.NewHandle(isolate);
     v8::Local<v8::Context> context = holder->CreationContext();
     v8::Context::Scope context_scope(context);
@@ -97,8 +95,7 @@ struct V8FunctionInvoker<ReturnType(ArgTypes...)> {
     ReturnType ret = ReturnType();
     if (!function.IsAlive())
       return ret;
-    v8::MicrotasksScope script_scope(isolate,
-                                     v8::MicrotasksScope::kRunMicrotasks);
+    gin_helper::MicrotasksScope microtasks_scope(isolate, true);
     v8::Local<v8::Function> holder = function.NewHandle(isolate);
     v8::Local<v8::Context> context = holder->CreationContext();
     v8::Context::Scope context_scope(context);

shell/common/gin_helper/event_emitter_caller.cc

@@ -5,6 +5,7 @@
 #include "shell/common/gin_helper/event_emitter_caller.h"
 
 #include "shell/common/gin_helper/locker.h"
+#include "shell/common/gin_helper/microtasks_scope.h"
 #include "shell/common/node_includes.h"
 
 namespace gin_helper {
@@ -16,8 +17,7 @@ v8::Local<v8::Value> CallMethodWithArgs(v8::Isolate* isolate,
                                         const char* method,
                                         ValueVector* args) {
   // Perform microtask checkpoint after running JavaScript.
-  v8::MicrotasksScope script_scope(isolate,
-                                   v8::MicrotasksScope::kRunMicrotasks);
+  gin_helper::MicrotasksScope microtasks_scope(isolate, true);
   // Use node::MakeCallback to call the callback, and it will also run pending
   // tasks in Node.js.
   v8::MaybeLocal<v8::Value> ret = node::MakeCallback(

shell/common/gin_helper/function_template.h

@@ -14,6 +14,7 @@
 #include "shell/common/gin_helper/arguments.h"
 #include "shell/common/gin_helper/destroyable.h"
 #include "shell/common/gin_helper/error_thrower.h"
+#include "shell/common/gin_helper/microtasks_scope.h"
 
 // This file is forked from gin/function_template.h with 2 differences:
 // 1. Support for additional types of arguments.
@@ -214,8 +215,7 @@ class Invoker<IndicesHolder<indices...>, ArgTypes...>
 
   template <typename ReturnType>
   void DispatchToCallback(base::Callback<ReturnType(ArgTypes...)> callback) {
-    v8::MicrotasksScope script_scope(args_->isolate(),
-                                     v8::MicrotasksScope::kRunMicrotasks);
+    gin_helper::MicrotasksScope microtasks_scope(args_->isolate(), true);
     args_->Return(
         callback.Run(std::move(ArgumentHolder<indices, ArgTypes>::value)...));
   }
@@ -224,8 +224,7 @@ class Invoker<IndicesHolder<indices...>, ArgTypes...>
   // expression to foo. As a result, we must specialize the case of Callbacks
   // that have the void return type.
   void DispatchToCallback(base::Callback<void(ArgTypes...)> callback) {
-    v8::MicrotasksScope script_scope(args_->isolate(),
-                                     v8::MicrotasksScope::kRunMicrotasks);
+    gin_helper::MicrotasksScope microtasks_scope(args_->isolate(), true);
     callback.Run(std::move(ArgumentHolder<indices, ArgTypes>::value)...);
   }
 

shell/common/gin_helper/microtasks_scope.cc

@@ -0,0 +1,24 @@
+// Copyright (c) 2020 GitHub, Inc.
+// Use of this source code is governed by the MIT license that can be
+// found in the LICENSE file.
+
+#include "shell/common/gin_helper/microtasks_scope.h"
+
+#include "shell/common/gin_helper/locker.h"
+
+namespace gin_helper {
+
+MicrotasksScope::MicrotasksScope(v8::Isolate* isolate,
+                                 bool ignore_browser_checkpoint) {
+  if (Locker::IsBrowserProcess()) {
+    if (!ignore_browser_checkpoint)
+      v8::MicrotasksScope::PerformCheckpoint(isolate);
+  } else {
+    v8_microtasks_scope_ = std::make_unique<v8::MicrotasksScope>(
+        isolate, v8::MicrotasksScope::kRunMicrotasks);
+  }
+}
+
+MicrotasksScope::~MicrotasksScope() = default;
+
+}  // namespace gin_helper

shell/common/gin_helper/microtasks_scope.h

@@ -0,0 +1,31 @@
+// Copyright (c) 2020 GitHub, Inc.
+// Use of this source code is governed by the MIT license that can be
+// found in the LICENSE file.
+
+#ifndef SHELL_COMMON_GIN_HELPER_MICROTASKS_SCOPE_H_
+#define SHELL_COMMON_GIN_HELPER_MICROTASKS_SCOPE_H_
+
+#include <memory>
+
+#include "base/macros.h"
+#include "v8/include/v8.h"
+
+namespace gin_helper {
+
+// In the browser process runs v8::MicrotasksScope::PerformCheckpoint
+// In the render process creates a v8::MicrotasksScope.
+class MicrotasksScope {
+ public:
+  explicit MicrotasksScope(v8::Isolate* isolate,
+                           bool ignore_browser_checkpoint = false);
+  ~MicrotasksScope();
+
+ private:
+  std::unique_ptr<v8::MicrotasksScope> v8_microtasks_scope_;
+
+  DISALLOW_COPY_AND_ASSIGN(MicrotasksScope);
+};
+
+}  // namespace gin_helper
+
+#endif  // SHELL_COMMON_GIN_HELPER_MICROTASKS_SCOPE_H_

shell/common/gin_helper/promise.cc

@@ -26,8 +26,7 @@ PromiseBase& PromiseBase::operator=(PromiseBase&&) = default;
 v8::Maybe<bool> PromiseBase::Reject() {
   gin_helper::Locker locker(isolate());
   v8::HandleScope handle_scope(isolate());
-  v8::MicrotasksScope script_scope(isolate(),
-                                   v8::MicrotasksScope::kRunMicrotasks);
+  gin_helper::MicrotasksScope microtasks_scope(isolate());
   v8::Context::Scope context_scope(GetContext());
 
   return GetInner()->Reject(GetContext(), v8::Undefined(isolate()));
@@ -36,8 +35,7 @@ v8::Maybe<bool> PromiseBase::Reject() {
 v8::Maybe<bool> PromiseBase::Reject(v8::Local<v8::Value> except) {
   gin_helper::Locker locker(isolate());
   v8::HandleScope handle_scope(isolate());
-  v8::MicrotasksScope script_scope(isolate(),
-                                   v8::MicrotasksScope::kRunMicrotasks);
+  gin_helper::MicrotasksScope microtasks_scope(isolate());
   v8::Context::Scope context_scope(GetContext());
 
   return GetInner()->Reject(GetContext(), except);
@@ -46,8 +44,7 @@ v8::Maybe<bool> PromiseBase::Reject(v8::Local<v8::Value> except) {
 v8::Maybe<bool> PromiseBase::RejectWithErrorMessage(base::StringPiece message) {
   gin_helper::Locker locker(isolate());
   v8::HandleScope handle_scope(isolate());
-  v8::MicrotasksScope script_scope(isolate(),
-                                   v8::MicrotasksScope::kRunMicrotasks);
+  gin_helper::MicrotasksScope microtasks_scope(isolate());
   v8::Context::Scope context_scope(GetContext());
 
   v8::Local<v8::Value> error =
@@ -90,8 +87,7 @@ v8::Local<v8::Promise> Promise<void>::ResolvedPromise(v8::Isolate* isolate) {
 v8::Maybe<bool> Promise<void>::Resolve() {
   gin_helper::Locker locker(isolate());
   v8::HandleScope handle_scope(isolate());
-  v8::MicrotasksScope script_scope(isolate(),
-                                   v8::MicrotasksScope::kRunMicrotasks);
+  gin_helper::MicrotasksScope microtasks_scope(isolate());
   v8::Context::Scope context_scope(GetContext());
 
   return GetInner()->Resolve(GetContext(), v8::Undefined(isolate()));

shell/common/gin_helper/promise.h

@@ -16,6 +16,7 @@
 #include "content/public/browser/browser_thread.h"
 #include "shell/common/gin_converters/std_converter.h"
 #include "shell/common/gin_helper/locker.h"
+#include "shell/common/gin_helper/microtasks_scope.h"
 
 namespace gin_helper {
 
@@ -110,8 +111,7 @@ class Promise : public PromiseBase {
   v8::Maybe<bool> Resolve(const RT& value) {
     gin_helper::Locker locker(isolate());
     v8::HandleScope handle_scope(isolate());
-    v8::MicrotasksScope script_scope(isolate(),
-                                     v8::MicrotasksScope::kRunMicrotasks);
+    gin_helper::MicrotasksScope microtasks_scope(isolate());
     v8::Context::Scope context_scope(GetContext());
 
     return GetInner()->Resolve(GetContext(),

shell/common/node_bindings.cc

@@ -28,6 +28,7 @@
 #include "shell/common/gin_helper/dictionary.h"
 #include "shell/common/gin_helper/event_emitter_caller.h"
 #include "shell/common/gin_helper/locker.h"
+#include "shell/common/gin_helper/microtasks_scope.h"
 #include "shell/common/mac/main_application_bundle.h"
 #include "shell/common/node_includes.h"
 
@@ -412,8 +413,7 @@ void NodeBindings::UvRunOnce() {
   v8::Context::Scope context_scope(env->context());
 
   // Perform microtask checkpoint after running JavaScript.
-  v8::MicrotasksScope script_scope(env->isolate(),
-                                   v8::MicrotasksScope::kRunMicrotasks);
+  gin_helper::MicrotasksScope microtasks_scope(env->isolate());
 
   if (browser_env_ != BrowserEnvironment::BROWSER)
     TRACE_EVENT_BEGIN0("devtools.timeline", "FunctionCall");

spec-main/api-session-spec.ts

@@ -293,6 +293,8 @@ describe('session module', () => {
       const { item, itemUrl, itemFilename } = await downloadPrevented;
       expect(itemUrl).to.equal(url);
       expect(itemFilename).to.equal('mockFile.txt');
+      // Delay till the next tick.
+      await new Promise(resolve => setImmediate(() => resolve()));
       expect(() => item.getURL()).to.throw('Object has been destroyed');
     });
   });

spec-main/api-web-contents-spec.ts

@@ -591,28 +591,50 @@ describe('webContents module', () => {
   // On Mac, zooming isn't done with the mouse wheel.
   ifdescribe(process.platform !== 'darwin')('zoom-changed', () => {
     afterEach(closeAllWindows);
-    it('is emitted with the correct zooming info', async () => {
+    it('is emitted with the correct zoom-in info', async () => {
       const w = new BrowserWindow({ show: false });
       await w.loadFile(path.join(fixturesPath, 'pages', 'base-page.html'));
 
-      const testZoomChanged = async ({ zoomingIn }: { zoomingIn: boolean }) => {
+      const testZoomChanged = async () => {
         w.webContents.sendInputEvent({
           type: 'mouseWheel',
           x: 300,
           y: 300,
           deltaX: 0,
-          deltaY: zoomingIn ? 1 : -1,
+          deltaY: 1,
           wheelTicksX: 0,
-          wheelTicksY: zoomingIn ? 1 : -1,
+          wheelTicksY: 1,
           modifiers: ['control', 'meta']
         });
 
         const [, zoomDirection] = await emittedOnce(w.webContents, 'zoom-changed');
-        expect(zoomDirection).to.equal(zoomingIn ? 'in' : 'out');
+        expect(zoomDirection).to.equal('in');
       };
 
-      await testZoomChanged({ zoomingIn: true });
-      await testZoomChanged({ zoomingIn: false });
+      await testZoomChanged();
+    });
+
+    it('is emitted with the correct zoom-out info', async () => {
+      const w = new BrowserWindow({ show: false });
+      await w.loadFile(path.join(fixturesPath, 'pages', 'base-page.html'));
+
+      const testZoomChanged = async () => {
+        w.webContents.sendInputEvent({
+          type: 'mouseWheel',
+          x: 300,
+          y: 300,
+          deltaX: 0,
+          deltaY: -1,
+          wheelTicksX: 0,
+          wheelTicksY: -1,
+          modifiers: ['control', 'meta']
+        });
+
+        const [, zoomDirection] = await emittedOnce(w.webContents, 'zoom-changed');
+        expect(zoomDirection).to.equal('out');
+      };
+
+      await testZoomChanged();
     });
   });
 

spec-main/node-spec.ts

@@ -273,4 +273,18 @@ describe('node feature', () => {
       done();
     });
   });
+
+  it('performs microtask checkpoint correctly', (done) => {
+    const f3 = async () => {
+      return new Promise((resolve, reject) => {
+        reject(new Error('oops'));
+      });
+    };
+
+    process.once('unhandledRejection', () => done('catch block is delayed to next tick'));
+
+    setTimeout(() => {
+      f3().catch(() => done());
+    });
+  });
 });