From 7cc780d077d1aa950b93db7954d1e321ac88e7f6 Mon Sep 17 00:00:00 2001 From: Robo Date: Wed, 17 Jun 2020 10:08:10 -0700 Subject: [PATCH] fix: let Node.js perform microtask checkpoint in the main process (#24131) * 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 | 2 ++ shell/browser/api/electron_api_url_loader.cc | 2 -- shell/browser/electron_browser_main_parts.h | 1 + shell/browser/javascript_environment.h | 2 ++ shell/browser/microtasks_runner.cc | 20 ++++++++++- shell/common/api/electron_bindings.cc | 4 +-- shell/common/gin_helper/callback.h | 11 +++--- .../common/gin_helper/event_emitter_caller.cc | 4 +-- shell/common/gin_helper/function_template.h | 7 ++-- shell/common/gin_helper/microtasks_scope.cc | 24 +++++++++++++ shell/common/gin_helper/microtasks_scope.h | 31 ++++++++++++++++ shell/common/gin_helper/promise.cc | 12 +++---- shell/common/gin_helper/promise.h | 4 +-- shell/common/node_bindings.cc | 4 +-- spec-main/api-session-spec.ts | 2 ++ spec-main/api-web-contents-spec.ts | 36 +++++++++++++++---- spec-main/node-spec.ts | 14 ++++++++ spec/fixtures/api/gpu-info.js | 4 +-- 18 files changed, 145 insertions(+), 39 deletions(-) create mode 100644 shell/common/gin_helper/microtasks_scope.cc create mode 100644 shell/common/gin_helper/microtasks_scope.h diff --git a/filenames.gni b/filenames.gni index 6fbe0ec51269..3984417736bf 100644 --- a/filenames.gni +++ b/filenames.gni @@ -525,6 +525,8 @@ filenames = { "shell/common/gin_helper/function_template_extensions.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", diff --git a/shell/browser/api/electron_api_url_loader.cc b/shell/browser/api/electron_api_url_loader.cc index ac5a6cea1d40..2758e0ddf7c7 100644 --- a/shell/browser/api/electron_api_url_loader.cc +++ b/shell/browser/api/electron_api_url_loader.cc @@ -135,8 +135,6 @@ class JSChunkedDataPipeGetter : public gin::Wrappable, data_producer_ = std::make_unique(std::move(pipe)); v8::HandleScope handle_scope(isolate_); - v8::MicrotasksScope script_scope(isolate_, - v8::MicrotasksScope::kRunMicrotasks); auto maybe_wrapper = GetWrapper(isolate_); v8::Local wrapper; if (!maybe_wrapper.ToLocal(&wrapper)) { diff --git a/shell/browser/electron_browser_main_parts.h b/shell/browser/electron_browser_main_parts.h index b2f36da53641..446e3e1c2474 100644 --- a/shell/browser/electron_browser_main_parts.h +++ b/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: diff --git a/shell/browser/javascript_environment.h b/shell/browser/javascript_environment.h index 17ce402d3a77..65fb113fd466 100644 --- a/shell/browser/javascript_environment.h +++ b/shell/browser/javascript_environment.h @@ -57,6 +57,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 index 9c20bc827eb9..94af2a9f66bc 100644 --- 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(), {0, 0}, + node::InternalCallbackScope::kAllowEmptyResource); + } } } // namespace electron diff --git a/shell/common/api/electron_bindings.cc b/shell/common/api/electron_bindings.cc index 0b23950f9157..9f43a874a352 100644 --- a/shell/common/api/electron_bindings.cc +++ b/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::New(isolate, context)); diff --git a/shell/common/gin_helper/callback.h b/shell/common/gin_helper/callback.h index a112f9731af5..3221dd2b6ba8 100644 --- a/shell/common/gin_helper/callback.h +++ b/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(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 holder = function.NewHandle(isolate); v8::Local context = holder->CreationContext(); v8::Context::Scope context_scope(context); @@ -73,8 +72,7 @@ struct V8FunctionInvoker { 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 holder = function.NewHandle(isolate); v8::Local context = holder->CreationContext(); v8::Context::Scope context_scope(context); @@ -97,8 +95,7 @@ struct V8FunctionInvoker { 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 holder = function.NewHandle(isolate); v8::Local 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 index 515f055b186e..8676732d3240 100644 --- a/shell/common/gin_helper/event_emitter_caller.cc +++ b/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 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 ret = node::MakeCallback( diff --git a/shell/common/gin_helper/function_template.h b/shell/common/gin_helper/function_template.h index 1ef0979c20a7..def4d2151ea4 100644 --- a/shell/common/gin_helper/function_template.h +++ b/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, ArgTypes...> template void DispatchToCallback(base::Callback 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::value)...)); } @@ -224,8 +224,7 @@ class Invoker, ArgTypes...> // expression to foo. As a result, we must specialize the case of Callbacks // that have the void return type. void DispatchToCallback(base::Callback callback) { - v8::MicrotasksScope script_scope(args_->isolate(), - v8::MicrotasksScope::kRunMicrotasks); + gin_helper::MicrotasksScope microtasks_scope(args_->isolate(), true); callback.Run(std::move(ArgumentHolder::value)...); } diff --git a/shell/common/gin_helper/microtasks_scope.cc b/shell/common/gin_helper/microtasks_scope.cc new file mode 100644 index 000000000000..06d67b8e9878 --- /dev/null +++ b/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( + isolate, v8::MicrotasksScope::kRunMicrotasks); + } +} + +MicrotasksScope::~MicrotasksScope() = default; + +} // namespace gin_helper diff --git a/shell/common/gin_helper/microtasks_scope.h b/shell/common/gin_helper/microtasks_scope.h new file mode 100644 index 000000000000..aaa6aaf5d4e9 --- /dev/null +++ b/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 + +#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_microtasks_scope_; + + DISALLOW_COPY_AND_ASSIGN(MicrotasksScope); +}; + +} // namespace gin_helper + +#endif // SHELL_COMMON_GIN_HELPER_MICROTASKS_SCOPE_H_ diff --git a/shell/common/gin_helper/promise.cc b/shell/common/gin_helper/promise.cc index 32737c18ab2b..e17881ef2c4a 100644 --- a/shell/common/gin_helper/promise.cc +++ b/shell/common/gin_helper/promise.cc @@ -26,8 +26,7 @@ PromiseBase& PromiseBase::operator=(PromiseBase&&) = default; v8::Maybe 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 PromiseBase::Reject() { v8::Maybe PromiseBase::Reject(v8::Local 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 PromiseBase::Reject(v8::Local except) { v8::Maybe 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 error = @@ -90,8 +87,7 @@ v8::Local Promise::ResolvedPromise(v8::Isolate* isolate) { v8::Maybe Promise::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())); diff --git a/shell/common/gin_helper/promise.h b/shell/common/gin_helper/promise.h index 813eef4665b2..329756ad231d 100644 --- a/shell/common/gin_helper/promise.h +++ b/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 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(), diff --git a/shell/common/node_bindings.cc b/shell/common/node_bindings.cc index b76c085dcc25..0646ffca9553 100644 --- a/shell/common/node_bindings.cc +++ b/shell/common/node_bindings.cc @@ -29,6 +29,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" @@ -475,8 +476,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"); diff --git a/spec-main/api-session-spec.ts b/spec-main/api-session-spec.ts index 278134b467c1..25089e970112 100644 --- a/spec-main/api-session-spec.ts +++ b/spec-main/api-session-spec.ts @@ -288,6 +288,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('DownloadItem used after being destroyed'); }); }); diff --git a/spec-main/api-web-contents-spec.ts b/spec-main/api-web-contents-spec.ts index 8fe84705976e..5f16a28df8a7 100644 --- a/spec-main/api-web-contents-spec.ts +++ b/spec-main/api-web-contents-spec.ts @@ -597,28 +597,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(); }); }); diff --git a/spec-main/node-spec.ts b/spec-main/node-spec.ts index a3e4af96897a..ea718462037a 100644 --- a/spec-main/node-spec.ts +++ b/spec-main/node-spec.ts @@ -323,4 +323,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()); + }); + }); }); diff --git a/spec/fixtures/api/gpu-info.js b/spec/fixtures/api/gpu-info.js index 3e7bc88b02d8..dc75c64cac2a 100644 --- a/spec/fixtures/api/gpu-info.js +++ b/spec/fixtures/api/gpu-info.js @@ -9,11 +9,11 @@ app.whenReady().then(() => { app.getGPUInfo(infoType).then( (gpuInfo) => { console.log(JSON.stringify(gpuInfo)); - app.exit(0); + setImmediate(() => app.exit(0)); }, (error) => { console.error(error); - app.exit(1); + setImmediate(() => app.exit(1)); } ); });