From 6d9b186fa7d913dd4353ab92f37fa935140cfa89 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Wed, 13 Dec 2017 16:36:27 +0900 Subject: [PATCH] Fix race condition when calling JsAsker::BeforeStartInUI --- atom/browser/net/js_asker.cc | 7 +++++- atom/browser/net/js_asker.h | 42 +++++++++++++++++++++++++++++++++--- 2 files changed, 45 insertions(+), 4 deletions(-) diff --git a/atom/browser/net/js_asker.cc b/atom/browser/net/js_asker.cc index 0df6bb5c37d7..9588fb0f2cc9 100644 --- a/atom/browser/net/js_asker.cc +++ b/atom/browser/net/js_asker.cc @@ -17,11 +17,13 @@ namespace { // The callback which is passed to |handler|. void HandlerCallback(const BeforeStartCallback& before_start, + const base::Closure& before_post_callback, const ResponseCallback& callback, mate::Arguments* args) { // If there is no argument passed then we failed. v8::Local value; if (!args->GetNext(&value)) { + before_post_callback.Run(); content::BrowserThread::PostTask( content::BrowserThread::IO, FROM_HERE, base::Bind(callback, false, nullptr)); @@ -35,6 +37,7 @@ void HandlerCallback(const BeforeStartCallback& before_start, V8ValueConverter converter; v8::Local context = args->isolate()->GetCurrentContext(); std::unique_ptr options(converter.FromV8Value(value, context)); + before_post_callback.Run(); content::BrowserThread::PostTask( content::BrowserThread::IO, FROM_HERE, base::Bind(callback, true, base::Passed(&options))); @@ -46,6 +49,7 @@ void AskForOptions(v8::Isolate* isolate, const JavaScriptHandler& handler, std::unique_ptr request_details, const BeforeStartCallback& before_start, + const base::Closure& before_post_callback, const ResponseCallback& callback) { DCHECK_CURRENTLY_ON(content::BrowserThread::UI); v8::Locker locker(isolate); @@ -55,7 +59,8 @@ void AskForOptions(v8::Isolate* isolate, handler.Run( *(request_details.get()), mate::ConvertToV8(isolate, - base::Bind(&HandlerCallback, before_start, callback))); + base::Bind(&HandlerCallback, before_start, + before_post_callback, callback))); } bool IsErrorOptions(base::Value* value, int* error) { diff --git a/atom/browser/net/js_asker.h b/atom/browser/net/js_asker.h index 4753afb50b1e..fa5c293b819e 100644 --- a/atom/browser/net/js_asker.h +++ b/atom/browser/net/js_asker.h @@ -9,6 +9,7 @@ #include "base/callback.h" #include "base/memory/ref_counted.h" #include "base/memory/weak_ptr.h" +#include "base/synchronization/waitable_event.h" #include "base/values.h" #include "content/public/browser/browser_thread.h" #include "net/base/net_errors.h" @@ -35,6 +36,7 @@ void AskForOptions(v8::Isolate* isolate, const JavaScriptHandler& handler, std::unique_ptr request_details, const BeforeStartCallback& before_start, + const base::Closure& before_post_callback, const ResponseCallback& callback); // Test whether the |options| means an error. @@ -46,7 +48,21 @@ template class JsAsker : public RequestJob { public: JsAsker(net::URLRequest* request, net::NetworkDelegate* network_delegate) - : RequestJob(request, network_delegate), weak_factory_(this) {} + : RequestJob(request, network_delegate), + wait_ui_event_(base::WaitableEvent::ResetPolicy::MANUAL, + base::WaitableEvent::InitialState::SIGNALED), + weak_factory_(this) {} + + ~JsAsker() override { + // It is not safe to destroy JsAsker without calling Kill() while waiting + // for response from UI thread. Because by the time ~JsAsker() is called, + // the subclass has already been destructed, while we are still executing + // the subclass's BeforeStartInUI() method. + // But this corner case should never happen, since Kill() is usually called + // before destroying the URLRequestJob, so we just assert for that case. + CHECK(wait_ui_event_.IsSignaled()) << + "JsAsker is destroyed while waiting for response from UI thread."; + } // Called by |CustomProtocolHandler| to store handler related information. void SetHandlerInfo( @@ -66,25 +82,38 @@ class JsAsker : public RequestJob { return request_context_getter_; } - private: + protected: // RequestJob: void Start() override { std::unique_ptr request_details( new base::DictionaryValue); request_start_time_ = base::TimeTicks::Now(); FillRequestDetails(request_details.get(), RequestJob::request()); + // Do not kill the job while waiting for UI thread's response. + wait_ui_event_.Reset(); + // Ask for options from the UI thread. content::BrowserThread::PostTask( content::BrowserThread::UI, FROM_HERE, base::Bind(&internal::AskForOptions, isolate_, handler_, base::Passed(&request_details), + // The RequestJob is guaranteed to exist before OnResponse() + // is called. base::Bind(&JsAsker::BeforeStartInUI, - weak_factory_.GetWeakPtr()), + base::Unretained(this)), + base::Bind(&base::WaitableEvent::Signal, + base::Unretained(&wait_ui_event_)), base::Bind(&JsAsker::OnResponse, weak_factory_.GetWeakPtr()))); } + void Kill() override { + // Wait for UI thread before killing the job. + wait_ui_event_.Wait(); + RequestJob::Kill(); + } + int GetResponseCode() const override { return net::HTTP_OK; } // NOTE: We have to implement this method or risk a crash in blink for @@ -100,6 +129,7 @@ class JsAsker : public RequestJob { info->headers = new net::HttpResponseHeaders(""); } + private: // Called when the JS handler has sent the response, we need to decide whether // to start, or fail the job. void OnResponse(bool success, std::unique_ptr value) { @@ -113,6 +143,12 @@ class JsAsker : public RequestJob { } } + // Prevent this class from being destroyed when it is still accessed by UI + // thread. + // FIXME(zcbenz): Refactor JsAsker and its subclasses so this class is not + // used by both UI and IO threads. + base::WaitableEvent wait_ui_event_; + v8::Isolate* isolate_; net::URLRequestContextGetter* request_context_getter_; JavaScriptHandler handler_;