Reparent SWProcessManager onto SWContextWrapper.

This will allow Wrapper::Shutdown to drop all process references synchronously
in a future change, which will make it possible to shutdown Chrome with service
workers running and without hitting the DCHECK in
ProfileDestroyer::DestroyProfileWhenAppropriate().

BUG=368570

Review URL: https://codereview.chromium.org/292973003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@272839 0039d316-1c4b-4281-b951-d872f2087c98

content/browser/service_worker/embedded_worker_test_helper.cc

@@ -23,10 +23,7 @@ EmbeddedWorkerTestHelper::EmbeddedWorkerTestHelper(int mock_render_process_id)
                          base::MessageLoopProxy::current(),
                          base::MessageLoopProxy::current(),
                          NULL);
-  scoped_ptr<ServiceWorkerProcessManager> process_manager(
-      new ServiceWorkerProcessManager(wrapper_));
-  process_manager->SetProcessIdForTest(mock_render_process_id);
-  wrapper_->context()->SetProcessManagerForTest(process_manager.Pass());
+  wrapper_->process_manager()->SetProcessIdForTest(mock_render_process_id);
   registry()->AddChildProcessSender(mock_render_process_id, this);
 }
 

content/browser/service_worker/service_worker_context_core.cc

@@ -82,14 +82,16 @@ ServiceWorkerContextCore::ServiceWorkerContextCore(
     base::MessageLoopProxy* disk_cache_thread,
     quota::QuotaManagerProxy* quota_manager_proxy,
     ObserverListThreadSafe<ServiceWorkerContextObserver>* observer_list,
-    scoped_ptr<ServiceWorkerProcessManager> process_manager)
+    ServiceWorkerContextWrapper* wrapper)
     : weak_factory_(this),
-      storage_(new ServiceWorkerStorage(
-          path, AsWeakPtr(), database_task_runner, disk_cache_thread,
-          quota_manager_proxy)),
+      wrapper_(wrapper),
+      storage_(new ServiceWorkerStorage(path,
+                                        AsWeakPtr(),
+                                        database_task_runner,
+                                        disk_cache_thread,
+                                        quota_manager_proxy)),
       embedded_worker_registry_(new EmbeddedWorkerRegistry(AsWeakPtr())),
       job_coordinator_(new ServiceWorkerJobCoordinator(AsWeakPtr())),
-      process_manager_(process_manager.Pass()),
       next_handle_id_(0),
       observer_list_(observer_list) {
 }
@@ -307,4 +309,8 @@ void ServiceWorkerContextCore::OnReportConsoleMessage(
           source_identifier, message_level, message, line_number, source_url));
 }
 
+ServiceWorkerProcessManager* ServiceWorkerContextCore::process_manager() {
+  return wrapper_->process_manager();
+}
+
 }  // namespace content

content/browser/service_worker/service_worker_context_core.h

@@ -37,6 +37,7 @@ namespace content {
 
 class EmbeddedWorkerRegistry;
 class ServiceWorkerContextObserver;
+class ServiceWorkerContextWrapper;
 class ServiceWorkerHandle;
 class ServiceWorkerJobCoordinator;
 class ServiceWorkerProviderHost;
@@ -91,7 +92,7 @@ class CONTENT_EXPORT ServiceWorkerContextCore
       base::MessageLoopProxy* disk_cache_thread,
       quota::QuotaManagerProxy* quota_manager_proxy,
       ObserverListThreadSafe<ServiceWorkerContextObserver>* observer_list,
-      scoped_ptr<ServiceWorkerProcessManager> process_manager);
+      ServiceWorkerContextWrapper* wrapper);
   virtual ~ServiceWorkerContextCore();
 
   // ServiceWorkerVersion::Listener overrides.
@@ -111,9 +112,7 @@ class CONTENT_EXPORT ServiceWorkerContextCore
                                       const GURL& source_url) OVERRIDE;
 
   ServiceWorkerStorage* storage() { return storage_.get(); }
-  ServiceWorkerProcessManager* process_manager() {
-    return process_manager_.get();
-  }
+  ServiceWorkerProcessManager* process_manager();
   EmbeddedWorkerRegistry* embedded_worker_registry() {
     return embedded_worker_registry_.get();
   }
@@ -159,12 +158,6 @@ class CONTENT_EXPORT ServiceWorkerContextCore
     return weak_factory_.GetWeakPtr();
   }
 
-  // Allows tests to change how processes are created.
-  void SetProcessManagerForTest(
-      scoped_ptr<ServiceWorkerProcessManager> new_process_manager) {
-    process_manager_ = new_process_manager.Pass();
-  }
-
  private:
   typedef std::map<int64, ServiceWorkerRegistration*> RegistrationsMap;
   typedef std::map<int64, ServiceWorkerVersion*> VersionMap;
@@ -184,11 +177,14 @@ class CONTENT_EXPORT ServiceWorkerContextCore
                               ServiceWorkerStatusCode status);
 
   base::WeakPtrFactory<ServiceWorkerContextCore> weak_factory_;
+  // It's safe to store a raw pointer instead of a scoped_refptr to |wrapper_|
+  // because the Wrapper::Shutdown call that hops threads to destroy |this| uses
+  // Bind() to hold a reference to |wrapper_| until |this| is fully destroyed.
+  ServiceWorkerContextWrapper* wrapper_;
   ProcessToProviderMap providers_;
   scoped_ptr<ServiceWorkerStorage> storage_;
   scoped_refptr<EmbeddedWorkerRegistry> embedded_worker_registry_;
   scoped_ptr<ServiceWorkerJobCoordinator> job_coordinator_;
-  scoped_ptr<ServiceWorkerProcessManager> process_manager_;
   std::map<int64, ServiceWorkerRegistration*> live_registrations_;
   std::map<int64, ServiceWorkerVersion*> live_versions_;
   int next_handle_id_;

content/browser/service_worker/service_worker_context_wrapper.cc

@@ -18,7 +18,7 @@ ServiceWorkerContextWrapper::ServiceWorkerContextWrapper(
     BrowserContext* browser_context)
     : observer_list_(
           new ObserverListThreadSafe<ServiceWorkerContextObserver>()),
-      browser_context_(browser_context) {
+      process_manager_(new ServiceWorkerProcessManager(browser_context)) {
 }
 
 ServiceWorkerContextWrapper::~ServiceWorkerContextWrapper() {
@@ -39,16 +39,12 @@ void ServiceWorkerContextWrapper::Init(
 }
 
 void ServiceWorkerContextWrapper::Shutdown() {
-  if (!BrowserThread::CurrentlyOn(BrowserThread::IO)) {
-    DCHECK_CURRENTLY_ON(BrowserThread::UI);
-    browser_context_ = NULL;
-    BrowserThread::PostTask(
-        BrowserThread::IO, FROM_HERE,
-        base::Bind(&ServiceWorkerContextWrapper::Shutdown, this));
-    return;
-  }
-  // Breaks the reference cycle through ServiceWorkerProcessManager.
-  context_core_.reset();
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  process_manager_->Shutdown();
+  BrowserThread::PostTask(
+      BrowserThread::IO,
+      FROM_HERE,
+      base::Bind(&ServiceWorkerContextWrapper::ShutdownOnIO, this));
 }
 
 ServiceWorkerContextCore* ServiceWorkerContextWrapper::context() {
@@ -149,13 +145,17 @@ void ServiceWorkerContextWrapper::InitInternal(
     return;
   }
   DCHECK(!context_core_);
-  context_core_.reset(new ServiceWorkerContextCore(
-      user_data_directory,
-      database_task_runner,
-      disk_cache_thread,
-      quota_manager_proxy,
-      observer_list_,
-      make_scoped_ptr(new ServiceWorkerProcessManager(this))));
+  context_core_.reset(new ServiceWorkerContextCore(user_data_directory,
+                                                   database_task_runner,
+                                                   disk_cache_thread,
+                                                   quota_manager_proxy,
+                                                   observer_list_,
+                                                   this));
+}
+
+void ServiceWorkerContextWrapper::ShutdownOnIO() {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  context_core_.reset();
 }
 
 }  // namespace content

content/browser/service_worker/service_worker_context_wrapper.h

@@ -49,6 +49,11 @@ class CONTENT_EXPORT ServiceWorkerContextWrapper
   // The core context is only for use on the IO thread.
   ServiceWorkerContextCore* context();
 
+  // The process manager can be used on either UI or IO.
+  ServiceWorkerProcessManager* process_manager() {
+    return process_manager_.get();
+  }
+
   // ServiceWorkerContext implementation:
   virtual void RegisterServiceWorker(
       const GURL& pattern,
@@ -71,11 +76,12 @@ class CONTENT_EXPORT ServiceWorkerContextWrapper
                     base::SequencedTaskRunner* database_task_runner,
                     base::MessageLoopProxy* disk_cache_thread,
                     quota::QuotaManagerProxy* quota_manager_proxy);
+  void ShutdownOnIO();
 
   const scoped_refptr<ObserverListThreadSafe<ServiceWorkerContextObserver> >
       observer_list_;
+  const scoped_ptr<ServiceWorkerProcessManager> process_manager_;
   // Cleared in Shutdown():
-  BrowserContext* browser_context_;
   scoped_ptr<ServiceWorkerContextCore> context_core_;
 };
 

content/browser/service_worker/service_worker_process_manager.cc

@@ -35,8 +35,8 @@ ServiceWorkerProcessManager::ProcessInfo::~ProcessInfo() {
 }
 
 ServiceWorkerProcessManager::ServiceWorkerProcessManager(
-    ServiceWorkerContextWrapper* context_wrapper)
-    : context_wrapper_(context_wrapper),
+    BrowserContext* browser_context)
+    : browser_context_(browser_context),
       process_id_for_test_(-1),
       weak_this_factory_(this),
       weak_this_(weak_this_factory_.GetWeakPtr()) {
@@ -44,6 +44,13 @@ ServiceWorkerProcessManager::ServiceWorkerProcessManager(
 
 ServiceWorkerProcessManager::~ServiceWorkerProcessManager() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DCHECK(browser_context_ == NULL)
+      << "Call Shutdown() before destroying |this|, so that racing method "
+      << "invocations don't use a destroyed BrowserContext.";
+}
+
+void ServiceWorkerProcessManager::Shutdown() {
+  browser_context_ = NULL;
 }
 
 void ServiceWorkerProcessManager::AllocateWorkerProcess(
@@ -91,7 +98,7 @@ void ServiceWorkerProcessManager::AllocateWorkerProcess(
     }
   }
 
-  if (!context_wrapper_->browser_context_) {
+  if (!browser_context_) {
     // Shutdown has started.
     BrowserThread::PostTask(
         BrowserThread::IO,
@@ -100,8 +107,8 @@ void ServiceWorkerProcessManager::AllocateWorkerProcess(
     return;
   }
   // No existing processes available; start a new one.
-  scoped_refptr<SiteInstance> site_instance = SiteInstance::CreateForURL(
-      context_wrapper_->browser_context_, script_url);
+  scoped_refptr<SiteInstance> site_instance =
+      SiteInstance::CreateForURL(browser_context_, script_url);
   RenderProcessHost* rph = site_instance->GetProcess();
   // This Init() call posts a task to the IO thread that adds the RPH's
   // ServiceWorkerDispatcherHost to the

content/browser/service_worker/service_worker_process_manager.h

@@ -18,21 +18,25 @@ class GURL;
 namespace content {
 
 class BrowserContext;
-class ServiceWorkerContextWrapper;
 class SiteInstance;
 
 // Interacts with the UI thread to keep RenderProcessHosts alive while the
-// ServiceWorker system is using them.  Each instance of
+// ServiceWorker system is using them. Each instance of
 // ServiceWorkerProcessManager is destroyed on the UI thread shortly after its
-// ServiceWorkerContextCore is destroyed on the IO thread.
+// ServiceWorkerContextWrapper is destroyed.
 class CONTENT_EXPORT ServiceWorkerProcessManager {
  public:
-  // |*this| must be owned by |context_wrapper|->context().
-  explicit ServiceWorkerProcessManager(
-      ServiceWorkerContextWrapper* context_wrapper);
+  // |*this| must be owned by a ServiceWorkerContextWrapper in a
+  // StoragePartition within |browser_context|.
+  explicit ServiceWorkerProcessManager(BrowserContext* browser_context);
 
+  // Shutdown must be called before the ProcessManager is destroyed.
   ~ServiceWorkerProcessManager();
 
+  // Synchronously prevents new processes from being allocated.
+  // TODO(jyasskin): Drop references to RenderProcessHosts too.
+  void Shutdown();
+
   // Returns a reference to a running process suitable for starting the Service
   // Worker at |script_url|. Processes in |process_ids| will be checked in order
   // for existence, and if none exist, then a new process will be created. Posts
@@ -80,10 +84,8 @@ class CONTENT_EXPORT ServiceWorkerProcessManager {
     int process_id;
   };
 
-  // These fields are only accessed on the UI thread after construction.
-  // The reference cycle through context_wrapper_ is broken in
-  // ServiceWorkerContextWrapper::Shutdown().
-  scoped_refptr<ServiceWorkerContextWrapper> context_wrapper_;
+  // These fields are only accessed on the UI thread.
+  BrowserContext* browser_context_;
 
   // Maps the ID of a running EmbeddedWorkerInstance to information about the
   // process it's running inside. Since the Instances themselves live on the IO

content/browser/service_worker/service_worker_provider_host_unittest.cc

@@ -23,13 +23,13 @@ class ServiceWorkerProviderHostTest : public testing::Test {
   virtual ~ServiceWorkerProviderHostTest() {}
 
   virtual void SetUp() OVERRIDE {
-    context_.reset(new ServiceWorkerContextCore(
-        base::FilePath(),
-        base::MessageLoopProxy::current(),
-        base::MessageLoopProxy::current(),
-        NULL,
-        NULL,
-        scoped_ptr<ServiceWorkerProcessManager>()));
+    context_.reset(
+        new ServiceWorkerContextCore(base::FilePath(),
+                                     base::MessageLoopProxy::current(),
+                                     base::MessageLoopProxy::current(),
+                                     NULL,
+                                     NULL,
+                                     NULL));
 
     scope_ = GURL("http://www.example.com/*");
     script_url_ = GURL("http://www.example.com/service_worker.js");

content/browser/service_worker/service_worker_registration_unittest.cc

@@ -22,13 +22,13 @@ class ServiceWorkerRegistrationTest : public testing::Test {
       : io_thread_(BrowserThread::IO, &message_loop_) {}
 
   virtual void SetUp() OVERRIDE {
-    context_.reset(new ServiceWorkerContextCore(
-        base::FilePath(),
-        base::MessageLoopProxy::current(),
-        base::MessageLoopProxy::current(),
-        NULL,
-        NULL,
-        scoped_ptr<ServiceWorkerProcessManager>()));
+    context_.reset(
+        new ServiceWorkerContextCore(base::FilePath(),
+                                     base::MessageLoopProxy::current(),
+                                     base::MessageLoopProxy::current(),
+                                     NULL,
+                                     NULL,
+                                     NULL));
     context_ptr_ = context_->AsWeakPtr();
   }
 

content/browser/service_worker/service_worker_storage_unittest.cc

@@ -150,13 +150,13 @@ class ServiceWorkerStorageTest : public testing::Test {
   }
 
   virtual void SetUp() OVERRIDE {
-    context_.reset(new ServiceWorkerContextCore(
-        base::FilePath(),
-        base::MessageLoopProxy::current(),
-        base::MessageLoopProxy::current(),
-        NULL,
-        NULL,
-        scoped_ptr<ServiceWorkerProcessManager>()));
+    context_.reset(
+        new ServiceWorkerContextCore(base::FilePath(),
+                                     base::MessageLoopProxy::current(),
+                                     base::MessageLoopProxy::current(),
+                                     NULL,
+                                     NULL,
+                                     NULL));
     context_ptr_ = context_->AsWeakPtr();
   }