Skip to content

Commit 64cdfda

Browse files
trop[bot]deepak1556
trop[bot]
and
deepak1556
authored
fix: let Node.js perform microtask checkpoint in the main process (electron#24174)
* 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 Co-authored-by: deepak1556 <hop2deep@gmail.com>
1 parent 9f8f82a commit 64cdfda

18 files changed

+145
-39
lines changed

‎filenames.gni

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -525,6 +525,8 @@ filenames = {
525525
"shell/common/gin_helper/function_template_extensions.h",
526526
"shell/common/gin_helper/locker.cc",
527527
"shell/common/gin_helper/locker.h",
528+
"shell/common/gin_helper/microtasks_scope.cc",
529+
"shell/common/gin_helper/microtasks_scope.h",
528530
"shell/common/gin_helper/object_template_builder.cc",
529531
"shell/common/gin_helper/object_template_builder.h",
530532
"shell/common/gin_helper/persistent_dictionary.cc",

‎shell/browser/api/electron_api_url_loader.cc

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -135,8 +135,6 @@ class JSChunkedDataPipeGetter : public gin::Wrappable<JSChunkedDataPipeGetter>,
135135
data_producer_ = std::make_unique<mojo::DataPipeProducer>(std::move(pipe));
136136

137137
v8::HandleScope handle_scope(isolate_);
138-
v8::MicrotasksScope script_scope(isolate_,
139-
v8::MicrotasksScope::kRunMicrotasks);
140138
auto maybe_wrapper = GetWrapper(isolate_);
141139
v8::Local<v8::Value> wrapper;
142140
if (!maybe_wrapper.ToLocal(&wrapper)) {

‎shell/browser/electron_browser_main_parts.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,7 @@ class ElectronBrowserMainParts : public content::BrowserMainParts {
9090

9191
Browser* browser() { return browser_.get(); }
9292
BrowserProcessImpl* browser_process() { return fake_browser_process_.get(); }
93+
NodeEnvironment* node_env() { return node_env_.get(); }
9394

9495
protected:
9596
// content::BrowserMainParts:

‎shell/browser/javascript_environment.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,8 @@ class NodeEnvironment {
5757
explicit NodeEnvironment(node::Environment* env);
5858
~NodeEnvironment();
5959

60+
node::Environment* env() { return env_; }
61+
6062
private:
6163
node::Environment* env_;
6264

‎shell/browser/microtasks_runner.cc

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,10 @@
44
// found in the LICENSE file.
55

66
#include "shell/browser/microtasks_runner.h"
7+
8+
#include "shell/browser/electron_browser_main_parts.h"
9+
#include "shell/browser/javascript_environment.h"
10+
#include "shell/common/node_includes.h"
711
#include "v8/include/v8.h"
812

913
namespace electron {
@@ -15,7 +19,21 @@ void MicrotasksRunner::WillProcessTask(const base::PendingTask& pending_task,
1519

1620
void MicrotasksRunner::DidProcessTask(const base::PendingTask& pending_task) {
1721
v8::Isolate::Scope scope(isolate_);
18-
v8::MicrotasksScope::PerformCheckpoint(isolate_);
22+
// In the browser process we follow Node.js microtask policy of kExplicit
23+
// and let the MicrotaskRunner which is a task observer for chromium UI thread
24+
// scheduler run the micotask checkpoint. This worked fine because Node.js
25+
// also runs microtasks through its task queue, but after
26+
// https://github.com/electron/electron/issues/20013 Node.js now performs its
27+
// own microtask checkpoint and it may happen is some situations that there is
28+
// contention for performing checkpoint between Node.js and chromium, ending
29+
// up Node.js dealying its callbacks. To fix this, now we always lets Node.js
30+
// handle the checkpoint in the browser process.
31+
{
32+
auto* node_env = electron::ElectronBrowserMainParts::Get()->node_env();
33+
node::InternalCallbackScope microtasks_scope(
34+
node_env->env(), v8::Local<v8::Object>(), {0, 0},
35+
node::InternalCallbackScope::kAllowEmptyResource);
36+
}
1937
}
2038

2139
} // namespace electron

‎shell/common/api/electron_bindings.cc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
#include "shell/common/gin_converters/file_path_converter.h"
2626
#include "shell/common/gin_helper/dictionary.h"
2727
#include "shell/common/gin_helper/locker.h"
28+
#include "shell/common/gin_helper/microtasks_scope.h"
2829
#include "shell/common/gin_helper/promise.h"
2930
#include "shell/common/heap_snapshot.h"
3031
#include "shell/common/node_includes.h"
@@ -271,8 +272,7 @@ void ElectronBindings::DidReceiveMemoryDump(
271272
v8::Isolate* isolate = promise.isolate();
272273
gin_helper::Locker locker(isolate);
273274
v8::HandleScope handle_scope(isolate);
274-
v8::MicrotasksScope script_scope(isolate,
275-
v8::MicrotasksScope::kRunMicrotasks);
275+
gin_helper::MicrotasksScope microtasks_scope(isolate, true);
276276
v8::Context::Scope context_scope(
277277
v8::Local<v8::Context>::New(isolate, context));
278278

‎shell/common/gin_helper/callback.h

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
#include "shell/common/gin_converters/std_converter.h"
1414
#include "shell/common/gin_helper/function_template.h"
1515
#include "shell/common/gin_helper/locker.h"
16-
16+
#include "shell/common/gin_helper/microtasks_scope.h"
1717
// Implements safe convertions between JS functions and base::Callback.
1818

1919
namespace gin_helper {
@@ -48,8 +48,7 @@ struct V8FunctionInvoker<v8::Local<v8::Value>(ArgTypes...)> {
4848
v8::EscapableHandleScope handle_scope(isolate);
4949
if (!function.IsAlive())
5050
return v8::Null(isolate);
51-
v8::MicrotasksScope script_scope(isolate,
52-
v8::MicrotasksScope::kRunMicrotasks);
51+
gin_helper::MicrotasksScope microtasks_scope(isolate, true);
5352
v8::Local<v8::Function> holder = function.NewHandle(isolate);
5453
v8::Local<v8::Context> context = holder->CreationContext();
5554
v8::Context::Scope context_scope(context);
@@ -73,8 +72,7 @@ struct V8FunctionInvoker<void(ArgTypes...)> {
7372
v8::HandleScope handle_scope(isolate);
7473
if (!function.IsAlive())
7574
return;
76-
v8::MicrotasksScope script_scope(isolate,
77-
v8::MicrotasksScope::kRunMicrotasks);
75+
gin_helper::MicrotasksScope microtasks_scope(isolate, true);
7876
v8::Local<v8::Function> holder = function.NewHandle(isolate);
7977
v8::Local<v8::Context> context = holder->CreationContext();
8078
v8::Context::Scope context_scope(context);
@@ -97,8 +95,7 @@ struct V8FunctionInvoker<ReturnType(ArgTypes...)> {
9795
ReturnType ret = ReturnType();
9896
if (!function.IsAlive())
9997
return ret;
100-
v8::MicrotasksScope script_scope(isolate,
101-
v8::MicrotasksScope::kRunMicrotasks);
98+
gin_helper::MicrotasksScope microtasks_scope(isolate, true);
10299
v8::Local<v8::Function> holder = function.NewHandle(isolate);
103100
v8::Local<v8::Context> context = holder->CreationContext();
104101
v8::Context::Scope context_scope(context);

‎shell/common/gin_helper/event_emitter_caller.cc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
#include "shell/common/gin_helper/event_emitter_caller.h"
66

77
#include "shell/common/gin_helper/locker.h"
8+
#include "shell/common/gin_helper/microtasks_scope.h"
89
#include "shell/common/node_includes.h"
910

1011
namespace gin_helper {
@@ -16,8 +17,7 @@ v8::Local<v8::Value> CallMethodWithArgs(v8::Isolate* isolate,
1617
const char* method,
1718
ValueVector* args) {
1819
// Perform microtask checkpoint after running JavaScript.
19-
v8::MicrotasksScope script_scope(isolate,
20-
v8::MicrotasksScope::kRunMicrotasks);
20+
gin_helper::MicrotasksScope microtasks_scope(isolate, true);
2121
// Use node::MakeCallback to call the callback, and it will also run pending
2222
// tasks in Node.js.
2323
v8::MaybeLocal<v8::Value> ret = node::MakeCallback(

‎shell/common/gin_helper/function_template.h

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
#include "shell/common/gin_helper/arguments.h"
1515
#include "shell/common/gin_helper/destroyable.h"
1616
#include "shell/common/gin_helper/error_thrower.h"
17+
#include "shell/common/gin_helper/microtasks_scope.h"
1718

1819
// This file is forked from gin/function_template.h with 2 differences:
1920
// 1. Support for additional types of arguments.
@@ -214,8 +215,7 @@ class Invoker<IndicesHolder<indices...>, ArgTypes...>
214215

215216
template <typename ReturnType>
216217
void DispatchToCallback(base::Callback<ReturnType(ArgTypes...)> callback) {
217-
v8::MicrotasksScope script_scope(args_->isolate(),
218-
v8::MicrotasksScope::kRunMicrotasks);
218+
gin_helper::MicrotasksScope microtasks_scope(args_->isolate(), true);
219219
args_->Return(
220220
callback.Run(std::move(ArgumentHolder<indices, ArgTypes>::value)...));
221221
}
@@ -224,8 +224,7 @@ class Invoker<IndicesHolder<indices...>, ArgTypes...>
224224
// expression to foo. As a result, we must specialize the case of Callbacks
225225
// that have the void return type.
226226
void DispatchToCallback(base::Callback<void(ArgTypes...)> callback) {
227-
v8::MicrotasksScope script_scope(args_->isolate(),
228-
v8::MicrotasksScope::kRunMicrotasks);
227+
gin_helper::MicrotasksScope microtasks_scope(args_->isolate(), true);
229228
callback.Run(std::move(ArgumentHolder<indices, ArgTypes>::value)...);
230229
}
231230

‎shell/common/gin_helper/microtasks_scope.cc

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
// Copyright (c) 2020 GitHub, Inc.
2+
// Use of this source code is governed by the MIT license that can be
3+
// found in the LICENSE file.
4+
5+
#include "shell/common/gin_helper/microtasks_scope.h"
6+
7+
#include "shell/common/gin_helper/locker.h"
8+
9+
namespace gin_helper {
10+
11+
MicrotasksScope::MicrotasksScope(v8::Isolate* isolate,
12+
bool ignore_browser_checkpoint) {
13+
if (Locker::IsBrowserProcess()) {
14+
if (!ignore_browser_checkpoint)
15+
v8::MicrotasksScope::PerformCheckpoint(isolate);
16+
} else {
17+
v8_microtasks_scope_ = std::make_unique<v8::MicrotasksScope>(
18+
isolate, v8::MicrotasksScope::kRunMicrotasks);
19+
}
20+
}
21+
22+
MicrotasksScope::~MicrotasksScope() = default;
23+
24+
} // namespace gin_helper

‎shell/common/gin_helper/microtasks_scope.h

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
// Copyright (c) 2020 GitHub, Inc.
2+
// Use of this source code is governed by the MIT license that can be
3+
// found in the LICENSE file.
4+
5+
#ifndef SHELL_COMMON_GIN_HELPER_MICROTASKS_SCOPE_H_
6+
#define SHELL_COMMON_GIN_HELPER_MICROTASKS_SCOPE_H_
7+
8+
#include <memory>
9+
10+
#include "base/macros.h"
11+
#include "v8/include/v8.h"
12+
13+
namespace gin_helper {
14+
15+
// In the browser process runs v8::MicrotasksScope::PerformCheckpoint
16+
// In the render process creates a v8::MicrotasksScope.
17+
class MicrotasksScope {
18+
public:
19+
explicit MicrotasksScope(v8::Isolate* isolate,
20+
bool ignore_browser_checkpoint = false);
21+
~MicrotasksScope();
22+
23+
private:
24+
std::unique_ptr<v8::MicrotasksScope> v8_microtasks_scope_;
25+
26+
DISALLOW_COPY_AND_ASSIGN(MicrotasksScope);
27+
};
28+
29+
} // namespace gin_helper
30+
31+
#endif // SHELL_COMMON_GIN_HELPER_MICROTASKS_SCOPE_H_

‎shell/common/gin_helper/promise.cc

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,7 @@ PromiseBase& PromiseBase::operator=(PromiseBase&&) = default;
2626
v8::Maybe<bool> PromiseBase::Reject() {
2727
gin_helper::Locker locker(isolate());
2828
v8::HandleScope handle_scope(isolate());
29-
v8::MicrotasksScope script_scope(isolate(),
30-
v8::MicrotasksScope::kRunMicrotasks);
29+
gin_helper::MicrotasksScope microtasks_scope(isolate());
3130
v8::Context::Scope context_scope(GetContext());
3231

3332
return GetInner()->Reject(GetContext(), v8::Undefined(isolate()));
@@ -36,8 +35,7 @@ v8::Maybe<bool> PromiseBase::Reject() {
3635
v8::Maybe<bool> PromiseBase::Reject(v8::Local<v8::Value> except) {
3736
gin_helper::Locker locker(isolate());
3837
v8::HandleScope handle_scope(isolate());
39-
v8::MicrotasksScope script_scope(isolate(),
40-
v8::MicrotasksScope::kRunMicrotasks);
38+
gin_helper::MicrotasksScope microtasks_scope(isolate());
4139
v8::Context::Scope context_scope(GetContext());
4240

4341
return GetInner()->Reject(GetContext(), except);
@@ -46,8 +44,7 @@ v8::Maybe<bool> PromiseBase::Reject(v8::Local<v8::Value> except) {
4644
v8::Maybe<bool> PromiseBase::RejectWithErrorMessage(base::StringPiece message) {
4745
gin_helper::Locker locker(isolate());
4846
v8::HandleScope handle_scope(isolate());
49-
v8::MicrotasksScope script_scope(isolate(),
50-
v8::MicrotasksScope::kRunMicrotasks);
47+
gin_helper::MicrotasksScope microtasks_scope(isolate());
5148
v8::Context::Scope context_scope(GetContext());
5249

5350
v8::Local<v8::Value> error =
@@ -90,8 +87,7 @@ v8::Local<v8::Promise> Promise<void>::ResolvedPromise(v8::Isolate* isolate) {
9087
v8::Maybe<bool> Promise<void>::Resolve() {
9188
gin_helper::Locker locker(isolate());
9289
v8::HandleScope handle_scope(isolate());
93-
v8::MicrotasksScope script_scope(isolate(),
94-
v8::MicrotasksScope::kRunMicrotasks);
90+
gin_helper::MicrotasksScope microtasks_scope(isolate());
9591
v8::Context::Scope context_scope(GetContext());
9692

9793
return GetInner()->Resolve(GetContext(), v8::Undefined(isolate()));

‎shell/common/gin_helper/promise.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
#include "content/public/browser/browser_thread.h"
1717
#include "shell/common/gin_converters/std_converter.h"
1818
#include "shell/common/gin_helper/locker.h"
19+
#include "shell/common/gin_helper/microtasks_scope.h"
1920

2021
namespace gin_helper {
2122

@@ -110,8 +111,7 @@ class Promise : public PromiseBase {
110111
v8::Maybe<bool> Resolve(const RT& value) {
111112
gin_helper::Locker locker(isolate());
112113
v8::HandleScope handle_scope(isolate());
113-
v8::MicrotasksScope script_scope(isolate(),
114-
v8::MicrotasksScope::kRunMicrotasks);
114+
gin_helper::MicrotasksScope microtasks_scope(isolate());
115115
v8::Context::Scope context_scope(GetContext());
116116

117117
return GetInner()->Resolve(GetContext(),

‎shell/common/node_bindings.cc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
#include "shell/common/gin_helper/dictionary.h"
3030
#include "shell/common/gin_helper/event_emitter_caller.h"
3131
#include "shell/common/gin_helper/locker.h"
32+
#include "shell/common/gin_helper/microtasks_scope.h"
3233
#include "shell/common/mac/main_application_bundle.h"
3334
#include "shell/common/node_includes.h"
3435

@@ -475,8 +476,7 @@ void NodeBindings::UvRunOnce() {
475476
v8::Context::Scope context_scope(env->context());
476477

477478
// Perform microtask checkpoint after running JavaScript.
478-
v8::MicrotasksScope script_scope(env->isolate(),
479-
v8::MicrotasksScope::kRunMicrotasks);
479+
gin_helper::MicrotasksScope microtasks_scope(env->isolate());
480480

481481
if (browser_env_ != BrowserEnvironment::BROWSER)
482482
TRACE_EVENT_BEGIN0("devtools.timeline", "FunctionCall");

‎spec-main/api-session-spec.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -288,6 +288,8 @@ describe('session module', () => {
288288
const { item, itemUrl, itemFilename } = await downloadPrevented;
289289
expect(itemUrl).to.equal(url);
290290
expect(itemFilename).to.equal('mockFile.txt');
291+
// Delay till the next tick.
292+
await new Promise(resolve => setImmediate(() => resolve()));
291293
expect(() => item.getURL()).to.throw('DownloadItem used after being destroyed');
292294
});
293295
});

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

Lines changed: 29 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -597,28 +597,50 @@ describe('webContents module', () => {
597597
// On Mac, zooming isn't done with the mouse wheel.
598598
ifdescribe(process.platform !== 'darwin')('zoom-changed', () => {
599599
afterEach(closeAllWindows);
600-
it('is emitted with the correct zooming info', async () => {
600+
it('is emitted with the correct zoom-in info', async () => {
601601
const w = new BrowserWindow({ show: false });
602602
await w.loadFile(path.join(fixturesPath, 'pages', 'base-page.html'));
603603

604-
const testZoomChanged = async ({ zoomingIn }: { zoomingIn: boolean }) => {
604+
const testZoomChanged = async () => {
605605
w.webContents.sendInputEvent({
606606
type: 'mouseWheel',
607607
x: 300,
608608
y: 300,
609609
deltaX: 0,
610-
deltaY: zoomingIn ? 1 : -1,
610+
deltaY: 1,
611611
wheelTicksX: 0,
612-
wheelTicksY: zoomingIn ? 1 : -1,
612+
wheelTicksY: 1,
613613
modifiers: ['control', 'meta']
614614
});
615615

616616
const [, zoomDirection] = await emittedOnce(w.webContents, 'zoom-changed');
617-
expect(zoomDirection).to.equal(zoomingIn ? 'in' : 'out');
617+
expect(zoomDirection).to.equal('in');
618618
};
619619

620-
await testZoomChanged({ zoomingIn: true });
621-
await testZoomChanged({ zoomingIn: false });
620+
await testZoomChanged();
621+
});
622+
623+
it('is emitted with the correct zoom-out info', async () => {
624+
const w = new BrowserWindow({ show: false });
625+
await w.loadFile(path.join(fixturesPath, 'pages', 'base-page.html'));
626+
627+
const testZoomChanged = async () => {
628+
w.webContents.sendInputEvent({
629+
type: 'mouseWheel',
630+
x: 300,
631+
y: 300,
632+
deltaX: 0,
633+
deltaY: -1,
634+
wheelTicksX: 0,
635+
wheelTicksY: -1,
636+
modifiers: ['control', 'meta']
637+
});
638+
639+
const [, zoomDirection] = await emittedOnce(w.webContents, 'zoom-changed');
640+
expect(zoomDirection).to.equal('out');
641+
};
642+
643+
await testZoomChanged();
622644
});
623645
});
624646

‎spec-main/node-spec.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -320,4 +320,18 @@ describe('node feature', () => {
320320
done();
321321
});
322322
});
323+
324+
it('performs microtask checkpoint correctly', (done) => {
325+
const f3 = async () => {
326+
return new Promise((resolve, reject) => {
327+
reject(new Error('oops'));
328+
});
329+
};
330+
331+
process.once('unhandledRejection', () => done('catch block is delayed to next tick'));
332+
333+
setTimeout(() => {
334+
f3().catch(() => done());
335+
});
336+
});
323337
});

0 commit comments

Comments
 (0)