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

…4131) * 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
electron · Jun 17, 2020 · 79d3f6a · 79d3f6a
1 parent 0b846b2
commit 79d3f6a
Show file tree

Hide file tree

Showing 19 changed files with 149 additions and 51 deletions.
diff --git a/native_mate/BUILD.gn b/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",

diff --git a/native_mate/native_mate/function_template.h b/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)...);
   }
 

diff --git a/native_mate/native_mate/microtasks_scope.cc b/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
diff --git a/native_mate/native_mate/microtasks_scope.h b/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_
diff --git a/shell/browser/api/electron_api_url_loader.cc b/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)) {

diff --git a/shell/browser/electron_browser_main_parts.h b/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:

diff --git a/shell/browser/javascript_environment.h b/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_;
 

diff --git a/shell/browser/microtasks_runner.cc b/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
diff --git a/shell/common/api/electron_bindings.cc b/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));
 

diff --git a/shell/common/api/event_emitter_caller_deprecated.cc b/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(

diff --git a/shell/common/gin_helper/callback.h b/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);

diff --git a/shell/common/gin_helper/event_emitter_caller.cc b/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(

diff --git a/shell/common/gin_helper/function_template.h b/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)...);
   }
 

diff --git a/shell/common/native_mate_converters/callback_converter_deprecated.h b/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);

diff --git a/shell/common/node_bindings.cc b/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");

diff --git a/shell/common/promise_util.h b/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()));
 

diff --git a/spec-main/api-session-spec.ts b/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')
     })
   })