From 9372d4df32450f839c48e9280d9536dc1d891b47 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Wed, 15 Jun 2016 17:12:45 +0900 Subject: [PATCH 1/4] Make sure BrowserContext is deleted after Protocol --- atom/browser/api/atom_api_protocol.cc | 24 ++++++++++----------- atom/browser/api/atom_api_protocol.h | 30 +++++++++++++++++---------- 2 files changed, 31 insertions(+), 23 deletions(-) diff --git a/atom/browser/api/atom_api_protocol.cc b/atom/browser/api/atom_api_protocol.cc index db8facfe09..6ec7f017d8 100644 --- a/atom/browser/api/atom_api_protocol.cc +++ b/atom/browser/api/atom_api_protocol.cc @@ -5,7 +5,6 @@ #include "atom/browser/api/atom_api_protocol.h" #include "atom/browser/atom_browser_client.h" -#include "atom/browser/atom_browser_context.h" #include "atom/browser/atom_browser_main_parts.h" #include "atom/browser/net/url_request_async_asar_job.h" #include "atom/browser/net/url_request_buffer_job.h" @@ -28,9 +27,10 @@ namespace atom { namespace api { Protocol::Protocol(v8::Isolate* isolate, AtomBrowserContext* browser_context) - : request_context_getter_(browser_context->GetRequestContext()), - job_factory_(browser_context->job_factory()) { - CHECK(job_factory_); + : browser_context_(browser_context), + request_context_getter_(browser_context->GetRequestContext()), + weak_factory_(this) { + CHECK(job_factory()); Init(isolate); } @@ -48,19 +48,19 @@ void Protocol::UnregisterProtocol( base::Bind(&Protocol::UnregisterProtocolInIO, base::Unretained(this), scheme), base::Bind(&Protocol::OnIOCompleted, - base::Unretained(this), callback)); + GetWeakPtr(), callback)); } Protocol::ProtocolError Protocol::UnregisterProtocolInIO( const std::string& scheme) { - if (!job_factory_->HasProtocolHandler(scheme)) + if (!job_factory()->HasProtocolHandler(scheme)) return PROTOCOL_NOT_REGISTERED; - job_factory_->SetProtocolHandler(scheme, nullptr); + job_factory()->SetProtocolHandler(scheme, nullptr); return PROTOCOL_OK; } void Protocol::IsProtocolHandled(const std::string& scheme, - const BooleanCallback& callback) { + const BooleanCallback& callback) { content::BrowserThread::PostTaskAndReplyWithResult( content::BrowserThread::IO, FROM_HERE, base::Bind(&Protocol::IsProtocolHandledInIO, @@ -69,7 +69,7 @@ void Protocol::IsProtocolHandled(const std::string& scheme, } bool Protocol::IsProtocolHandledInIO(const std::string& scheme) { - return job_factory_->IsHandledProtocol(scheme); + return job_factory()->IsHandledProtocol(scheme); } void Protocol::UninterceptProtocol( @@ -81,15 +81,15 @@ void Protocol::UninterceptProtocol( base::Bind(&Protocol::UninterceptProtocolInIO, base::Unretained(this), scheme), base::Bind(&Protocol::OnIOCompleted, - base::Unretained(this), callback)); + GetWeakPtr(), callback)); } Protocol::ProtocolError Protocol::UninterceptProtocolInIO( const std::string& scheme) { if (!original_protocols_.contains(scheme)) return PROTOCOL_NOT_INTERCEPTED; - job_factory_->ReplaceProtocol(scheme, - original_protocols_.take_and_erase(scheme)); + job_factory()->ReplaceProtocol(scheme, + original_protocols_.take_and_erase(scheme)); return PROTOCOL_OK; } diff --git a/atom/browser/api/atom_api_protocol.h b/atom/browser/api/atom_api_protocol.h index 734d179dfa..bdcd4a9cfa 100644 --- a/atom/browser/api/atom_api_protocol.h +++ b/atom/browser/api/atom_api_protocol.h @@ -10,9 +10,11 @@ #include #include "atom/browser/api/trackable_object.h" +#include "atom/browser/atom_browser_context.h" #include "atom/browser/net/atom_url_request_job_factory.h" #include "base/callback.h" #include "base/containers/scoped_ptr_hash_map.h" +#include "base/memory/weak_ptr.h" #include "content/public/browser/browser_thread.h" #include "native_mate/arguments.h" #include "native_mate/dictionary.h" @@ -29,9 +31,6 @@ class URLRequestContextGetter; namespace atom { -class AtomBrowserContext; -class AtomURLRequestJobFactory; - namespace api { class Protocol : public mate::TrackableObject { @@ -50,6 +49,14 @@ class Protocol : public mate::TrackableObject { protected: Protocol(v8::Isolate* isolate, AtomBrowserContext* browser_context); + AtomURLRequestJobFactory* job_factory() const { + return browser_context_->job_factory(); + } + + base::WeakPtr GetWeakPtr() { + return weak_factory_.GetWeakPtr(); + } + private: // Possible errors. enum ProtocolError { @@ -107,17 +114,17 @@ class Protocol : public mate::TrackableObject { base::Bind(&Protocol::RegisterProtocolInIO, base::Unretained(this), scheme, handler), base::Bind(&Protocol::OnIOCompleted, - base::Unretained(this), callback)); + GetWeakPtr(), callback)); } template ProtocolError RegisterProtocolInIO(const std::string& scheme, const Handler& handler) { - if (job_factory_->IsHandledProtocol(scheme)) + if (job_factory()->IsHandledProtocol(scheme)) return PROTOCOL_REGISTERED; std::unique_ptr> protocol_handler( new CustomProtocolHandler( isolate(), request_context_getter_, handler)); - if (job_factory_->SetProtocolHandler(scheme, std::move(protocol_handler))) + if (job_factory()->SetProtocolHandler(scheme, std::move(protocol_handler))) return PROTOCOL_OK; else return PROTOCOL_FAIL; @@ -144,15 +151,15 @@ class Protocol : public mate::TrackableObject { base::Bind(&Protocol::InterceptProtocolInIO, base::Unretained(this), scheme, handler), base::Bind(&Protocol::OnIOCompleted, - base::Unretained(this), callback)); + GetWeakPtr(), callback)); } template ProtocolError InterceptProtocolInIO(const std::string& scheme, const Handler& handler) { - if (!job_factory_->IsHandledProtocol(scheme)) + if (!job_factory()->IsHandledProtocol(scheme)) return PROTOCOL_NOT_REGISTERED; // It is possible a protocol is handled but can not be intercepted. - if (!job_factory_->HasProtocolHandler(scheme)) + if (!job_factory()->HasProtocolHandler(scheme)) return PROTOCOL_FAIL; if (ContainsKey(original_protocols_, scheme)) return PROTOCOL_INTERCEPTED; @@ -161,7 +168,7 @@ class Protocol : public mate::TrackableObject { isolate(), request_context_getter_, handler)); original_protocols_.set( scheme, - job_factory_->ReplaceProtocol(scheme, std::move(protocol_handler))); + job_factory()->ReplaceProtocol(scheme, std::move(protocol_handler))); return PROTOCOL_OK; } @@ -175,6 +182,7 @@ class Protocol : public mate::TrackableObject { // Convert error code to string. std::string ErrorCodeToString(ProtocolError error); + scoped_refptr browser_context_; net::URLRequestContextGetter* request_context_getter_; // Map that stores the original protocols of schemes. @@ -183,7 +191,7 @@ class Protocol : public mate::TrackableObject { std::unique_ptr>; OriginalProtocolsMap original_protocols_; - AtomURLRequestJobFactory* job_factory_; // weak ref + base::WeakPtrFactory weak_factory_; DISALLOW_COPY_AND_ASSIGN(Protocol); }; From 659384383ecc150cd4e1b0aff462a4b317444192 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Wed, 15 Jun 2016 20:31:29 +0900 Subject: [PATCH 2/4] Avoid storing JobFactory in BrowserContext JobFactory should always be created and accessed in IO thread. --- atom/browser/api/atom_api_protocol.cc | 4 +--- atom/browser/api/atom_api_protocol.h | 18 ++++++++---------- atom/browser/atom_browser_context.cc | 27 +++++++++------------------ atom/browser/atom_browser_context.h | 8 +------- vendor/brightray | 2 +- 5 files changed, 20 insertions(+), 39 deletions(-) diff --git a/atom/browser/api/atom_api_protocol.cc b/atom/browser/api/atom_api_protocol.cc index 6ec7f017d8..b53fd6c2b9 100644 --- a/atom/browser/api/atom_api_protocol.cc +++ b/atom/browser/api/atom_api_protocol.cc @@ -27,10 +27,8 @@ namespace atom { namespace api { Protocol::Protocol(v8::Isolate* isolate, AtomBrowserContext* browser_context) - : browser_context_(browser_context), - request_context_getter_(browser_context->GetRequestContext()), + : request_context_getter_(browser_context->GetRequestContext()), weak_factory_(this) { - CHECK(job_factory()); Init(isolate); } diff --git a/atom/browser/api/atom_api_protocol.h b/atom/browser/api/atom_api_protocol.h index bdcd4a9cfa..3c262d205a 100644 --- a/atom/browser/api/atom_api_protocol.h +++ b/atom/browser/api/atom_api_protocol.h @@ -19,16 +19,12 @@ #include "native_mate/arguments.h" #include "native_mate/dictionary.h" #include "native_mate/handle.h" +#include "net/url_request/url_request_context.h" namespace base { class DictionaryValue; } -namespace net { -class URLRequest; -class URLRequestContextGetter; -} - namespace atom { namespace api { @@ -50,7 +46,10 @@ class Protocol : public mate::TrackableObject { Protocol(v8::Isolate* isolate, AtomBrowserContext* browser_context); AtomURLRequestJobFactory* job_factory() const { - return browser_context_->job_factory(); + request_context_getter_->GetURLRequestContext(); // Force init. + return static_cast( + static_cast( + request_context_getter_.get())->job_factory()); } base::WeakPtr GetWeakPtr() { @@ -123,7 +122,7 @@ class Protocol : public mate::TrackableObject { return PROTOCOL_REGISTERED; std::unique_ptr> protocol_handler( new CustomProtocolHandler( - isolate(), request_context_getter_, handler)); + isolate(), request_context_getter_.get(), handler)); if (job_factory()->SetProtocolHandler(scheme, std::move(protocol_handler))) return PROTOCOL_OK; else @@ -165,7 +164,7 @@ class Protocol : public mate::TrackableObject { return PROTOCOL_INTERCEPTED; std::unique_ptr> protocol_handler( new CustomProtocolHandler( - isolate(), request_context_getter_, handler)); + isolate(), request_context_getter_.get(), handler)); original_protocols_.set( scheme, job_factory()->ReplaceProtocol(scheme, std::move(protocol_handler))); @@ -182,8 +181,7 @@ class Protocol : public mate::TrackableObject { // Convert error code to string. std::string ErrorCodeToString(ProtocolError error); - scoped_refptr browser_context_; - net::URLRequestContextGetter* request_context_getter_; + scoped_refptr request_context_getter_; // Map that stores the original protocols of schemes. using OriginalProtocolsMap = base::ScopedPtrHashMap< diff --git a/atom/browser/atom_browser_context.cc b/atom/browser/atom_browser_context.cc index 6f28cf6df7..8268a77781 100644 --- a/atom/browser/atom_browser_context.cc +++ b/atom/browser/atom_browser_context.cc @@ -20,14 +20,15 @@ #include "atom/common/options_switches.h" #include "base/command_line.h" #include "base/files/file_path.h" +#include "base/memory/ptr_util.h" #include "base/path_service.h" -#include "components/prefs/pref_registry_simple.h" #include "base/strings/string_util.h" #include "base/strings/stringprintf.h" #include "base/threading/sequenced_worker_pool.h" #include "base/threading/worker_pool.h" #include "chrome/common/chrome_paths.h" #include "chrome/common/pref_names.h" +#include "components/prefs/pref_registry_simple.h" #include "content/public/browser/browser_thread.h" #include "content/public/common/url_constants.h" #include "content/public/common/user_agent.h" @@ -66,7 +67,6 @@ AtomBrowserContext::AtomBrowserContext(const std::string& partition, bool in_memory) : brightray::BrowserContext(partition, in_memory), cert_verifier_(new AtomCertVerifier), - job_factory_(new AtomURLRequestJobFactory), network_delegate_(new AtomNetworkDelegate) { } @@ -96,15 +96,15 @@ std::string AtomBrowserContext::GetUserAgent() { std::unique_ptr AtomBrowserContext::CreateURLRequestJobFactory( - content::ProtocolHandlerMap* handlers, - content::URLRequestInterceptorScopedVector* interceptors) { - std::unique_ptr job_factory(job_factory_); + content::ProtocolHandlerMap* protocol_handlers) { + std::unique_ptr job_factory( + new AtomURLRequestJobFactory); - for (auto& it : *handlers) { + for (auto& it : *protocol_handlers) { job_factory->SetProtocolHandler(it.first, - make_scoped_ptr(it.second.release())); + base::WrapUnique(it.second.release())); } - handlers->clear(); + protocol_handlers->clear(); job_factory->SetProtocolHandler( url::kDataScheme, make_scoped_ptr(new net::DataProtocolHandler)); @@ -132,16 +132,7 @@ AtomBrowserContext::CreateURLRequestJobFactory( make_scoped_ptr(new net::FtpProtocolHandler( new net::FtpNetworkLayer(host_resolver)))); - // Set up interceptors in the reverse order. - std::unique_ptr top_job_factory = - std::move(job_factory); - content::URLRequestInterceptorScopedVector::reverse_iterator it; - for (it = interceptors->rbegin(); it != interceptors->rend(); ++it) - top_job_factory.reset(new net::URLRequestInterceptingJobFactory( - std::move(top_job_factory), make_scoped_ptr(*it))); - interceptors->weak_clear(); - - return top_job_factory; + return std::move(job_factory); } net::HttpCache::BackendFactory* diff --git a/atom/browser/atom_browser_context.h b/atom/browser/atom_browser_context.h index 028f5d1e79..c9b1de947f 100644 --- a/atom/browser/atom_browser_context.h +++ b/atom/browser/atom_browser_context.h @@ -15,7 +15,6 @@ class AtomDownloadManagerDelegate; class AtomCertVerifier; class AtomNetworkDelegate; class AtomPermissionManager; -class AtomURLRequestJobFactory; class WebViewManager; class AtomBrowserContext : public brightray::BrowserContext { @@ -27,8 +26,7 @@ class AtomBrowserContext : public brightray::BrowserContext { net::NetworkDelegate* CreateNetworkDelegate() override; std::string GetUserAgent() override; std::unique_ptr CreateURLRequestJobFactory( - content::ProtocolHandlerMap* handlers, - content::URLRequestInterceptorScopedVector* interceptors) override; + content::ProtocolHandlerMap* protocol_handlers) override; net::HttpCache::BackendFactory* CreateHttpCacheBackendFactory( const base::FilePath& base_path) override; std::unique_ptr CreateCertVerifier() override; @@ -43,9 +41,6 @@ class AtomBrowserContext : public brightray::BrowserContext { void RegisterPrefs(PrefRegistrySimple* pref_registry) override; AtomCertVerifier* cert_verifier() const { return cert_verifier_; } - - AtomURLRequestJobFactory* job_factory() const { return job_factory_; } - AtomNetworkDelegate* network_delegate() const { return network_delegate_; } private: @@ -55,7 +50,6 @@ class AtomBrowserContext : public brightray::BrowserContext { // Managed by brightray::BrowserContext. AtomCertVerifier* cert_verifier_; - AtomURLRequestJobFactory* job_factory_; AtomNetworkDelegate* network_delegate_; DISALLOW_COPY_AND_ASSIGN(AtomBrowserContext); diff --git a/vendor/brightray b/vendor/brightray index 55423bdca5..717d92968d 160000 --- a/vendor/brightray +++ b/vendor/brightray @@ -1 +1 @@ -Subproject commit 55423bdca586a20fe052f6c0e19565710dcebd42 +Subproject commit 717d92968d7959814060b9409c4720ab647bd90e From fe0e17d1c3714f7f87afca5700c15c0664eee143 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Wed, 15 Jun 2016 21:11:42 +0900 Subject: [PATCH 3/4] Make api::Protocol thread safe --- atom/browser/api/atom_api_protocol.cc | 41 +++++++--- atom/browser/api/atom_api_protocol.h | 80 +++++++++---------- .../net/atom_url_request_job_factory.cc | 19 ++++- .../net/atom_url_request_job_factory.h | 16 ++-- 4 files changed, 94 insertions(+), 62 deletions(-) diff --git a/atom/browser/api/atom_api_protocol.cc b/atom/browser/api/atom_api_protocol.cc index b53fd6c2b9..c762559ab6 100644 --- a/atom/browser/api/atom_api_protocol.cc +++ b/atom/browser/api/atom_api_protocol.cc @@ -27,7 +27,8 @@ namespace atom { namespace api { Protocol::Protocol(v8::Isolate* isolate, AtomBrowserContext* browser_context) - : request_context_getter_(browser_context->GetRequestContext()), + : request_context_getter_(static_cast( + browser_context->GetRequestContext())), weak_factory_(this) { Init(isolate); } @@ -44,16 +45,20 @@ void Protocol::UnregisterProtocol( content::BrowserThread::PostTaskAndReplyWithResult( content::BrowserThread::IO, FROM_HERE, base::Bind(&Protocol::UnregisterProtocolInIO, - base::Unretained(this), scheme), + request_context_getter_, scheme), base::Bind(&Protocol::OnIOCompleted, GetWeakPtr(), callback)); } +// static Protocol::ProtocolError Protocol::UnregisterProtocolInIO( + scoped_refptr request_context_getter, const std::string& scheme) { - if (!job_factory()->HasProtocolHandler(scheme)) + auto job_factory = static_cast( + request_context_getter->job_factory()); + if (!job_factory->HasProtocolHandler(scheme)) return PROTOCOL_NOT_REGISTERED; - job_factory()->SetProtocolHandler(scheme, nullptr); + job_factory->SetProtocolHandler(scheme, nullptr); return PROTOCOL_OK; } @@ -62,12 +67,15 @@ void Protocol::IsProtocolHandled(const std::string& scheme, content::BrowserThread::PostTaskAndReplyWithResult( content::BrowserThread::IO, FROM_HERE, base::Bind(&Protocol::IsProtocolHandledInIO, - base::Unretained(this), scheme), + request_context_getter_, scheme), callback); } -bool Protocol::IsProtocolHandledInIO(const std::string& scheme) { - return job_factory()->IsHandledProtocol(scheme); +// static +bool Protocol::IsProtocolHandledInIO( + scoped_refptr request_context_getter, + const std::string& scheme) { + return request_context_getter->job_factory()->IsHandledProtocol(scheme); } void Protocol::UninterceptProtocol( @@ -77,18 +85,18 @@ void Protocol::UninterceptProtocol( content::BrowserThread::PostTaskAndReplyWithResult( content::BrowserThread::IO, FROM_HERE, base::Bind(&Protocol::UninterceptProtocolInIO, - base::Unretained(this), scheme), + request_context_getter_, scheme), base::Bind(&Protocol::OnIOCompleted, GetWeakPtr(), callback)); } +// static Protocol::ProtocolError Protocol::UninterceptProtocolInIO( + scoped_refptr request_context_getter, const std::string& scheme) { - if (!original_protocols_.contains(scheme)) - return PROTOCOL_NOT_INTERCEPTED; - job_factory()->ReplaceProtocol(scheme, - original_protocols_.take_and_erase(scheme)); - return PROTOCOL_OK; + return static_cast( + request_context_getter->job_factory())->UninterceptProtocol(scheme) ? + PROTOCOL_OK : PROTOCOL_NOT_INTERCEPTED; } void Protocol::OnIOCompleted( @@ -119,6 +127,13 @@ std::string Protocol::ErrorCodeToString(ProtocolError error) { } } +AtomURLRequestJobFactory* Protocol::GetJobFactoryInIO() const { + request_context_getter_->GetURLRequestContext(); // Force init. + return static_cast( + static_cast( + request_context_getter_.get())->job_factory()); +} + // static mate::Handle Protocol::Create( v8::Isolate* isolate, AtomBrowserContext* browser_context) { diff --git a/atom/browser/api/atom_api_protocol.h b/atom/browser/api/atom_api_protocol.h index 3c262d205a..379c43bfcd 100644 --- a/atom/browser/api/atom_api_protocol.h +++ b/atom/browser/api/atom_api_protocol.h @@ -13,7 +13,6 @@ #include "atom/browser/atom_browser_context.h" #include "atom/browser/net/atom_url_request_job_factory.h" #include "base/callback.h" -#include "base/containers/scoped_ptr_hash_map.h" #include "base/memory/weak_ptr.h" #include "content/public/browser/browser_thread.h" #include "native_mate/arguments.h" @@ -45,17 +44,6 @@ class Protocol : public mate::TrackableObject { protected: Protocol(v8::Isolate* isolate, AtomBrowserContext* browser_context); - AtomURLRequestJobFactory* job_factory() const { - request_context_getter_->GetURLRequestContext(); // Force init. - return static_cast( - static_cast( - request_context_getter_.get())->job_factory()); - } - - base::WeakPtr GetWeakPtr() { - return weak_factory_.GetWeakPtr(); - } - private: // Possible errors. enum ProtocolError { @@ -86,13 +74,13 @@ class Protocol : public mate::TrackableObject { net::URLRequest* request, net::NetworkDelegate* network_delegate) const override { RequestJob* request_job = new RequestJob(request, network_delegate); - request_job->SetHandlerInfo(isolate_, request_context_, handler_); + request_job->SetHandlerInfo(isolate_, request_context_.get(), handler_); return request_job; } private: v8::Isolate* isolate_; - net::URLRequestContextGetter* request_context_; + scoped_refptr request_context_; Protocol::Handler handler_; DISALLOW_COPY_AND_ASSIGN(CustomProtocolHandler); @@ -111,19 +99,24 @@ class Protocol : public mate::TrackableObject { content::BrowserThread::PostTaskAndReplyWithResult( content::BrowserThread::IO, FROM_HERE, base::Bind(&Protocol::RegisterProtocolInIO, - base::Unretained(this), scheme, handler), + request_context_getter_, isolate(), scheme, handler), base::Bind(&Protocol::OnIOCompleted, GetWeakPtr(), callback)); } template - ProtocolError RegisterProtocolInIO(const std::string& scheme, - const Handler& handler) { - if (job_factory()->IsHandledProtocol(scheme)) + static ProtocolError RegisterProtocolInIO( + scoped_refptr request_context_getter, + v8::Isolate* isolate, + const std::string& scheme, + const Handler& handler) { + auto job_factory = static_cast( + request_context_getter->job_factory()); + if (job_factory->IsHandledProtocol(scheme)) return PROTOCOL_REGISTERED; std::unique_ptr> protocol_handler( new CustomProtocolHandler( - isolate(), request_context_getter_.get(), handler)); - if (job_factory()->SetProtocolHandler(scheme, std::move(protocol_handler))) + isolate, request_context_getter.get(), handler)); + if (job_factory->SetProtocolHandler(scheme, std::move(protocol_handler))) return PROTOCOL_OK; else return PROTOCOL_FAIL; @@ -131,12 +124,16 @@ class Protocol : public mate::TrackableObject { // Unregister the protocol handler that handles |scheme|. void UnregisterProtocol(const std::string& scheme, mate::Arguments* args); - ProtocolError UnregisterProtocolInIO(const std::string& scheme); + static ProtocolError UnregisterProtocolInIO( + scoped_refptr request_context_getter, + const std::string& scheme); // Whether the protocol has handler registered. void IsProtocolHandled(const std::string& scheme, const BooleanCallback& callback); - bool IsProtocolHandledInIO(const std::string& scheme); + static bool IsProtocolHandledInIO( + scoped_refptr request_context_getter, + const std::string& scheme); // Replace the protocol handler with a new one. template @@ -148,32 +145,36 @@ class Protocol : public mate::TrackableObject { content::BrowserThread::PostTaskAndReplyWithResult( content::BrowserThread::IO, FROM_HERE, base::Bind(&Protocol::InterceptProtocolInIO, - base::Unretained(this), scheme, handler), + request_context_getter_, isolate(), scheme, handler), base::Bind(&Protocol::OnIOCompleted, GetWeakPtr(), callback)); } template - ProtocolError InterceptProtocolInIO(const std::string& scheme, - const Handler& handler) { - if (!job_factory()->IsHandledProtocol(scheme)) + static ProtocolError InterceptProtocolInIO( + scoped_refptr request_context_getter, + v8::Isolate* isolate, + const std::string& scheme, + const Handler& handler) { + auto job_factory = static_cast( + request_context_getter->job_factory()); + if (!job_factory->IsHandledProtocol(scheme)) return PROTOCOL_NOT_REGISTERED; // It is possible a protocol is handled but can not be intercepted. - if (!job_factory()->HasProtocolHandler(scheme)) + if (!job_factory->HasProtocolHandler(scheme)) return PROTOCOL_FAIL; - if (ContainsKey(original_protocols_, scheme)) - return PROTOCOL_INTERCEPTED; std::unique_ptr> protocol_handler( new CustomProtocolHandler( - isolate(), request_context_getter_.get(), handler)); - original_protocols_.set( - scheme, - job_factory()->ReplaceProtocol(scheme, std::move(protocol_handler))); + isolate, request_context_getter.get(), handler)); + if (!job_factory->InterceptProtocol(scheme, std::move(protocol_handler))) + return PROTOCOL_INTERCEPTED; return PROTOCOL_OK; } // Restore the |scheme| to its original protocol handler. void UninterceptProtocol(const std::string& scheme, mate::Arguments* args); - ProtocolError UninterceptProtocolInIO(const std::string& scheme); + static ProtocolError UninterceptProtocolInIO( + scoped_refptr request_context_getter, + const std::string& scheme); // Convert error code to JS exception and call the callback. void OnIOCompleted(const CompletionCallback& callback, ProtocolError error); @@ -181,14 +182,13 @@ class Protocol : public mate::TrackableObject { // Convert error code to string. std::string ErrorCodeToString(ProtocolError error); - scoped_refptr request_context_getter_; + AtomURLRequestJobFactory* GetJobFactoryInIO() const; - // Map that stores the original protocols of schemes. - using OriginalProtocolsMap = base::ScopedPtrHashMap< - std::string, - std::unique_ptr>; - OriginalProtocolsMap original_protocols_; + base::WeakPtr GetWeakPtr() { + return weak_factory_.GetWeakPtr(); + } + scoped_refptr request_context_getter_; base::WeakPtrFactory weak_factory_; DISALLOW_COPY_AND_ASSIGN(Protocol); diff --git a/atom/browser/net/atom_url_request_job_factory.cc b/atom/browser/net/atom_url_request_job_factory.cc index aff2565814..d78b7026e5 100644 --- a/atom/browser/net/atom_url_request_job_factory.cc +++ b/atom/browser/net/atom_url_request_job_factory.cc @@ -5,6 +5,7 @@ #include "atom/browser/net/atom_url_request_job_factory.h" +#include "base/memory/ptr_util.h" #include "base/stl_util.h" #include "content/public/browser/browser_thread.h" #include "net/base/load_flags.h" @@ -41,14 +42,24 @@ bool AtomURLRequestJobFactory::SetProtocolHandler( return true; } -std::unique_ptr AtomURLRequestJobFactory::ReplaceProtocol( +bool AtomURLRequestJobFactory::InterceptProtocol( const std::string& scheme, std::unique_ptr protocol_handler) { - if (!ContainsKey(protocol_handler_map_, scheme)) - return nullptr; + if (!ContainsKey(protocol_handler_map_, scheme) || + ContainsKey(original_protocols_, scheme)) + return false; ProtocolHandler* original_protocol_handler = protocol_handler_map_[scheme]; protocol_handler_map_[scheme] = protocol_handler.release(); - return make_scoped_ptr(original_protocol_handler); + original_protocols_.set(scheme, base::WrapUnique(original_protocol_handler)); + return true; +} + +bool AtomURLRequestJobFactory::UninterceptProtocol(const std::string& scheme) { + if (!original_protocols_.contains(scheme)) + return false; + protocol_handler_map_[scheme] = + original_protocols_.take_and_erase(scheme).release(); + return true; } ProtocolHandler* AtomURLRequestJobFactory::GetProtocolHandler( diff --git a/atom/browser/net/atom_url_request_job_factory.h b/atom/browser/net/atom_url_request_job_factory.h index e3dbd77542..ea2710bb4c 100644 --- a/atom/browser/net/atom_url_request_job_factory.h +++ b/atom/browser/net/atom_url_request_job_factory.h @@ -7,11 +7,11 @@ #define ATOM_BROWSER_NET_ATOM_URL_REQUEST_JOB_FACTORY_H_ #include +#include #include #include -#include "base/memory/scoped_ptr.h" -#include "base/synchronization/lock.h" +#include "base/containers/scoped_ptr_hash_map.h" #include "net/url_request/url_request_job_factory.h" namespace atom { @@ -27,11 +27,11 @@ class AtomURLRequestJobFactory : public net::URLRequestJobFactory { bool SetProtocolHandler(const std::string& scheme, std::unique_ptr protocol_handler); - // Intercepts the ProtocolHandler for a scheme. Returns the original protocol - // handler on success, otherwise returns NULL. - std::unique_ptr ReplaceProtocol( + // Intercepts the ProtocolHandler for a scheme. + bool InterceptProtocol( const std::string& scheme, std::unique_ptr protocol_handler); + bool UninterceptProtocol(const std::string& scheme); // Returns the protocol handler registered with scheme. ProtocolHandler* GetProtocolHandler(const std::string& scheme) const; @@ -60,6 +60,12 @@ class AtomURLRequestJobFactory : public net::URLRequestJobFactory { ProtocolHandlerMap protocol_handler_map_; + // Map that stores the original protocols of schemes. + using OriginalProtocolsMap = base::ScopedPtrHashMap< + std::string, std::unique_ptr>; + // Can only be accessed in IO thread. + OriginalProtocolsMap original_protocols_; + DISALLOW_COPY_AND_ASSIGN(AtomURLRequestJobFactory); }; From 335cd8779c2f55e3581f34e6b1a8773cbc6eab2e Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Wed, 15 Jun 2016 21:12:06 +0900 Subject: [PATCH 4/4] spec: Correctly cleanup the protocol test --- spec/api-session-spec.js | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/spec/api-session-spec.js b/spec/api-session-spec.js index 9529ec5e00..25a09ba313 100644 --- a/spec/api-session-spec.js +++ b/spec/api-session-spec.js @@ -18,7 +18,7 @@ describe('session module', function () { var url = 'http://127.0.0.1' var partitionName = 'temp' var protocolName = 'sp' - const tempProtocol = session.fromPartition(partitionName).protocol + const partitionProtocol = session.fromPartition(partitionName).protocol const protocol = session.defaultSession.protocol beforeEach(function () { @@ -288,19 +288,23 @@ describe('session module', function () { }) }) + afterEach(function (done) { + partitionProtocol.unregisterProtocol(protocolName, () => done()) + }) + it('handles requests from a partition', function (done) { var handler = function (error, callback) { callback({ data: 'test' }) } - tempProtocol.registerStringProtocol(protocolName, handler, function (error) { + partitionProtocol.registerStringProtocol(protocolName, handler, function (error) { if (error) { return done(error) } protocol.isProtocolHandled(protocolName, function (result) { assert.equal(result, false) - tempProtocol.isProtocolHandled(protocolName, function (result) { + partitionProtocol.isProtocolHandled(protocolName, function (result) { assert.equal(result, true) w.webContents.on('did-finish-load', function () { done()