From 8e2ab8b20b3729dc93b6d5324bfb0d7495c6cc9c Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Tue, 15 Jan 2019 09:54:59 -0800 Subject: [PATCH] refactor: promise_util promise creation (#16401) * refactor: promise_util creation * enter correct contexts on resolve/reject * return Local in helper * set context correctly * forgot one --- atom/browser/api/atom_api_app.cc | 11 ++------- atom/common/api/atom_api_shell.cc | 18 +++----------- atom/common/promise_util.cc | 13 +++++++++-- atom/common/promise_util.h | 39 ++++++++++++++++++++++++++----- 4 files changed, 49 insertions(+), 32 deletions(-) diff --git a/atom/browser/api/atom_api_app.cc b/atom/browser/api/atom_api_app.cc index 69c45d00bb682..2bcdd281cacd8 100644 --- a/atom/browser/api/atom_api_app.cc +++ b/atom/browser/api/atom_api_app.cc @@ -533,12 +533,8 @@ int ImportIntoCertStore(CertificateManagerModel* model, } #endif -void OnIconDataAvailable(v8::Isolate* isolate, - scoped_refptr promise, +void OnIconDataAvailable(scoped_refptr promise, gfx::Image* icon) { - v8::Locker locker(isolate); - v8::HandleScope handle_scope(isolate); - if (icon && !icon->IsEmpty()) { promise->Resolve(*icon); } else { @@ -1130,9 +1126,6 @@ JumpListResult App::SetJumpList(v8::Local val, v8::Local App::GetFileIcon(const base::FilePath& path, mate::Arguments* args) { - v8::Locker locker(isolate()); - v8::HandleScope handle_scope(isolate()); - scoped_refptr promise = new util::Promise(isolate()); base::FilePath normalized_path = path.NormalizePathSeparators(); @@ -1153,7 +1146,7 @@ v8::Local App::GetFileIcon(const base::FilePath& path, promise->Resolve(*icon); } else { icon_manager->LoadIcon(normalized_path, icon_size, - base::Bind(&OnIconDataAvailable, isolate(), promise), + base::Bind(&OnIconDataAvailable, promise), &cancelable_task_tracker_); } return promise->GetHandle(); diff --git a/atom/common/api/atom_api_shell.cc b/atom/common/api/atom_api_shell.cc index b49ca97c1c29f..a52dd74b8907b 100644 --- a/atom/common/api/atom_api_shell.cc +++ b/atom/common/api/atom_api_shell.cc @@ -44,17 +44,8 @@ struct Converter { namespace { -void OnOpenExternalFinished(const v8::Global& context, - scoped_refptr promise, +void OnOpenExternalFinished(scoped_refptr promise, const std::string& error) { - 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::New(isolate, context)); - if (error.empty()) promise->Resolve(); else @@ -99,11 +90,8 @@ v8::Local OpenExternal( } } - v8::Global context(args->isolate(), - args->isolate()->GetCurrentContext()); - platform_util::OpenExternal( - url, options, - base::Bind(&OnOpenExternalFinished, std::move(context), promise)); + platform_util::OpenExternal(url, options, + base::Bind(&OnOpenExternalFinished, promise)); return promise->GetHandle(); } diff --git a/atom/common/promise_util.cc b/atom/common/promise_util.cc index fd5236e01d334..27eec4fe4f394 100644 --- a/atom/common/promise_util.cc +++ b/atom/common/promise_util.cc @@ -2,10 +2,11 @@ // Use of this source code is governed by the MIT license that can be // found in the LICENSE file. -#include "atom/common/promise_util.h" - #include +#include "atom/common/api/locker.h" +#include "atom/common/promise_util.h" + namespace atom { namespace util { @@ -14,12 +15,20 @@ Promise::Promise(v8::Isolate* isolate) { auto context = isolate->GetCurrentContext(); auto resolver = v8::Promise::Resolver::New(context).ToLocalChecked(); isolate_ = isolate; + + context_.Reset(isolate, context); resolver_.Reset(isolate, resolver); } Promise::~Promise() = default; v8::Maybe Promise::RejectWithErrorMessage(const std::string& string) { + v8::HandleScope handle_scope(isolate()); + v8::MicrotasksScope script_scope(isolate(), + v8::MicrotasksScope::kRunMicrotasks); + v8::Context::Scope context_scope( + v8::Local::New(isolate(), GetContext())); + v8::Local error_message = v8::String::NewFromUtf8(isolate(), string.c_str()); v8::Local error = v8::Exception::Error(error_message); diff --git a/atom/common/promise_util.h b/atom/common/promise_util.h index 2fcf2a59d827e..c133423cfd434 100644 --- a/atom/common/promise_util.h +++ b/atom/common/promise_util.h @@ -7,6 +7,7 @@ #include +#include "atom/common/api/locker.h" #include "content/public/browser/browser_thread.h" #include "native_mate/converter.h" @@ -19,30 +20,55 @@ class Promise : public base::RefCounted { explicit Promise(v8::Isolate* isolate); v8::Isolate* isolate() const { return isolate_; } + v8::Local GetContext() { + return v8::Local::New(isolate_, context_); + } virtual v8::Local GetHandle() const; v8::Maybe Resolve() { - return GetInner()->Resolve(isolate()->GetCurrentContext(), - v8::Undefined(isolate())); + v8::HandleScope handle_scope(isolate()); + v8::MicrotasksScope script_scope(isolate(), + v8::MicrotasksScope::kRunMicrotasks); + v8::Context::Scope context_scope( + v8::Local::New(isolate(), GetContext())); + + return GetInner()->Resolve(GetContext(), v8::Undefined(isolate())); } v8::Maybe Reject() { - return GetInner()->Reject(isolate()->GetCurrentContext(), - v8::Undefined(isolate())); + v8::HandleScope handle_scope(isolate()); + v8::MicrotasksScope script_scope(isolate(), + v8::MicrotasksScope::kRunMicrotasks); + v8::Context::Scope context_scope( + v8::Local::New(isolate(), GetContext())); + + return GetInner()->Reject(GetContext(), 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(), + v8::HandleScope handle_scope(isolate()); + v8::MicrotasksScope script_scope(isolate(), + v8::MicrotasksScope::kRunMicrotasks); + v8::Context::Scope context_scope( + v8::Local::New(isolate(), GetContext())); + + return GetInner()->Resolve(GetContext(), mate::ConvertToV8(isolate(), value)); } template v8::Maybe Reject(const T& value) { - return GetInner()->Reject(isolate()->GetCurrentContext(), + v8::HandleScope handle_scope(isolate()); + v8::MicrotasksScope script_scope(isolate(), + v8::MicrotasksScope::kRunMicrotasks); + v8::Context::Scope context_scope( + v8::Local::New(isolate(), GetContext())); + + return GetInner()->Reject(GetContext(), mate::ConvertToV8(isolate(), value)); } @@ -52,6 +78,7 @@ class Promise : public base::RefCounted { virtual ~Promise(); friend class base::RefCounted; v8::Isolate* isolate_; + v8::Global context_; private: v8::Local GetInner() const {