Save running SW instance info, including its SiteInstance, into the ProcessManager.

We can use this map to drop all the process references on shutdown, in order to
fix http://crbug.com/368570.

This starts to fix an assumption in the content layer that SiteInstances outlive
their RPHs so that process permissions can be controlled through the
SiteInstance instead of by process id. However, there's still work to do before
that's completely fixed.

This reverts https://codereview.chromium.org/296053013/ and avoids the incorrect
down-cast that caused https://codereview.chromium.org/292903002 to break the
Asan bot.

BUG=368570

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

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

content/browser/service_worker/embedded_worker_instance.cc

@@ -93,7 +93,7 @@ EmbeddedWorkerInstance::~EmbeddedWorkerInstance() {
   if (worker_devtools_agent_route_id_ != MSG_ROUTING_NONE)
     NotifyWorkerDestroyed(process_id_, worker_devtools_agent_route_id_);
   if (context_ && process_id_ != -1)
-    context_->process_manager()->ReleaseWorkerProcess(process_id_);
+    context_->process_manager()->ReleaseWorkerProcess(embedded_worker_id_);
   registry_->RemoveWorker(process_id_, embedded_worker_id_);
 }
 
@@ -117,6 +117,7 @@ void EmbeddedWorkerInstance::Start(int64 service_worker_version_id,
   params->worker_devtools_agent_route_id = MSG_ROUTING_NONE;
   params->pause_on_start = false;
   context_->process_manager()->AllocateWorkerProcess(
+      embedded_worker_id_,
       SortProcesses(possible_process_ids),
       script_url,
       base::Bind(&EmbeddedWorkerInstance::RunProcessAllocated,
@@ -186,7 +187,11 @@ void EmbeddedWorkerInstance::RunProcessAllocated(
     return;
   }
   if (!instance) {
-    context->process_manager()->ReleaseWorkerProcess(process_id);
+    if (status == SERVICE_WORKER_OK) {
+      // We only have a process allocated if the status is OK.
+      context->process_manager()->ReleaseWorkerProcess(
+          params->embedded_worker_id);
+    }
     callback.Run(SERVICE_WORKER_ERROR_ABORT);
     return;
   }
@@ -249,7 +254,7 @@ void EmbeddedWorkerInstance::OnStopped() {
   if (worker_devtools_agent_route_id_ != MSG_ROUTING_NONE)
     NotifyWorkerDestroyed(process_id_, worker_devtools_agent_route_id_);
   if (context_)
-    context_->process_manager()->ReleaseWorkerProcess(process_id_);
+    context_->process_manager()->ReleaseWorkerProcess(embedded_worker_id_);
   status_ = STOPPED;
   process_id_ = -1;
   thread_id_ = -1;

content/browser/service_worker/embedded_worker_instance.h

@@ -106,6 +106,7 @@ class CONTENT_EXPORT EmbeddedWorkerInstance {
 
   friend class EmbeddedWorkerRegistry;
   FRIEND_TEST_ALL_PREFIXES(EmbeddedWorkerInstanceTest, StartAndStop);
+  FRIEND_TEST_ALL_PREFIXES(EmbeddedWorkerInstanceTest, SortProcesses);
 
   typedef std::map<int, int> ProcessRefMap;
 

content/browser/service_worker/embedded_worker_instance_unittest.cc

@@ -12,6 +12,7 @@
 #include "content/browser/service_worker/service_worker_context_wrapper.h"
 #include "content/common/service_worker/embedded_worker_messages.h"
 #include "content/public/test/test_browser_thread_bundle.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace content {
@@ -129,7 +130,7 @@ TEST_F(EmbeddedWorkerInstanceTest, InstanceDestroyedBeforeStartFinishes) {
       ipc_sink()->GetUniqueMessageMatching(EmbeddedWorkerMsg_StartWorker::ID));
 }
 
-TEST_F(EmbeddedWorkerInstanceTest, ChooseProcess) {
+TEST_F(EmbeddedWorkerInstanceTest, SortProcesses) {
   scoped_ptr<EmbeddedWorkerInstance> worker =
       embedded_worker_registry()->CreateWorker();
   EXPECT_EQ(EmbeddedWorkerInstance::STOPPED, worker->status());
@@ -145,22 +146,22 @@ TEST_F(EmbeddedWorkerInstanceTest, ChooseProcess) {
   helper_->SimulateAddProcessToWorker(embedded_worker_id, 3);
 
   // Process 3 has the biggest # of references and it should be chosen.
-  ServiceWorkerStatusCode status;
-  base::RunLoop run_loop;
-  worker->Start(
-      1L,
-      GURL("http://example.com/*"),
-      GURL("http://example.com/worker.js"),
-      std::vector<int>(),
-      base::Bind(&SaveStatusAndCall, &status, run_loop.QuitClosure()));
-  run_loop.Run();
-  EXPECT_EQ(SERVICE_WORKER_OK, status) << ServiceWorkerStatusToString(status);
-  EXPECT_EQ(EmbeddedWorkerInstance::STARTING, worker->status());
-  EXPECT_EQ(3, worker->process_id());
-
-  // Wait until started message is sent back.
-  base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(EmbeddedWorkerInstance::RUNNING, worker->status());
+  EXPECT_THAT(worker->SortProcesses(std::vector<int>()),
+              testing::ElementsAre(3, 2, 1));
+  EXPECT_EQ(-1, worker->process_id());
+
+  // Argument processes are added to the existing set, but only for a single
+  // call.
+  std::vector<int> registering_processes;
+  registering_processes.push_back(1);
+  registering_processes.push_back(1);
+  registering_processes.push_back(1);
+  registering_processes.push_back(4);
+  EXPECT_THAT(worker->SortProcesses(registering_processes),
+              testing::ElementsAre(1, 3, 2, 4));
+
+  EXPECT_THAT(worker->SortProcesses(std::vector<int>()),
+              testing::ElementsAre(3, 2, 1));
 }
 
 }  // namespace content

content/browser/service_worker/embedded_worker_test_helper.cc

@@ -15,10 +15,6 @@
 
 namespace content {
 
-static bool AlwaysTrue(int process_id) {
-  return true;
-}
-
 EmbeddedWorkerTestHelper::EmbeddedWorkerTestHelper(int mock_render_process_id)
     : wrapper_(new ServiceWorkerContextWrapper(NULL)),
       next_thread_id_(0),
@@ -29,8 +25,7 @@ EmbeddedWorkerTestHelper::EmbeddedWorkerTestHelper(int mock_render_process_id)
                          NULL);
   scoped_ptr<ServiceWorkerProcessManager> process_manager(
       new ServiceWorkerProcessManager(wrapper_));
-  process_manager->SetProcessRefcountOpsForTest(base::Bind(AlwaysTrue),
-                                                base::Bind(AlwaysTrue));
+  process_manager->SetProcessIdForTest(mock_render_process_id);
   wrapper_->context()->SetProcessManagerForTest(process_manager.Pass());
   registry()->AddChildProcessSender(mock_render_process_id, this);
 }

content/browser/service_worker/service_worker_browsertest.cc

@@ -550,10 +550,6 @@ class ServiceWorkerBlackBoxBrowserTest : public ServiceWorkerBrowserTest {
     continuation.Run();
   }
 
-  int RenderProcessID() {
-    return shell()->web_contents()->GetRenderProcessHost()->GetID();
-  }
-
   void FindRegistrationOnIO(const GURL& document_url,
                             ServiceWorkerStatusCode* status,
                             GURL* script_url,
@@ -583,7 +579,21 @@ class ServiceWorkerBlackBoxBrowserTest : public ServiceWorkerBrowserTest {
   }
 };
 
+static int CountRenderProcessHosts() {
+  int result = 0;
+  for (RenderProcessHost::iterator iter(RenderProcessHost::AllHostsIterator());
+       !iter.IsAtEnd();
+       iter.Advance()) {
+    result++;
+  }
+  return result;
+}
+
 IN_PROC_BROWSER_TEST_F(ServiceWorkerBlackBoxBrowserTest, Registration) {
+  // Close the only window to be sure we're not re-using its RenderProcessHost.
+  shell()->Close();
+  EXPECT_EQ(0, CountRenderProcessHosts());
+
   const std::string kWorkerUrl = "/service_worker/fetch_event.js";
 
   // Unregistering nothing should return true.
@@ -597,6 +607,19 @@ IN_PROC_BROWSER_TEST_F(ServiceWorkerBlackBoxBrowserTest, Registration) {
     run_loop.Run();
   }
 
+  // If we use a worker URL that doesn't exist, registration fails.
+  {
+    base::RunLoop run_loop;
+    public_context()->RegisterServiceWorker(
+        embedded_test_server()->GetURL("/*"),
+        embedded_test_server()->GetURL("/does/not/exist"),
+        base::Bind(&ServiceWorkerBlackBoxBrowserTest::ExpectResultAndRun,
+                   false,
+                   run_loop.QuitClosure()));
+    run_loop.Run();
+  }
+  EXPECT_EQ(0, CountRenderProcessHosts());
+
   // Register returns when the promise would be resolved.
   {
     base::RunLoop run_loop;
@@ -608,6 +631,7 @@ IN_PROC_BROWSER_TEST_F(ServiceWorkerBlackBoxBrowserTest, Registration) {
                    run_loop.QuitClosure()));
     run_loop.Run();
   }
+  EXPECT_EQ(1, CountRenderProcessHosts());
 
   // Registering again should succeed, although the algo still
   // might not be complete.
@@ -636,6 +660,9 @@ IN_PROC_BROWSER_TEST_F(ServiceWorkerBlackBoxBrowserTest, Registration) {
                    run_loop.QuitClosure()));
     run_loop.Run();
   }
+  EXPECT_GE(1, CountRenderProcessHosts()) << "Unregistering doesn't stop the "
+                                             "workers eagerly, so their RPHs "
+                                             "can still be running.";
 
   // Should not be able to find it.
   {

content/browser/service_worker/service_worker_context_unittest.cc

@@ -29,6 +29,7 @@ void SaveResponseCallback(bool* called,
                           ServiceWorkerStatusCode status,
                           int64 registration_id,
                           int64 version_id) {
+  EXPECT_EQ(SERVICE_WORKER_OK, status) << ServiceWorkerStatusToString(status);
   *called = true;
   *store_registration_id = registration_id;
   *store_version_id = version_id;

content/browser/service_worker/service_worker_process_manager.cc

@@ -12,9 +12,32 @@
 
 namespace content {
 
+static bool IncrementWorkerRefCountByPid(int process_id) {
+  RenderProcessHost* rph = RenderProcessHost::FromID(process_id);
+  if (!rph || rph->FastShutdownStarted())
+    return false;
+
+  static_cast<RenderProcessHostImpl*>(rph)->IncrementWorkerRefCount();
+  return true;
+}
+
+ServiceWorkerProcessManager::ProcessInfo::ProcessInfo(
+    const scoped_refptr<SiteInstance>& site_instance)
+    : site_instance(site_instance),
+      process_id(site_instance->GetProcess()->GetID()) {
+}
+
+ServiceWorkerProcessManager::ProcessInfo::ProcessInfo(int process_id)
+    : process_id(process_id) {
+}
+
+ServiceWorkerProcessManager::ProcessInfo::~ProcessInfo() {
+}
+
 ServiceWorkerProcessManager::ServiceWorkerProcessManager(
     ServiceWorkerContextWrapper* context_wrapper)
     : context_wrapper_(context_wrapper),
+      process_id_for_test_(-1),
       weak_this_factory_(this),
       weak_this_(weak_this_factory_.GetWeakPtr()) {
 }
@@ -24,26 +47,43 @@ ServiceWorkerProcessManager::~ServiceWorkerProcessManager() {
 }
 
 void ServiceWorkerProcessManager::AllocateWorkerProcess(
+    int embedded_worker_id,
     const std::vector<int>& process_ids,
     const GURL& script_url,
     const base::Callback<void(ServiceWorkerStatusCode, int process_id)>&
-        callback) const {
+        callback) {
   if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
     BrowserThread::PostTask(
         BrowserThread::UI,
         FROM_HERE,
         base::Bind(&ServiceWorkerProcessManager::AllocateWorkerProcess,
                    weak_this_,
+                   embedded_worker_id,
                    process_ids,
                    script_url,
                    callback));
     return;
   }
 
+  if (process_id_for_test_ != -1) {
+    // Let tests specify the returned process ID. Note: We may need to be able
+    // to specify the error code too.
+    BrowserThread::PostTask(
+        BrowserThread::IO,
+        FROM_HERE,
+        base::Bind(callback, SERVICE_WORKER_OK, process_id_for_test_));
+    return;
+  }
+
+  DCHECK(!ContainsKey(instance_info_, embedded_worker_id))
+      << embedded_worker_id << " already has a process allocated";
+
   for (std::vector<int>::const_iterator it = process_ids.begin();
        it != process_ids.end();
        ++it) {
-    if (IncrementWorkerRefcountByPid(*it)) {
+    if (IncrementWorkerRefCountByPid(*it)) {
+      instance_info_.insert(
+          std::make_pair(embedded_worker_id, ProcessInfo(*it)));
       BrowserThread::PostTask(BrowserThread::IO,
                               FROM_HERE,
                               base::Bind(callback, SERVICE_WORKER_OK, *it));
@@ -75,6 +115,9 @@ void ServiceWorkerProcessManager::AllocateWorkerProcess(
     return;
   }
 
+  instance_info_.insert(
+      std::make_pair(embedded_worker_id, ProcessInfo(site_instance)));
+
   static_cast<RenderProcessHostImpl*>(rph)->IncrementWorkerRefCount();
   BrowserThread::PostTask(
       BrowserThread::IO,
@@ -82,53 +125,38 @@ void ServiceWorkerProcessManager::AllocateWorkerProcess(
       base::Bind(callback, SERVICE_WORKER_OK, rph->GetID()));
 }
 
-void ServiceWorkerProcessManager::ReleaseWorkerProcess(int process_id) {
+void ServiceWorkerProcessManager::ReleaseWorkerProcess(int embedded_worker_id) {
   if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
     BrowserThread::PostTask(
         BrowserThread::UI,
         FROM_HERE,
         base::Bind(&ServiceWorkerProcessManager::ReleaseWorkerProcess,
                    weak_this_,
-                   process_id));
+                   embedded_worker_id));
     return;
   }
-  if (!DecrementWorkerRefcountByPid(process_id)) {
-    DCHECK(false) << "DecrementWorkerRef(" << process_id
-                  << ") doesn't match a previous IncrementWorkerRef";
+  if (process_id_for_test_ != -1) {
+    // Unittests don't increment or decrement the worker refcount of a
+    // RenderProcessHost.
+    return;
   }
-}
-
-void ServiceWorkerProcessManager::SetProcessRefcountOpsForTest(
-    const base::Callback<bool(int)>& increment_for_test,
-    const base::Callback<bool(int)>& decrement_for_test) {
-  increment_for_test_ = increment_for_test;
-  decrement_for_test_ = decrement_for_test;
-}
-
-bool ServiceWorkerProcessManager::IncrementWorkerRefcountByPid(
-    int process_id) const {
-  if (!increment_for_test_.is_null())
-    return increment_for_test_.Run(process_id);
-
-  RenderProcessHost* rph = RenderProcessHost::FromID(process_id);
-  if (rph && !rph->FastShutdownStarted()) {
-    static_cast<RenderProcessHostImpl*>(rph)->IncrementWorkerRefCount();
-    return true;
+  std::map<int, ProcessInfo>::iterator info =
+      instance_info_.find(embedded_worker_id);
+  DCHECK(info != instance_info_.end());
+  RenderProcessHost* rph = NULL;
+  if (info->second.site_instance) {
+    rph = info->second.site_instance->GetProcess();
+    DCHECK_EQ(info->second.process_id, rph->GetID())
+        << "A SiteInstance's process shouldn't get destroyed while we're "
+           "holding a reference to it. Was the reference actually held?";
+  } else {
+    rph = RenderProcessHost::FromID(info->second.process_id);
+    DCHECK(rph)
+        << "Process " << info->second.process_id
+        << " was destroyed unexpectedly. Did we actually hold a reference?";
   }
-
-  return false;
-}
-
-bool ServiceWorkerProcessManager::DecrementWorkerRefcountByPid(
-    int process_id) const {
-  if (!decrement_for_test_.is_null())
-    return decrement_for_test_.Run(process_id);
-
-  RenderProcessHost* rph = RenderProcessHost::FromID(process_id);
-  if (rph)
-    static_cast<RenderProcessHostImpl*>(rph)->DecrementWorkerRefCount();
-
-  return rph != NULL;
+  static_cast<RenderProcessHostImpl*>(rph)->DecrementWorkerRefCount();
+  instance_info_.erase(info);
 }
 
 }  // namespace content

content/browser/service_worker/service_worker_process_manager.h

@@ -5,6 +5,7 @@
 #ifndef CONTENT_BROWSER_SERVICE_WORKER_SERVICE_WORKER_PROCESS_MANAGER_H_
 #define CONTENT_BROWSER_SERVICE_WORKER_SERVICE_WORKER_PROCESS_MANAGER_H_
 
+#include <map>
 #include <vector>
 
 #include "base/callback.h"
@@ -18,6 +19,7 @@ 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
@@ -40,37 +42,65 @@ class CONTENT_EXPORT ServiceWorkerProcessManager {
   // Allocation can fail with SERVICE_WORKER_ERROR_START_WORKER_FAILED if
   // RenderProcessHost::Init fails.
   void AllocateWorkerProcess(
+      int embedded_worker_id,
       const std::vector<int>& process_ids,
       const GURL& script_url,
       const base::Callback<void(ServiceWorkerStatusCode, int process_id)>&
-          callback) const;
+          callback);
 
-  // Drops a reference to a process that was running a Service Worker.  This
-  // must match a call to AllocateWorkerProcess.
-  void ReleaseWorkerProcess(int process_id);
+  // Drops a reference to a process that was running a Service Worker, and its
+  // SiteInstance.  This must match a call to AllocateWorkerProcess.
+  void ReleaseWorkerProcess(int embedded_worker_id);
 
-  // |increment_for_test| and |decrement_for_test| define how to look up a
-  // process by ID and increment or decrement its worker reference count. This
-  // must be called before any reference to this object escapes to another
-  // thread, and is considered part of construction.
-  void SetProcessRefcountOpsForTest(
-      const base::Callback<bool(int)>& increment_for_test,
-      const base::Callback<bool(int)>& decrement_for_test);
+  // Sets a single process ID that will be used for all embedded workers.  This
+  // bypasses the work of creating a process and managing its worker refcount so
+  // that unittests can run without a BrowserContext.  The test is in charge of
+  // making sure this is only called on the same thread as runs the UI message
+  // loop.
+  void SetProcessIdForTest(int process_id) {
+    process_id_for_test_ = process_id;
+  }
 
  private:
-  bool IncrementWorkerRefcountByPid(int process_id) const;
-  bool DecrementWorkerRefcountByPid(int process_id) const;
+  // Information about the process for an EmbeddedWorkerInstance.
+  struct ProcessInfo {
+    explicit ProcessInfo(const scoped_refptr<SiteInstance>& site_instance);
+    explicit ProcessInfo(int process_id);
+    ~ProcessInfo();
+
+    // Stores the SiteInstance the Worker lives inside. This needs to outlive
+    // the instance's use of its RPH to uphold assumptions in the
+    // ContentBrowserClient interface.
+    scoped_refptr<SiteInstance> site_instance;
+
+    // In case the process was allocated without using a SiteInstance, we need
+    // to store a process ID to decrement a worker reference on shutdown.
+    // TODO(jyasskin): Implement http://crbug.com/372045 or thread a frame_id in
+    // so all processes can be allocated with a SiteInstance.
+    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_;
-  base::Callback<bool(int)> increment_for_test_;
-  base::Callback<bool(int)> decrement_for_test_;
+
+  // Maps the ID of a running EmbeddedWorkerInstance to information about the
+  // process it's running inside. Since the Instances themselves live on the IO
+  // thread, this can be slightly out of date:
+  //  * instance_info_ is populated while an Instance is STARTING and before
+  //    it's RUNNING.
+  //  * instance_info_ is depopulated in a message sent as the Instance becomes
+  //    STOPPED.
+  std::map<int, ProcessInfo> instance_info_;
+
+  // In unit tests, this will be returned as the process for all
+  // EmbeddedWorkerInstances.
+  int process_id_for_test_;
 
   // Used to double-check that we don't access *this after it's destroyed.
   base::WeakPtrFactory<ServiceWorkerProcessManager> weak_this_factory_;
-  base::WeakPtr<ServiceWorkerProcessManager> weak_this_;
+  const base::WeakPtr<ServiceWorkerProcessManager> weak_this_;
 };
 
 }  // namespace content