From 0c711f690e6a50840e7866af87ed1cc1949bac37 Mon Sep 17 00:00:00 2001 From: Nitish Sakhawalkar Date: Mon, 15 Oct 2018 08:26:47 -0700 Subject: [PATCH] fix: add microtask runner and fix promise test (#15071) * fix: Promise resolution and unit test * Update to use microtask runner * Address review --- atom/browser/api/atom_api_app.cc | 2 +- atom/browser/api/gpuinfo_manager.cc | 11 ++++---- atom/browser/api/gpuinfo_manager.h | 2 +- atom/browser/atom_browser_main_parts.cc | 1 + atom/browser/javascript_environment.cc | 9 +++++++ atom/browser/javascript_environment.h | 6 +++++ atom/browser/microtasks_runner.cc | 20 ++++++++++++++ atom/browser/microtasks_runner.h | 36 +++++++++++++++++++++++++ atom/common/promise_util.cc | 3 +-- atom/common/promise_util.h | 2 ++ filenames.gni | 2 ++ package.json | 2 +- spec/api-app-spec.js | 12 +++------ 13 files changed, 90 insertions(+), 18 deletions(-) create mode 100644 atom/browser/microtasks_runner.cc create mode 100644 atom/browser/microtasks_runner.h diff --git a/atom/browser/api/atom_api_app.cc b/atom/browser/api/atom_api_app.cc index 5d655cf7004e..dafde725c47d 100644 --- a/atom/browser/api/atom_api_app.cc +++ b/atom/browser/api/atom_api_app.cc @@ -1178,7 +1178,7 @@ v8::Local App::GetGPUInfo(v8::Isolate* isolate, auto* const info_mgr = GPUInfoManager::GetInstance(); if (info_type == "complete") { -#if defined(OS_WIN) +#if defined(OS_WIN) || defined(OS_MACOSX) info_mgr->FetchCompleteInfo(promise); #else info_mgr->FetchBasicInfo(promise); diff --git a/atom/browser/api/gpuinfo_manager.cc b/atom/browser/api/gpuinfo_manager.cc index 92002ab42db1..4dda3eb5e128 100644 --- a/atom/browser/api/gpuinfo_manager.cc +++ b/atom/browser/api/gpuinfo_manager.cc @@ -26,11 +26,12 @@ GPUInfoManager::~GPUInfoManager() { // Based on // https://chromium.googlesource.com/chromium/src.git/+/69.0.3497.106/content/browser/gpu/gpu_data_manager_impl_private.cc#838 -bool GPUInfoManager::NeedsCompleteGpuInfoCollection() { -#if defined(OS_WIN) - const auto& gpu_info = gpu_data_manager_->GetGPUInfo(); - return (gpu_info.dx_diagnostics.values.empty() && - gpu_info.dx_diagnostics.children.empty()); +bool GPUInfoManager::NeedsCompleteGpuInfoCollection() const { +#if defined(OS_MACOSX) + return gpu_data_manager_->GetGPUInfo().gl_vendor.empty(); +#elif defined(OS_WIN) + return (gpu_data_manager_->GetGPUInfo().dx_diagnostics.values.empty() && + gpu_data_manager_->GetGPUInfo().dx_diagnostics.children.empty()); #else return false; #endif diff --git a/atom/browser/api/gpuinfo_manager.h b/atom/browser/api/gpuinfo_manager.h index 9911b5786c2c..46955192203c 100644 --- a/atom/browser/api/gpuinfo_manager.h +++ b/atom/browser/api/gpuinfo_manager.h @@ -24,7 +24,7 @@ class GPUInfoManager : public content::GpuDataManagerObserver { GPUInfoManager(); ~GPUInfoManager() override; - bool NeedsCompleteGpuInfoCollection(); + bool NeedsCompleteGpuInfoCollection() const; void FetchCompleteInfo(scoped_refptr promise); void FetchBasicInfo(scoped_refptr promise); void OnGpuInfoUpdate() override; diff --git a/atom/browser/atom_browser_main_parts.cc b/atom/browser/atom_browser_main_parts.cc index 0603c406ddad..0cc9f7f2cdd9 100644 --- a/atom/browser/atom_browser_main_parts.cc +++ b/atom/browser/atom_browser_main_parts.cc @@ -253,6 +253,7 @@ void AtomBrowserMainParts::PreMainMessageLoopRun() { } bool AtomBrowserMainParts::MainMessageLoopRun(int* result_code) { + js_env_->OnMessageLoopCreated(); exit_code_ = result_code; return brightray::BrowserMainParts::MainMessageLoopRun(result_code); } diff --git a/atom/browser/javascript_environment.cc b/atom/browser/javascript_environment.cc index b27ceb27fe8f..3ed8eb1e9fa5 100644 --- a/atom/browser/javascript_environment.cc +++ b/atom/browser/javascript_environment.cc @@ -6,6 +6,7 @@ #include +#include "atom/browser/microtasks_runner.h" #include "base/command_line.h" #include "base/message_loop/message_loop.h" #include "base/task_scheduler/initialization_util.h" @@ -62,7 +63,15 @@ v8::Isolate* JavascriptEnvironment::Initialize(uv_loop_t* event_loop) { return isolate; } +void JavascriptEnvironment::OnMessageLoopCreated() { + DCHECK(!microtasks_runner_); + microtasks_runner_.reset(new MicrotasksRunner(isolate())); + base::MessageLoopCurrent::Get()->AddTaskObserver(microtasks_runner_.get()); +} + void JavascriptEnvironment::OnMessageLoopDestroying() { + DCHECK(microtasks_runner_); + base::MessageLoopCurrent::Get()->RemoveTaskObserver(microtasks_runner_.get()); platform_->UnregisterIsolate(isolate_); } diff --git a/atom/browser/javascript_environment.h b/atom/browser/javascript_environment.h index 2d5bc4dc17e5..e1a3d921d360 100644 --- a/atom/browser/javascript_environment.h +++ b/atom/browser/javascript_environment.h @@ -5,6 +5,8 @@ #ifndef ATOM_BROWSER_JAVASCRIPT_ENVIRONMENT_H_ #define ATOM_BROWSER_JAVASCRIPT_ENVIRONMENT_H_ +#include + #include "base/macros.h" #include "gin/public/isolate_holder.h" #include "uv.h" // NOLINT(build/include) @@ -16,12 +18,14 @@ class MultiIsolatePlatform; namespace atom { +class MicrotasksRunner; // Manage the V8 isolate and context automatically. class JavascriptEnvironment { public: explicit JavascriptEnvironment(uv_loop_t* event_loop); ~JavascriptEnvironment(); + void OnMessageLoopCreated(); void OnMessageLoopDestroying(); node::MultiIsolatePlatform* platform() const { return platform_; } @@ -43,6 +47,8 @@ class JavascriptEnvironment { v8::Global context_; v8::Context::Scope context_scope_; + std::unique_ptr microtasks_runner_; + DISALLOW_COPY_AND_ASSIGN(JavascriptEnvironment); }; diff --git a/atom/browser/microtasks_runner.cc b/atom/browser/microtasks_runner.cc new file mode 100644 index 000000000000..451eb8ff06b1 --- /dev/null +++ b/atom/browser/microtasks_runner.cc @@ -0,0 +1,20 @@ + +// Copyright (c) 2018 GitHub, Inc. +// Use of this source code is governed by the MIT license that can be +// found in the LICENSE file. + +#include "atom/browser/microtasks_runner.h" +#include "v8/include/v8.h" + +namespace atom { + +MicrotasksRunner::MicrotasksRunner(v8::Isolate* isolate) : isolate_(isolate) {} + +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_); +} + +} // namespace atom diff --git a/atom/browser/microtasks_runner.h b/atom/browser/microtasks_runner.h new file mode 100644 index 000000000000..c4c8e6d80052 --- /dev/null +++ b/atom/browser/microtasks_runner.h @@ -0,0 +1,36 @@ +// Copyright (c) 2018 GitHub, Inc. +// Use of this source code is governed by the MIT license that can be +// found in the LICENSE file. + +#ifndef ATOM_BROWSER_MICROTASKS_RUNNER_H_ +#define ATOM_BROWSER_MICROTASKS_RUNNER_H_ + +#include "base/message_loop/message_loop.h" + +namespace v8 { +class Isolate; +} + +namespace atom { + +// Microtasks like promise resolution, are run at the end of the current +// task. This class implements a task observer that runs tells v8 to run them. +// Microtasks runner implementation is based on the EndOfTaskRunner in blink. +// Node follows the kExplicit MicrotasksPolicy, and we do the same in browser +// process. Hence, we need to have this task observer to flush the queued +// microtasks. +class MicrotasksRunner : public base::MessageLoop::TaskObserver { + public: + explicit MicrotasksRunner(v8::Isolate* isolate); + + // base::MessageLoop::TaskObserver + void WillProcessTask(const base::PendingTask& pending_task) override; + void DidProcessTask(const base::PendingTask& pending_task) override; + + private: + v8::Isolate* isolate_; +}; + +} // namespace atom + +#endif // ATOM_BROWSER_MICROTASKS_RUNNER_H_ diff --git a/atom/common/promise_util.cc b/atom/common/promise_util.cc index 270ae3fed356..a841be9fe8b5 100644 --- a/atom/common/promise_util.cc +++ b/atom/common/promise_util.cc @@ -21,8 +21,7 @@ v8::Maybe Promise::RejectWithErrorMessage(const std::string& string) { v8::Local error_message = v8::String::NewFromUtf8(isolate(), string.c_str()); v8::Local error = v8::Exception::Error(error_message); - return GetInner()->Reject(isolate()->GetCurrentContext(), - mate::ConvertToV8(isolate(), error)); + return Reject(error); } v8::Local Promise::GetHandle() const { diff --git a/atom/common/promise_util.h b/atom/common/promise_util.h index 3cae133b41ba..2fcf2a59d827 100644 --- a/atom/common/promise_util.h +++ b/atom/common/promise_util.h @@ -32,6 +32,8 @@ class Promise : public base::RefCounted { v8::Undefined(isolate())); } + // Promise resolution is a microtask + // We use the MicrotasksRunner to trigger the running of pending microtasks template v8::Maybe Resolve(const T& value) { return GetInner()->Resolve(isolate()->GetCurrentContext(), diff --git a/filenames.gni b/filenames.gni index 49cc9c3c3deb..28e700afaaa9 100644 --- a/filenames.gni +++ b/filenames.gni @@ -271,6 +271,8 @@ filenames = { "atom/browser/mac/in_app_purchase_observer.mm", "atom/browser/mac/in_app_purchase_product.h", "atom/browser/mac/in_app_purchase_product.mm", + "atom/browser/microtasks_runner.cc", + "atom/browser/microtasks_runner.h", "atom/browser/native_browser_view.cc", "atom/browser/native_browser_view.h", "atom/browser/native_browser_view_mac.h", diff --git a/package.json b/package.json index a4c50c2e0cff..65b0768bda7f 100644 --- a/package.json +++ b/package.json @@ -45,7 +45,7 @@ "check-tls": "python ./script/tls.py", "clang-format": "find atom/ brightray/ chromium_src/ -iname *.h -o -iname *.cc -o -iname *.mm | xargs clang-format -i", "lint": "node ./script/lint.js && npm run lint:clang-format && npm run lint:docs", - "lint:js": ".node /script/lint.js --js", + "lint:js": "node ./script/lint.js --js", "lint:clang-format": "python script/run-clang-format.py -r -c atom/ chromium_src/ brightray/ || (echo \"\\nCode not formatted correctly.\" && exit 1)", "lint:cpp": "node ./script/lint.js --cc", "lint:py": "node ./script/lint.js --py", diff --git a/spec/api-app-spec.js b/spec/api-app-spec.js index ae1d0c0eb982..9f2e868b6fd3 100644 --- a/spec/api-app-spec.js +++ b/spec/api-app-spec.js @@ -852,10 +852,9 @@ describe('app module', () => { await verifyBasicGPUInfo(gpuInfo) }) - // FIXME: this broke with the M69 upgrade. - xit('succeeds with complete GPUInfo', async () => { + it('succeeds with complete GPUInfo', async () => { const completeInfo = await getGPUInfo('complete') - if (process.platform === 'linux' || process.platform === 'darwin') { + if (process.platform === 'linux') { // For linux and macOS complete info is same as basic info await verifyBasicGPUInfo(completeInfo) const basicInfo = await getGPUInfo('basic') @@ -872,11 +871,8 @@ describe('app module', () => { it('fails for invalid info_type', () => { const invalidType = 'invalid' - const errorMessage = - `app.getGPUInfo() didn't fail for the "${invalidType}" info type` - return app.getGPUInfo(invalidType).then( - () => Promise.reject(new Error(errorMessage)), - () => Promise.resolve()) + const expectedErrorMessage = "Invalid info type. Use 'basic' or 'complete'" + return expect(app.getGPUInfo(invalidType)).to.eventually.be.rejectedWith(expectedErrorMessage) }) })