Merge pull request #6073 from electron/protocol-crash

Make api::Protocol thread safe

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,9 @@ namespace atom {
 namespace api {
 
 Protocol::Protocol(v8::Isolate* isolate, AtomBrowserContext* browser_context)
-    : request_context_getter_(browser_context->GetRequestContext()),
+    : request_context_getter_(static_cast<brightray::URLRequestContextGetter*>(
-      job_factory_(browser_context->job_factory()) {
+          browser_context->GetRequestContext())),
-  CHECK(job_factory_);
+      weak_factory_(this) {
   Init(isolate);
 }
 
@@ -46,30 +45,37 @@ 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,
-                 base::Unretained(this), callback));
+                 GetWeakPtr(), callback));
 }
 
+// static
 Protocol::ProtocolError Protocol::UnregisterProtocolInIO(
+    scoped_refptr<brightray::URLRequestContextGetter> request_context_getter,
     const std::string& scheme) {
-  if (!job_factory_->HasProtocolHandler(scheme))
+  auto job_factory = static_cast<AtomURLRequestJobFactory*>(
+      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;
 }
 
 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,
-                 base::Unretained(this), scheme),
+                 request_context_getter_, scheme),
       callback);
 }
 
-bool Protocol::IsProtocolHandledInIO(const std::string& scheme) {
+// static
-  return job_factory_->IsHandledProtocol(scheme);
+bool Protocol::IsProtocolHandledInIO(
+    scoped_refptr<brightray::URLRequestContextGetter> request_context_getter,
+    const std::string& scheme) {
+  return request_context_getter->job_factory()->IsHandledProtocol(scheme);
 }
 
 void Protocol::UninterceptProtocol(
@@ -79,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,
-                 base::Unretained(this), callback));
+                 GetWeakPtr(), callback));
 }
 
+// static
 Protocol::ProtocolError Protocol::UninterceptProtocolInIO(
+    scoped_refptr<brightray::URLRequestContextGetter> request_context_getter,
     const std::string& scheme) {
-  if (!original_protocols_.contains(scheme))
+  return static_cast<AtomURLRequestJobFactory*>(
-    return PROTOCOL_NOT_INTERCEPTED;
+      request_context_getter->job_factory())->UninterceptProtocol(scheme) ?
-  job_factory_->ReplaceProtocol(scheme,
+          PROTOCOL_OK : PROTOCOL_NOT_INTERCEPTED;
-                                original_protocols_.take_and_erase(scheme));
-  return PROTOCOL_OK;
 }
 
 void Protocol::OnIOCompleted(
@@ -121,6 +127,13 @@ std::string Protocol::ErrorCodeToString(ProtocolError error) {
   }
 }
 
+AtomURLRequestJobFactory* Protocol::GetJobFactoryInIO() const {
+  request_context_getter_->GetURLRequestContext();  // Force init.
+  return static_cast<AtomURLRequestJobFactory*>(
+      static_cast<brightray::URLRequestContextGetter*>(
+          request_context_getter_.get())->job_factory());
+}
+
 // static
 mate::Handle<Protocol> Protocol::Create(
     v8::Isolate* isolate, AtomBrowserContext* browser_context) {

atom/browser/api/atom_api_protocol.h

@@ -10,28 +10,22 @@
 #include <vector>
 
 #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"
 #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 {
 
-class AtomBrowserContext;
-class AtomURLRequestJobFactory;
-
 namespace api {
 
 class Protocol : public mate::TrackableObject<Protocol> {
@@ -80,13 +74,13 @@ class Protocol : public mate::TrackableObject<Protocol> {
         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<net::URLRequestContextGetter> request_context_;
     Protocol::Handler handler_;
 
     DISALLOW_COPY_AND_ASSIGN(CustomProtocolHandler);
@@ -105,19 +99,24 @@ class Protocol : public mate::TrackableObject<Protocol> {
     content::BrowserThread::PostTaskAndReplyWithResult(
         content::BrowserThread::IO, FROM_HERE,
         base::Bind(&Protocol::RegisterProtocolInIO<RequestJob>,
-                   base::Unretained(this), scheme, handler),
+                   request_context_getter_, isolate(), scheme, handler),
         base::Bind(&Protocol::OnIOCompleted,
-                   base::Unretained(this), callback));
+                   GetWeakPtr(), callback));
   }
   template<typename RequestJob>
-  ProtocolError RegisterProtocolInIO(const std::string& scheme,
+  static ProtocolError RegisterProtocolInIO(
-                                     const Handler& handler) {
+      scoped_refptr<brightray::URLRequestContextGetter> request_context_getter,
-    if (job_factory_->IsHandledProtocol(scheme))
+      v8::Isolate* isolate,
+      const std::string& scheme,
+      const Handler& handler) {
+    auto job_factory = static_cast<AtomURLRequestJobFactory*>(
+        request_context_getter->job_factory());
+    if (job_factory->IsHandledProtocol(scheme))
       return PROTOCOL_REGISTERED;
     std::unique_ptr<CustomProtocolHandler<RequestJob>> protocol_handler(
         new CustomProtocolHandler<RequestJob>(
-            isolate(), request_context_getter_, handler));
+            isolate, request_context_getter.get(), 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;
@@ -125,12 +124,16 @@ class Protocol : public mate::TrackableObject<Protocol> {
 
   // 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<brightray::URLRequestContextGetter> 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<brightray::URLRequestContextGetter> request_context_getter,
+      const std::string& scheme);
 
   // Replace the protocol handler with a new one.
   template<typename RequestJob>
@@ -142,32 +145,36 @@ class Protocol : public mate::TrackableObject<Protocol> {
     content::BrowserThread::PostTaskAndReplyWithResult(
         content::BrowserThread::IO, FROM_HERE,
         base::Bind(&Protocol::InterceptProtocolInIO<RequestJob>,
-                   base::Unretained(this), scheme, handler),
+                   request_context_getter_, isolate(), scheme, handler),
         base::Bind(&Protocol::OnIOCompleted,
-                   base::Unretained(this), callback));
+                   GetWeakPtr(), callback));
   }
   template<typename RequestJob>
-  ProtocolError InterceptProtocolInIO(const std::string& scheme,
+  static ProtocolError InterceptProtocolInIO(
-                                      const Handler& handler) {
+      scoped_refptr<brightray::URLRequestContextGetter> request_context_getter,
-    if (!job_factory_->IsHandledProtocol(scheme))
+      v8::Isolate* isolate,
+      const std::string& scheme,
+      const Handler& handler) {
+    auto job_factory = static_cast<AtomURLRequestJobFactory*>(
+        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<CustomProtocolHandler<RequestJob>> protocol_handler(
         new CustomProtocolHandler<RequestJob>(
-            isolate(), request_context_getter_, handler));
+            isolate, request_context_getter.get(), handler));
-    original_protocols_.set(
+    if (!job_factory->InterceptProtocol(scheme, std::move(protocol_handler)))
-        scheme,
+      return PROTOCOL_INTERCEPTED;
-        job_factory_->ReplaceProtocol(scheme, std::move(protocol_handler)));
     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<brightray::URLRequestContextGetter> request_context_getter,
+      const std::string& scheme);
 
   // Convert error code to JS exception and call the callback.
   void OnIOCompleted(const CompletionCallback& callback, ProtocolError error);
@@ -175,15 +182,14 @@ class Protocol : public mate::TrackableObject<Protocol> {
   // Convert error code to string.
   std::string ErrorCodeToString(ProtocolError error);
 
-  net::URLRequestContextGetter* request_context_getter_;
+  AtomURLRequestJobFactory* GetJobFactoryInIO() const;
 
-  // Map that stores the original protocols of schemes.
+  base::WeakPtr<Protocol> GetWeakPtr() {
-  using OriginalProtocolsMap = base::ScopedPtrHashMap<
+    return weak_factory_.GetWeakPtr();
-      std::string,
+  }
-      std::unique_ptr<net::URLRequestJobFactory::ProtocolHandler>>;
-  OriginalProtocolsMap original_protocols_;
 
-  AtomURLRequestJobFactory* job_factory_;  // weak ref
+  scoped_refptr<brightray::URLRequestContextGetter> request_context_getter_;
+  base::WeakPtrFactory<Protocol> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(Protocol);
 };

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<net::URLRequestJobFactory>
 AtomBrowserContext::CreateURLRequestJobFactory(
-    content::ProtocolHandlerMap* handlers,
+    content::ProtocolHandlerMap* protocol_handlers) {
-    content::URLRequestInterceptorScopedVector* interceptors) {
+  std::unique_ptr<AtomURLRequestJobFactory> job_factory(
-  std::unique_ptr<AtomURLRequestJobFactory> job_factory(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.
+  return std::move(job_factory);
-  std::unique_ptr<net::URLRequestJobFactory> 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;
 }
 
 net::HttpCache::BackendFactory*

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<net::URLRequestJobFactory> CreateURLRequestJobFactory(
-      content::ProtocolHandlerMap* handlers,
+      content::ProtocolHandlerMap* protocol_handlers) override;
-      content::URLRequestInterceptorScopedVector* interceptors) override;
   net::HttpCache::BackendFactory* CreateHttpCacheBackendFactory(
       const base::FilePath& base_path) override;
   std::unique_ptr<net::CertVerifier> 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);

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<ProtocolHandler> AtomURLRequestJobFactory::ReplaceProtocol(
+bool AtomURLRequestJobFactory::InterceptProtocol(
     const std::string& scheme,
     std::unique_ptr<ProtocolHandler> protocol_handler) {
-  if (!ContainsKey(protocol_handler_map_, scheme))
+  if (!ContainsKey(protocol_handler_map_, scheme) ||
-    return nullptr;
+      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(

atom/browser/net/atom_url_request_job_factory.h

@@ -7,11 +7,11 @@
 #define ATOM_BROWSER_NET_ATOM_URL_REQUEST_JOB_FACTORY_H_
 
 #include <map>
+#include <memory>
 #include <string>
 #include <vector>
 
-#include "base/memory/scoped_ptr.h"
+#include "base/containers/scoped_ptr_hash_map.h"
-#include "base/synchronization/lock.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<ProtocolHandler> protocol_handler);
 
-  // Intercepts the ProtocolHandler for a scheme. Returns the original protocol
+  // Intercepts the ProtocolHandler for a scheme.
-  // handler on success, otherwise returns NULL.
+  bool InterceptProtocol(
-  std::unique_ptr<ProtocolHandler> ReplaceProtocol(
       const std::string& scheme,
       std::unique_ptr<ProtocolHandler> 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<ProtocolHandler>>;
+  // Can only be accessed in IO thread.
+  OriginalProtocolsMap original_protocols_;
+
   DISALLOW_COPY_AND_ASSIGN(AtomURLRequestJobFactory);
 };
 

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

vendor/brightray

@@ -1 +1 @@
-Subproject commit 55423bdca586a20fe052f6c0e19565710dcebd42
+Subproject commit 717d92968d7959814060b9409c4720ab647bd90e