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

@@ -1178,7 +1178,7 @@ v8::Local<v8::Promise> 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);

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

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<util::Promise> promise);
   void FetchBasicInfo(scoped_refptr<util::Promise> promise);
   void OnGpuInfoUpdate() override;

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);
 }

atom/browser/javascript_environment.cc

@@ -6,6 +6,7 @@
 
 #include <string>
 
+#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_);
 }
 

atom/browser/javascript_environment.h

@@ -5,6 +5,8 @@
 #ifndef ATOM_BROWSER_JAVASCRIPT_ENVIRONMENT_H_
 #define ATOM_BROWSER_JAVASCRIPT_ENVIRONMENT_H_
 
+#include <memory>
+
 #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<v8::Context> context_;
   v8::Context::Scope context_scope_;
 
+  std::unique_ptr<MicrotasksRunner> microtasks_runner_;
+
   DISALLOW_COPY_AND_ASSIGN(JavascriptEnvironment);
 };
 

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

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_

atom/common/promise_util.cc

@@ -21,8 +21,7 @@ v8::Maybe<bool> Promise::RejectWithErrorMessage(const std::string& string) {
   v8::Local<v8::String> error_message =
       v8::String::NewFromUtf8(isolate(), string.c_str());
   v8::Local<v8::Value> error = v8::Exception::Error(error_message);
-  return GetInner()->Reject(isolate()->GetCurrentContext(),
-                            mate::ConvertToV8(isolate(), error));
+  return Reject(error);
 }
 
 v8::Local<v8::Promise> Promise::GetHandle() const {

atom/common/promise_util.h

@@ -32,6 +32,8 @@ class Promise : public base::RefCounted<Promise> {
                               v8::Undefined(isolate()));
   }
 
+  // Promise resolution is a microtask
+  // We use the MicrotasksRunner to trigger the running of pending microtasks
   template <typename T>
   v8::Maybe<bool> Resolve(const T& value) {
     return GetInner()->Resolve(isolate()->GetCurrentContext(),

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",

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",

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)
     })
   })