fix: let Node.js perform microtask checkpoint in the main process (#24131) (#24180)

* 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

native_mate/BUILD.gn

@@ -23,6 +23,8 @@ source_set("native_mate") {
     "native_mate/function_template.cc",
     "native_mate/function_template.h",
     "native_mate/handle.h",
+    "native_mate/microtasks_scope.cc",
+    "native_mate/microtasks_scope.h",
     "native_mate/object_template_builder.cc",
     "native_mate/object_template_builder_deprecated.h",
     "native_mate/persistent_dictionary.cc",

native_mate/native_mate/function_template.h

@@ -9,6 +9,7 @@
 
 #include "base/callback.h"
 #include "native_mate/arguments.h"
+#include "native_mate/microtasks_scope.h"
 #include "native_mate/wrappable_base.h"
 #include "shell/common/gin_helper/destroyable.h"
 #include "shell/common/gin_helper/error_thrower.h"
@@ -194,8 +195,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);
+    mate::MicrotasksScope microtasks_scope(args_->isolate(), true);
     args_->Return(
         callback.Run(std::move(ArgumentHolder<indices, ArgTypes>::value)...));
   }
@@ -204,8 +204,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);
+    mate::MicrotasksScope microtasks_scope(args_->isolate(), true);
     callback.Run(std::move(ArgumentHolder<indices, ArgTypes>::value)...);
   }
 

native_mate/native_mate/microtasks_scope.cc

@@ -0,0 +1,22 @@
+// 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 "native_mate/microtasks_scope.h"
+
+namespace mate {
+
+MicrotasksScope::MicrotasksScope(v8::Isolate* isolate,
+                                 bool ignore_browser_checkpoint) {
+  if (v8::Locker::IsActive()) {
+    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 mate

native_mate/native_mate/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 NATIVE_MATE_NATIVE_MATE_MICROTASKS_SCOPE_H_
+#define NATIVE_MATE_NATIVE_MATE_MICROTASKS_SCOPE_H_
+
+#include <memory>
+
+#include "base/macros.h"
+#include "v8/include/v8.h"
+
+namespace mate {
+
+// 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 mate
+
+#endif  // NATIVE_MATE_NATIVE_MATE_MICROTASKS_SCOPE_H_

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

@@ -84,6 +84,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

@@ -273,8 +273,6 @@ void ElectronBindings::DidReceiveMemoryDump(
   v8::Isolate* isolate = promise.isolate();
   mate::Locker locker(isolate);
   v8::HandleScope handle_scope(isolate);
-  v8::MicrotasksScope script_scope(isolate,
-                                   v8::MicrotasksScope::kRunMicrotasks);
   v8::Context::Scope context_scope(
       v8::Local<v8::Context>::New(isolate, context));
 

shell/common/api/event_emitter_caller_deprecated.cc

@@ -4,6 +4,7 @@
 
 #include "shell/common/api/event_emitter_caller_deprecated.h"
 
+#include "native_mate/microtasks_scope.h"
 #include "shell/common/api/locker.h"
 #include "shell/common/node_includes.h"
 
@@ -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);
+  mate::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/callback.h

@@ -9,10 +9,10 @@
 #include <vector>
 
 #include "base/bind.h"
+#include "native_mate/microtasks_scope.h"
 #include "shell/common/api/locker.h"
 #include "shell/common/gin_converters/std_converter.h"
 #include "shell/common/gin_helper/function_template.h"
-
 // Implements safe convertions between JS functions and base::Callback.
 
 namespace gin_helper {
@@ -47,8 +47,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);
+    mate::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);
@@ -72,8 +71,7 @@ struct V8FunctionInvoker<void(ArgTypes...)> {
     v8::HandleScope handle_scope(isolate);
     if (!function.IsAlive())
       return;
-    v8::MicrotasksScope script_scope(isolate,
-                                     v8::MicrotasksScope::kRunMicrotasks);
+    mate::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);
@@ -96,8 +94,7 @@ struct V8FunctionInvoker<ReturnType(ArgTypes...)> {
     ReturnType ret = ReturnType();
     if (!function.IsAlive())
       return ret;
-    v8::MicrotasksScope script_scope(isolate,
-                                     v8::MicrotasksScope::kRunMicrotasks);
+    mate::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

@@ -4,6 +4,7 @@
 
 #include "shell/common/gin_helper/event_emitter_caller.h"
 
+#include "native_mate/microtasks_scope.h"
 #include "shell/common/api/locker.h"
 #include "shell/common/node_includes.h"
 
@@ -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);
+  mate::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

@@ -11,6 +11,7 @@
 #include "base/callback.h"
 #include "base/optional.h"
 #include "gin/arguments.h"
+#include "native_mate/microtasks_scope.h"
 #include "shell/common/gin_helper/arguments.h"
 #include "shell/common/gin_helper/destroyable.h"
 #include "shell/common/gin_helper/error_thrower.h"
@@ -213,8 +214,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);
+    mate::MicrotasksScope microtasks_scope(args_->isolate(), true);
     args_->Return(
         callback.Run(std::move(ArgumentHolder<indices, ArgTypes>::value)...));
   }
@@ -223,8 +223,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);
+    mate::MicrotasksScope microtasks_scope(args_->isolate(), true);
     callback.Run(std::move(ArgumentHolder<indices, ArgTypes>::value)...);
   }
 

shell/common/native_mate_converters/callback_converter_deprecated.h

@@ -13,6 +13,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "native_mate/function_template.h"
+#include "native_mate/microtasks_scope.h"
 #include "native_mate/scoped_persistent.h"
 #include "shell/common/api/locker.h"
 
@@ -54,8 +55,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);
+    mate::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);
@@ -79,8 +79,7 @@ struct V8FunctionInvoker<void(ArgTypes...)> {
     v8::HandleScope handle_scope(isolate);
     if (!function.IsAlive())
       return;
-    v8::MicrotasksScope script_scope(isolate,
-                                     v8::MicrotasksScope::kRunMicrotasks);
+    mate::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);
@@ -103,8 +102,7 @@ struct V8FunctionInvoker<ReturnType(ArgTypes...)> {
     ReturnType ret = ReturnType();
     if (!function.IsAlive())
       return ret;
-    v8::MicrotasksScope script_scope(isolate,
-                                     v8::MicrotasksScope::kRunMicrotasks);
+    mate::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/node_bindings.cc

@@ -23,6 +23,7 @@
 #include "content/public/browser/browser_thread.h"
 #include "content/public/common/content_paths.h"
 #include "electron/buildflags/buildflags.h"
+#include "native_mate/microtasks_scope.h"
 #include "shell/common/api/locker.h"
 #include "shell/common/electron_command_line.h"
 #include "shell/common/gin_converters/file_path_converter.h"
@@ -410,8 +411,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);
+  mate::MicrotasksScope microtasks_scope(env->isolate());
 
   if (browser_env_ != BrowserEnvironment::BROWSER)
     TRACE_EVENT_BEGIN0("devtools.timeline", "FunctionCall");

shell/common/promise_util.h

@@ -15,6 +15,7 @@
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/browser_thread.h"
 #include "native_mate/converter.h"
+#include "native_mate/microtasks_scope.h"
 #include "shell/common/gin_converters/std_converter.h"
 
 namespace electron {
@@ -109,8 +110,7 @@ class Promise {
     static_assert(std::is_same<void*, RT>(),
                   "Can only resolve void* promises with no value");
     v8::HandleScope handle_scope(isolate());
-    v8::MicrotasksScope script_scope(isolate(),
-                                     v8::MicrotasksScope::kRunMicrotasks);
+    mate::MicrotasksScope microtasks_scope(isolate());
     v8::Context::Scope context_scope(
         v8::Local<v8::Context>::New(isolate(), GetContext()));
 
@@ -119,8 +119,7 @@ class Promise {
 
   v8::Maybe<bool> Reject() {
     v8::HandleScope handle_scope(isolate());
-    v8::MicrotasksScope script_scope(isolate(),
-                                     v8::MicrotasksScope::kRunMicrotasks);
+    mate::MicrotasksScope microtasks_scope(isolate());
     v8::Context::Scope context_scope(
         v8::Local<v8::Context>::New(isolate(), GetContext()));
 
@@ -129,8 +128,7 @@ class Promise {
 
   v8::Maybe<bool> Reject(v8::Local<v8::Value> exception) {
     v8::HandleScope handle_scope(isolate());
-    v8::MicrotasksScope script_scope(isolate(),
-                                     v8::MicrotasksScope::kRunMicrotasks);
+    mate::MicrotasksScope microtasks_scope(isolate());
     v8::Context::Scope context_scope(
         v8::Local<v8::Context>::New(isolate(), GetContext()));
 
@@ -163,8 +161,7 @@ class Promise {
     static_assert(!std::is_same<void*, RT>(),
                   "void* promises can not be resolved with a value");
     v8::HandleScope handle_scope(isolate());
-    v8::MicrotasksScope script_scope(isolate(),
-                                     v8::MicrotasksScope::kRunMicrotasks);
+    mate::MicrotasksScope microtasks_scope(isolate());
     v8::Context::Scope context_scope(
         v8::Local<v8::Context>::New(isolate(), GetContext()));
 
@@ -176,8 +173,7 @@ class Promise {
     static_assert(!std::is_same<void*, RT>(),
                   "void* promises can not be resolved with a value");
     v8::HandleScope handle_scope(isolate());
-    v8::MicrotasksScope script_scope(isolate(),
-                                     v8::MicrotasksScope::kRunMicrotasks);
+    mate::MicrotasksScope microtasks_scope(isolate());
     v8::Context::Scope context_scope(
         v8::Local<v8::Context>::New(isolate(), GetContext()));
 
@@ -187,8 +183,7 @@ class Promise {
 
   v8::Maybe<bool> RejectWithErrorMessage(base::StringPiece string) {
     v8::HandleScope handle_scope(isolate());
-    v8::MicrotasksScope script_scope(isolate(),
-                                     v8::MicrotasksScope::kRunMicrotasks);
+    mate::MicrotasksScope microtasks_scope(isolate());
     v8::Context::Scope context_scope(
         v8::Local<v8::Context>::New(isolate(), GetContext()));
 

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

@@ -588,28 +588,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

@@ -203,4 +203,18 @@ describe('node feature', () => {
     const result = childProcess.spawnSync(process.execPath, [path.resolve(fixtures, 'api', 'electron-main-module', 'app.asar')])
     expect(result.status).to.equal(0)
   })
+
+  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())
+    })
+  })
 })