Fixed ServiceProcessControl::Launch to remember the Task supplied to it in a list so that multiple successive Launch calls will work. Also added separate success and failure tasks to Launch. Modified the browser test to test this new functionality. Modified callers.

BUG=58802
TEST=Browser tests, Cloud Print Proxy UI.

Review URL: http://codereview.chromium.org/3851001

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

6 files changed, 162 insertions, 68 deletions

diff --git a/chrome/browser/browser_main.cc b/chrome/browser/browser_main.cc
index f21b2e2..76c94fd 100644
--- a/chrome/browser/browser_main.cc
+++ b/chrome/browser/browser_main.cc
@@ -1451,7 +1451,7 @@ int BrowserMain(const MainFunctionParams& parameters) {
     if (user_prefs->GetBoolean(prefs::kRemotingHasSetupCompleted)) {
       ServiceProcessControl* control = ServiceProcessControlManager::instance()
           ->GetProcessControl(profile);
-       control->Launch(NULL);
+       control->Launch(NULL, NULL);
     }
   }
 
diff --git a/chrome/browser/printing/cloud_print/cloud_print_proxy_service.cc b/chrome/browser/printing/cloud_print/cloud_print_proxy_service.cc
index 9b46a95..3c63d7d 100644
--- a/chrome/browser/printing/cloud_print/cloud_print_proxy_service.cc
+++ b/chrome/browser/printing/cloud_print/cloud_print_proxy_service.cc
@@ -142,36 +142,30 @@ void CloudPrintProxyService::RefreshCloudPrintProxyStatus() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   ServiceProcessControl* process_control =
       ServiceProcessControlManager::instance()->GetProcessControl(profile_);
-  DCHECK(process_control && process_control->is_connected());
-  if (process_control && process_control->is_connected()) {
-    Callback2<bool, std::string>::Type* callback =
-         NewCallback(this, &CloudPrintProxyService::StatusCallback);
-    // GetCloudPrintProxyStatus takes ownership of callback.
-    process_control->GetCloudPrintProxyStatus(callback);
-  }
+  DCHECK(process_control->is_connected());
+  Callback2<bool, std::string>::Type* callback =
+       NewCallback(this, &CloudPrintProxyService::StatusCallback);
+  // GetCloudPrintProxyStatus takes ownership of callback.
+  process_control->GetCloudPrintProxyStatus(callback);
 }
 
 void CloudPrintProxyService::EnableCloudPrintProxy(const std::string& lsid,
                                                    const std::string& email) {
   ServiceProcessControl* process_control =
       ServiceProcessControlManager::instance()->GetProcessControl(profile_);
-  DCHECK(process_control && process_control->is_connected());
-  if (process_control->is_connected()) {
-    process_control->Send(new ServiceMsg_EnableCloudPrintProxy(lsid));
-    // Assume the IPC worked.
-    profile_->GetPrefs()->SetString(prefs::kCloudPrintEmail, email);
-  }
+  DCHECK(process_control->is_connected());
+  process_control->Send(new ServiceMsg_EnableCloudPrintProxy(lsid));
+  // Assume the IPC worked.
+  profile_->GetPrefs()->SetString(prefs::kCloudPrintEmail, email);
 }
 
 void CloudPrintProxyService::DisableCloudPrintProxy() {
   ServiceProcessControl* process_control =
       ServiceProcessControlManager::instance()->GetProcessControl(profile_);
-  DCHECK(process_control && process_control->is_connected());
-  if (process_control->is_connected()) {
-    process_control->Send(new ServiceMsg_DisableCloudPrintProxy);
-    // Assume the IPC worked.
-    profile_->GetPrefs()->SetString(prefs::kCloudPrintEmail, std::string());
-  }
+  DCHECK(process_control->is_connected());
+  process_control->Send(new ServiceMsg_DisableCloudPrintProxy);
+  // Assume the IPC worked.
+  profile_->GetPrefs()->SetString(prefs::kCloudPrintEmail, std::string());
 }
 
 void CloudPrintProxyService::StatusCallback(bool enabled, std::string email) {
@@ -184,7 +178,7 @@ bool CloudPrintProxyService::InvokeServiceTask(Task* task) {
       ServiceProcessControlManager::instance()->GetProcessControl(profile_);
   DCHECK(process_control);
   if (process_control)
-    process_control->Launch(task);
+    process_control->Launch(task, NULL);
   return !!process_control;
 }
 
diff --git a/chrome/browser/remoting/remoting_setup_flow.cc b/chrome/browser/remoting/remoting_setup_flow.cc
index 67a9050..f5b59379 100644
--- a/chrome/browser/remoting/remoting_setup_flow.cc
+++ b/chrome/browser/remoting/remoting_setup_flow.cc
@@ -214,9 +214,10 @@ void RemotingSetupFlow::OnIssueAuthTokenSuccess(const std::string& service,
     // TODO(hclam): This call only works on Windows. I need to make it work
     // on other platforms.
     service_process_helper_ = new RemotingServiceProcessHelper(this);
-    process_control_->Launch(
+    Task* process_launched_task =
         NewRunnableMethod(service_process_helper_.get(),
-                          &RemotingServiceProcessHelper::OnProcessLaunched));
+                          &RemotingServiceProcessHelper::OnProcessLaunched);
+    process_control_->Launch(process_launched_task, process_launched_task);
 #else
     ShowSetupDone();
 #endif
diff --git a/chrome/browser/service/service_process_control.cc b/chrome/browser/service/service_process_control.cc
index 08a1c9f..fcef987 100644
--- a/chrome/browser/service/service_process_control.cc
+++ b/chrome/browser/service/service_process_control.cc
@@ -7,6 +7,7 @@
 #include "base/command_line.h"
 #include "base/file_path.h"
 #include "base/process_util.h"
+#include "base/stl_util-inl.h"
 #include "base/thread.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/browser_thread.h"
@@ -87,23 +88,21 @@ ServiceProcessControl::ServiceProcessControl(Profile* profile)
 }
 
 ServiceProcessControl::~ServiceProcessControl() {
+  STLDeleteElements(&connect_done_tasks_);
+  STLDeleteElements(&connect_success_tasks_);
+  STLDeleteElements(&connect_failure_tasks_);
 }
 
-void ServiceProcessControl::ConnectInternal(Task* task) {
+void ServiceProcessControl::ConnectInternal() {
   // If the channel has already been established then we run the task
   // and return.
   if (channel_.get()) {
-    if (task) {
-      task->Run();
-      delete task;
-    }
+    RunConnectDoneTasks();
     return;
   }
 
   // Actually going to connect.
   LOG(INFO) << "Connecting to Service Process IPC Server";
-  connect_done_task_.reset(task);
-
   // Run the IPC channel on the shared IO thread.
   base::Thread* io_thread = g_browser_process->io_thread();
 
@@ -126,19 +125,54 @@ void ServiceProcessControl::ConnectInternal(Task* task) {
   }
 }
 
-void ServiceProcessControl::Launch(Task* task) {
+void ServiceProcessControl::RunConnectDoneTasks() {
+  RunAllTasksHelper(&connect_done_tasks_);
+  DCHECK(connect_done_tasks_.empty());
+  if (is_connected()) {
+    RunAllTasksHelper(&connect_success_tasks_);
+    DCHECK(connect_success_tasks_.empty());
+    STLDeleteElements(&connect_failure_tasks_);
+  } else {
+    RunAllTasksHelper(&connect_failure_tasks_);
+    DCHECK(connect_failure_tasks_.empty());
+    STLDeleteElements(&connect_success_tasks_);
+  }
+}
+
+// static
+void ServiceProcessControl::RunAllTasksHelper(TaskList* task_list) {
+  TaskList::iterator index = task_list->begin();
+  while (index != task_list->end()) {
+    (*index)->Run();
+    delete (*index);
+    index = task_list->erase(index);
+  }
+}
+
+void ServiceProcessControl::Launch(Task* success_task, Task* failure_task) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
+  if (success_task) {
+    if (success_task == failure_task) {
+      // If the tasks are the same, then the same task needs to be invoked
+      // for success and failure.
+      failure_task = NULL;
+      connect_done_tasks_.push_back(success_task);
+    } else {
+      connect_success_tasks_.push_back(success_task);
+    }
+  }
+
+  if (failure_task)
+    connect_failure_tasks_.push_back(failure_task);
+
   // If the service process is already running then connects to it.
   if (CheckServiceProcessReady()) {
-    ConnectInternal(task);
+    ConnectInternal();
     return;
   }
 
-  // Prevent quick calls to Launch in succession from causing a crash.
-  // TODO(scottbyer) - ServiceProcessControl launch task callback code needs to
-  // be enhanced to deal with this properly.  See
-  // http://code.google.com/p/chromium/issues/detail?id=58802
+  // If we already in the process of launching, then we are done.
   if (launcher_) {
     return;
   }
@@ -172,20 +206,19 @@ void ServiceProcessControl::Launch(Task* task) {
   // And then start the process asynchronously.
   launcher_ = new Launcher(this, cmd_line);
   launcher_->Run(
-      NewRunnableMethod(this, &ServiceProcessControl::OnProcessLaunched, task));
+      NewRunnableMethod(this, &ServiceProcessControl::OnProcessLaunched));
 }
 
-void ServiceProcessControl::OnProcessLaunched(Task* task) {
+void ServiceProcessControl::OnProcessLaunched() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   if (launcher_->launched()) {
     // After we have successfully created the service process we try to connect
     // to it. The launch task is transfered to a connect task.
-    ConnectInternal(task);
-  } else if (task) {
+    ConnectInternal();
+  } else {
     // If we don't have process handle that means launching the service process
     // has failed.
-    task->Run();
-    delete task;
+    RunConnectDoneTasks();
   }
 
   // We don't need the launcher anymore.
@@ -202,19 +235,13 @@ void ServiceProcessControl::OnMessageReceived(const IPC::Message& message) {
 
 void ServiceProcessControl::OnChannelConnected(int32 peer_pid) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  if (!connect_done_task_.get())
-    return;
-  connect_done_task_->Run();
-  connect_done_task_.reset();
+  RunConnectDoneTasks();
 }
 
 void ServiceProcessControl::OnChannelError() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   channel_.reset();
-  if (!connect_done_task_.get())
-    return;
-  connect_done_task_->Run();
-  connect_done_task_.reset();
+  RunConnectDoneTasks();
 }
 
 bool ServiceProcessControl::Send(IPC::Message* message) {
diff --git a/chrome/browser/service/service_process_control.h b/chrome/browser/service/service_process_control.h
index e6df376..ff22eb3 100644
--- a/chrome/browser/service/service_process_control.h
+++ b/chrome/browser/service/service_process_control.h
@@ -7,6 +7,7 @@
 
 #include <queue>
 #include <string>
+#include <vector>
 
 #include "base/id_map.h"
 #include "base/callback.h"
@@ -55,12 +56,19 @@ class ServiceProcessControl : public IPC::Channel::Sender,
   // Return true if this object is connected to the service.
   bool is_connected() const { return channel_.get() != NULL; }
 
-  // Create a new service process and connects to it.
-  // |launch_done_task| is called if launching the service process has failed
-  // or we have successfully launched the process and connected to it.
-  // If the service process is already running this method will try to connect
-  // to the service process.
-  void Launch(Task* launch_done_task);
+  // If no service process is currently running, creates a new service process
+  // and connects to it.
+  // If a service process is already running this method will try to connect
+  // to it.
+  // |success_task| is called when we have successfully launched the process
+  // and connected to it.
+  // |failure_task| is called when we failed to connect to the service process.
+  // It is OK to pass the same value for |success_task| and |failure_task|. In
+  // this case, the task is invoked on success or failure.
+  // Note that if we are already connected to service process then
+  // |success_task| can be invoked in the context of the Launch call.
+  // Takes ownership of |success_task| and |failure_task|.
+  void Launch(Task* success_task, Task* failure_task);
 
   // IPC::Channel::Listener implementation.
   virtual void OnMessageReceived(const IPC::Message& message);
@@ -109,13 +117,18 @@ class ServiceProcessControl : public IPC::Channel::Sender,
 
  private:
   class Launcher;
+  typedef std::vector<Task*> TaskList;
+
+  // Helper method to invoke all the callbacks based on success on failure.
+  void RunConnectDoneTasks();
 
   // Method called by Launcher when the service process is launched.
-  void OnProcessLaunched(Task* launch_done_task);
+  void OnProcessLaunched();
+
+  // Used internally to connect to the service process.
+  void ConnectInternal();
 
-  // Used internally to connect to the service process. |task| is executed
-  // when the connection is made or an error occurred.
-  void ConnectInternal(Task* task);
+  static void RunAllTasksHelper(TaskList* task_list);
 
   Profile* profile_;
 
@@ -125,9 +138,13 @@ class ServiceProcessControl : public IPC::Channel::Sender,
   // Service process launcher.
   scoped_refptr<Launcher> launcher_;
 
-  // Callback that gets invoked when the channel is connected or failed to
-  // connect.
-  scoped_ptr<Task> connect_done_task_;
+  // Callbacks that get invoked when the channel is successfully connected or
+  // if there was a failure in connecting.
+  TaskList connect_done_tasks_;
+  // Callbacks that get invoked ONLY when the channel is successfully connected.
+  TaskList connect_success_tasks_;
+  // Callbacks that get invoked ONLY when there was a connection failure.
+  TaskList connect_failure_tasks_;
 
   // Callback that gets invoked when a status message is received from
   // the cloud print proxy.
diff --git a/chrome/browser/service/service_process_control_browsertest.cc b/chrome/browser/service/service_process_control_browsertest.cc
index 9775e40..fe22dbc 100644
--- a/chrome/browser/service/service_process_control_browsertest.cc
+++ b/chrome/browser/service/service_process_control_browsertest.cc
@@ -22,6 +22,8 @@ class ServiceProcessControlBrowserTest
   ~ServiceProcessControlBrowserTest() {
     base::CloseProcessHandle(service_process_handle_);
     service_process_handle_ = base::kNullProcessHandle;
+    // Delete all instances of ServiceProcessControl.
+    ServiceProcessControlManager::instance()->Shutdown();
   }
 
  protected:
@@ -35,7 +37,10 @@ class ServiceProcessControlBrowserTest
     process->Launch(
         NewRunnableMethod(
             this,
-            &ServiceProcessControlBrowserTest::ProcessControlLaunched));
+            &ServiceProcessControlBrowserTest::ProcessControlLaunched),
+        NewRunnableMethod(
+            this,
+            &ServiceProcessControlBrowserTest::ProcessControlLaunchFailed));
 
     // Then run the message loop to keep things running.
     ui_test_utils::RunMessageLoop();
@@ -68,15 +73,21 @@ class ServiceProcessControlBrowserTest
         base::kProcessAccessWaitForTermination,
         &service_process_handle_));
     process()->SetMessageHandler(this);
+    // Quit the current message. Post a QuitTask instead of just calling Quit()
+    // because this can get invoked in the context of a Launch() call and we
+    // may not be in Run() yet.
+    MessageLoop::current()->PostTask(FROM_HERE, new MessageLoop::QuitTask());
+  }
+
+  void ProcessControlLaunchFailed() {
+    ADD_FAILURE();
     // Quit the current message.
-    MessageLoop::current()->PostTask(FROM_HERE,
-        new MessageLoop::QuitTask());
+    MessageLoop::current()->PostTask(FROM_HERE, new MessageLoop::QuitTask());
   }
 
   // ServiceProcessControl::MessageHandler implementations.
   virtual void OnGoodDay() {
-    MessageLoop::current()->PostTask(FROM_HERE,
-        new MessageLoop::QuitTask());
+    MessageLoop::current()->Quit();
   }
 
   ServiceProcessControl* process() { return process_; }
@@ -119,6 +130,50 @@ IN_PROC_BROWSER_TEST_F(ServiceProcessControlBrowserTest, LaunchTwice) {
   EXPECT_TRUE(process()->Shutdown());
 }
 
+static void DecrementUntilZero(int* count) {
+  (*count)--;
+  if (!(*count))
+    MessageLoop::current()->PostTask(FROM_HERE, new MessageLoop::QuitTask());
+}
+
+// Invoke multiple Launch calls in succession and ensure that all the tasks
+// get invoked.
+IN_PROC_BROWSER_TEST_F(ServiceProcessControlBrowserTest, MultipleLaunchTasks) {
+  ServiceProcessControl* process =
+      ServiceProcessControlManager::instance()->GetProcessControl(
+          browser()->profile());
+  int launch_count = 5;
+  for (int i = 0; i < launch_count; i++) {
+    // Launch the process asynchronously.
+    process->Launch(
+        NewRunnableFunction(&DecrementUntilZero, &launch_count),
+        new MessageLoop::QuitTask());
+  }
+  // Then run the message loop to keep things running.
+  ui_test_utils::RunMessageLoop();
+  EXPECT_EQ(0, launch_count);
+  // And then shutdown the service process.
+  EXPECT_TRUE(process->Shutdown());
+}
+
+// Make sure using the same task for success and failure tasks works.
+IN_PROC_BROWSER_TEST_F(ServiceProcessControlBrowserTest, SameLaunchTask) {
+  ServiceProcessControl* process =
+      ServiceProcessControlManager::instance()->GetProcessControl(
+          browser()->profile());
+  int launch_count = 5;
+  for (int i = 0; i < launch_count; i++) {
+    // Launch the process asynchronously.
+    Task * task = NewRunnableFunction(&DecrementUntilZero, &launch_count);
+    process->Launch(task, task);
+  }
+  // Then run the message loop to keep things running.
+  ui_test_utils::RunMessageLoop();
+  EXPECT_EQ(0, launch_count);
+  // And then shutdown the service process.
+  EXPECT_TRUE(process->Shutdown());
+}
+
 // Tests whether disconnecting from the service IPC causes the service process
 // to die.
 IN_PROC_BROWSER_TEST_F(ServiceProcessControlBrowserTest, DieOnDisconnect) {