Make the mac service process handling code clean itself up properly as far as launchd is concerned.

Note new documentation at https://sites.google.com/a/chromium.org/dev/developers/design-documents/service-processes

BUG=None
TEST= 1) Build.
      2) Launch Chromium with cloud print disabled.
      3) Start Terminal
      4) launchctl list
      5) You should not see any org.chromium.Chromium.framework.service_process/* processes listed
      6) Go to Preferences in chromium
      7) launchctl list
      8) You should now see a org.chromium.Chromium.framework.service_process/* process
      9) Quit chromium
      10) launchctl list
      11) There should not be any org.chromium.Chromium.framework.service_process/* processes anymore

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

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

chrome/common/service_process_util.h

@@ -67,12 +67,12 @@ class ServiceProcessState {
 
   // Signal that the service process is ready.
   // This method is called when the service process is running and initialized.
-  // |shutdown_task| is invoked when we get a shutdown request from another
+  // |terminate_task| is invoked when we get a terminate request from another
   // process (in the same thread that called SignalReady). It can be NULL.
   // |message_loop_proxy| must be of type IO and is the loop that POSIX uses
   // to monitor the service process.
   bool SignalReady(
-      base::MessageLoopProxy* message_loop_proxy, Task* shutdown_task);
+      base::MessageLoopProxy* message_loop_proxy, Task* terminate_task);
 
   // Signal that the service process is stopped.
   void SignalStopped();

chrome/common/service_process_util_mac.mm

@@ -120,7 +120,8 @@ bool ForceServiceProcessShutdown(const std::string& /* version */,
   CFErrorRef err = NULL;
   bool ret = Launchd::GetInstance()->RemoveJob(label, &err);
   if (!ret) {
-    LOG(ERROR) << "ForceServiceProcessShutdown: " << err;
+    LOG(ERROR) << "ForceServiceProcessShutdown: " << err << " "
+               << base::SysCFStringRefToUTF8(label);
     CFRelease(err);
   }
   return ret;
@@ -177,9 +178,9 @@ bool ServiceProcessState::Initialize() {
   CFErrorRef err = NULL;
   CFDictionaryRef dict =
       Launchd::GetInstance()->CopyDictionaryByCheckingIn(&err);
-
   if (!dict) {
-    LOG(ERROR) << "CopyLaunchdDictionaryByCheckingIn: " << err;
+    LOG(ERROR) << "ServiceProcess must be launched by launchd. "
+               << "CopyLaunchdDictionaryByCheckingIn: " << err;
     CFRelease(err);
     return false;
   }

chrome/common/service_process_util_posix.cc

@@ -13,21 +13,21 @@ namespace {
 int g_signal_socket = -1;
 }
 
-ServiceProcessShutdownMonitor::ServiceProcessShutdownMonitor(
-    Task* shutdown_task)
-    : shutdown_task_(shutdown_task) {
+ServiceProcessTerminateMonitor::ServiceProcessTerminateMonitor(
+    Task* terminate_task)
+    : terminate_task_(terminate_task) {
 }
 
-ServiceProcessShutdownMonitor::~ServiceProcessShutdownMonitor() {
+ServiceProcessTerminateMonitor::~ServiceProcessTerminateMonitor() {
 }
 
-void ServiceProcessShutdownMonitor::OnFileCanReadWithoutBlocking(int fd) {
-  if (shutdown_task_.get()) {
+void ServiceProcessTerminateMonitor::OnFileCanReadWithoutBlocking(int fd) {
+  if (terminate_task_.get()) {
     int buffer;
     int length = read(fd, &buffer, sizeof(buffer));
-    if ((length == sizeof(buffer)) && (buffer == kShutDownMessage)) {
-      shutdown_task_->Run();
-      shutdown_task_.reset();
+    if ((length == sizeof(buffer)) && (buffer == kTerminateMessage)) {
+      terminate_task_->Run();
+      terminate_task_.reset();
     } else if (length > 0) {
       LOG(ERROR) << "Unexpected read: " << buffer;
     } else if (length == 0) {
@@ -38,7 +38,7 @@ void ServiceProcessShutdownMonitor::OnFileCanReadWithoutBlocking(int fd) {
   }
 }
 
-void ServiceProcessShutdownMonitor::OnFileCanWriteWithoutBlocking(int fd) {
+void ServiceProcessTerminateMonitor::OnFileCanWriteWithoutBlocking(int fd) {
   NOTIMPLEMENTED();
 }
 
@@ -48,7 +48,7 @@ void ServiceProcessShutdownMonitor::OnFileCanWriteWithoutBlocking(int fd) {
 static void SigTermHandler(int sig, siginfo_t* info, void* uap) {
   // TODO(dmaclach): add security here to make sure that we are being shut
   //                 down by an appropriate process.
-  int message = ServiceProcessShutdownMonitor::kShutDownMessage;
+  int message = ServiceProcessTerminateMonitor::kTerminateMessage;
   if (write(g_signal_socket, &message, sizeof(message)) < 0) {
     PLOG(ERROR) << "write";
   }
@@ -65,7 +65,7 @@ void ServiceProcessState::StateData::SignalReady(base::WaitableEvent* signal,
   CHECK(!signal->IsSignaled());
    *success = MessageLoopForIO::current()->WatchFileDescriptor(
       sockets_[0], true, MessageLoopForIO::WATCH_READ,
-      &watcher_, shut_down_monitor_.get());
+      &watcher_, terminate_monitor_.get());
   if (!*success) {
     LOG(ERROR) << "WatchFileDescriptor";
     signal->Signal();
@@ -135,18 +135,18 @@ void ServiceProcessState::CreateState() {
 }
 
 bool ServiceProcessState::SignalReady(
-    base::MessageLoopProxy* message_loop_proxy, Task* shutdown_task) {
+    base::MessageLoopProxy* message_loop_proxy, Task* terminate_task) {
   CHECK(state_);
 
-  scoped_ptr<Task> scoped_shutdown_task(shutdown_task);
+  scoped_ptr<Task> scoped_terminate_task(terminate_task);
 #if defined(OS_POSIX) && !defined(OS_MACOSX)
   state_->running_lock_.reset(TakeServiceRunningLock(true));
   if (state_->running_lock_.get() == NULL) {
     return false;
   }
 #endif
-  state_->shut_down_monitor_.reset(
-      new ServiceProcessShutdownMonitor(scoped_shutdown_task.release()));
+  state_->terminate_monitor_.reset(
+      new ServiceProcessTerminateMonitor(scoped_terminate_task.release()));
   if (pipe(state_->sockets_) < 0) {
     PLOG(ERROR) << "pipe";
     return false;

chrome/common/service_process_util_posix.h

@@ -31,26 +31,26 @@ namespace base {
 class WaitableEvent;
 }
 
-// Watches for |kShutDownMessage| to be written to the file descriptor it is
-// watching. When it reads |kShutDownMessage|, it performs |shutdown_task_|.
+// Watches for |kTerminateMessage| to be written to the file descriptor it is
+// watching. When it reads |kTerminateMessage|, it performs |terminate_task_|.
 // Used here to monitor the socket listening to g_signal_socket.
-class ServiceProcessShutdownMonitor
+class ServiceProcessTerminateMonitor
     : public MessageLoopForIO::Watcher {
  public:
 
   enum {
-    kShutDownMessage = 0xdecea5e
+    kTerminateMessage = 0xdecea5e
   };
 
-  explicit ServiceProcessShutdownMonitor(Task* shutdown_task);
-  virtual ~ServiceProcessShutdownMonitor();
+  explicit ServiceProcessTerminateMonitor(Task* terminate_task);
+  virtual ~ServiceProcessTerminateMonitor();
 
   // MessageLoopForIO::Watcher overrides
   virtual void OnFileCanReadWithoutBlocking(int fd);
   virtual void OnFileCanWriteWithoutBlocking(int fd);
 
  private:
-  scoped_ptr<Task> shutdown_task_;
+  scoped_ptr<Task> terminate_task_;
 };
 
 struct ServiceProcessState::StateData
@@ -75,7 +75,7 @@ struct ServiceProcessState::StateData
   scoped_ptr<MultiProcessLock> initializing_lock_;
   scoped_ptr<MultiProcessLock> running_lock_;
 #endif
-  scoped_ptr<ServiceProcessShutdownMonitor> shut_down_monitor_;
+  scoped_ptr<ServiceProcessTerminateMonitor> terminate_monitor_;
   MessageLoopForIO::FileDescriptorWatcher watcher_;
   int sockets_[2];
   struct sigaction old_action_;

chrome/common/service_process_util_unittest.cc

@@ -5,12 +5,13 @@
 #include "chrome/common/service_process_util.h"
 
 #include "base/basictypes.h"
+#include "base/command_line.h"
+#include "base/file_path.h"
+#include "base/process_util.h"
 
 #if !defined(OS_MACOSX)
 #include "base/at_exit.h"
-#include "base/command_line.h"
 #include "base/memory/scoped_ptr.h"
-#include "base/process_util.h"
 #include "base/string_util.h"
 #include "base/test/multiprocess_test.h"
 #include "base/test/test_timeouts.h"
@@ -24,7 +25,7 @@
 #include "base/win/win_util.h"
 #endif
 
-#if defined(OS_POSIX) && !defined(OS_MACOSX)
+#if defined(OS_POSIX)
 #include "chrome/common/auto_start_linux.h"
 #include <glib.h>
 #endif
@@ -497,6 +498,26 @@ void TrashFunc(const FilePath& src) {
   EXPECT_EQ(status, noErr)  << "FSMoveObjectToTrashSync " << status;
 }
 
+TEST_F(ServiceProcessStateFileManipulationTest, VerifyLaunchD) {
+  // There have been problems where launchd has gotten into a bad state, usually
+  // because something had deleted all the files in /tmp. launchd depends on
+  // a Unix Domain Socket that it creates at /tmp/launchd*/sock.
+  // The symptom of this problem is that the service process connect fails
+  // on Mac and "launch_msg(): Socket is not connected" appears.
+  // This test is designed to make sure that launchd is working.
+  // http://crbug/75518
+
+  CommandLine cl(FilePath("/bin/launchctl"));
+  cl.AppendArg("list");
+  cl.AppendArg("com.apple.launchctl.Aqua");
+
+  std::string output;
+  int exit_code = -1;
+  ASSERT_TRUE(base::GetAppOutputWithExitCode(cl, &output, &exit_code)
+              && exit_code == 0)
+      << " exit_code:" << exit_code << " " << output;
+}
+
 TEST_F(ServiceProcessStateFileManipulationTest, DeleteFile) {
   GetIOMessageLoopProxy()->PostTask(
       FROM_HERE,

chrome/common/service_process_util_win.cc

@@ -20,14 +20,16 @@
 
 namespace {
 
+const char* kTerminateEventSuffix = "_service_terminate_evt";
+
 string16 GetServiceProcessReadyEventName() {
   return UTF8ToWide(
       GetServiceProcessScopedVersionedName("_service_ready"));
 }
 
-string16 GetServiceProcessShutdownEventName() {
+string16 GetServiceProcessTerminateEventName() {
   return UTF8ToWide(
-      GetServiceProcessScopedVersionedName("_service_shutdown_evt"));
+      GetServiceProcessScopedVersionedName(kTerminateEventSuffix));
 }
 
 std::string GetServiceProcessAutoRunKey() {
@@ -46,29 +48,29 @@ std::string GetObsoleteServiceProcessAutoRunKey() {
   return scoped_name;
 }
 
-class ServiceProcessShutdownMonitor
+class ServiceProcessTerminateMonitor
     : public base::win::ObjectWatcher::Delegate {
  public:
-  explicit ServiceProcessShutdownMonitor(Task* shutdown_task)
-      : shutdown_task_(shutdown_task) {
+  explicit ServiceProcessTerminateMonitor(Task* terminate_task)
+      : terminate_task_(terminate_task) {
   }
   void Start() {
-    string16 event_name = GetServiceProcessShutdownEventName();
+    string16 event_name = GetServiceProcessTerminateEventName();
     CHECK(event_name.length() <= MAX_PATH);
-    shutdown_event_.Set(CreateEvent(NULL, TRUE, FALSE, event_name.c_str()));
-    watcher_.StartWatching(shutdown_event_.Get(), this);
+    terminate_event_.Set(CreateEvent(NULL, TRUE, FALSE, event_name.c_str()));
+    watcher_.StartWatching(terminate_event_.Get(), this);
   }
 
   // base::ObjectWatcher::Delegate implementation.
   virtual void OnObjectSignaled(HANDLE object) {
-    shutdown_task_->Run();
-    shutdown_task_.reset();
+    terminate_task_->Run();
+    terminate_task_.reset();
   }
 
  private:
-  base::win::ScopedHandle shutdown_event_;
+  base::win::ScopedHandle terminate_event_;
   base::win::ObjectWatcher watcher_;
-  scoped_ptr<Task> shutdown_task_;
+  scoped_ptr<Task> terminate_task_;
 };
 
 }  // namespace
@@ -80,15 +82,15 @@ IPC::ChannelHandle GetServiceProcessChannel() {
 
 bool ForceServiceProcessShutdown(const std::string& version,
                                  base::ProcessId process_id) {
-  base::win::ScopedHandle shutdown_event;
+  base::win::ScopedHandle terminate_event;
   std::string versioned_name = version;
-  versioned_name.append("_service_shutdown_evt");
+  versioned_name.append(kTerminateEventSuffix);
   string16 event_name =
       UTF8ToWide(GetServiceProcessScopedName(versioned_name));
-  shutdown_event.Set(OpenEvent(EVENT_MODIFY_STATE, FALSE, event_name.c_str()));
-  if (!shutdown_event.IsValid())
+  terminate_event.Set(OpenEvent(EVENT_MODIFY_STATE, FALSE, event_name.c_str()));
+  if (!terminate_event.IsValid())
     return false;
-  SetEvent(shutdown_event.Get());
+  SetEvent(terminate_event.Get());
   return true;
 }
 
@@ -105,7 +107,7 @@ bool CheckServiceProcessReady() {
 struct ServiceProcessState::StateData {
   // An event that is signaled when a service process is ready.
   base::win::ScopedHandle ready_event;
-  scoped_ptr<ServiceProcessShutdownMonitor> shutdown_monitor;
+  scoped_ptr<ServiceProcessTerminateMonitor> terminate_monitor;
 };
 
 void ServiceProcessState::CreateState() {
@@ -129,17 +131,17 @@ bool ServiceProcessState::TakeSingletonLock() {
 }
 
 bool ServiceProcessState::SignalReady(
-    base::MessageLoopProxy* message_loop_proxy, Task* shutdown_task) {
+    base::MessageLoopProxy* message_loop_proxy, Task* terminate_task) {
   DCHECK(state_);
   DCHECK(state_->ready_event.IsValid());
-  scoped_ptr<Task> scoped_shutdown_task(shutdown_task);
+  scoped_ptr<Task> scoped_terminate_task(terminate_task);
   if (!SetEvent(state_->ready_event.Get())) {
     return false;
   }
-  if (shutdown_task) {
-    state_->shutdown_monitor.reset(
-        new ServiceProcessShutdownMonitor(scoped_shutdown_task.release()));
-    state_->shutdown_monitor->Start();
+  if (terminate_task) {
+    state_->terminate_monitor.reset(
+        new ServiceProcessTerminateMonitor(scoped_terminate_task.release()));
+    state_->terminate_monitor->Start();
   }
   return true;
 }

chrome/service/service_process.cc

@@ -210,8 +210,9 @@ bool ServiceProcess::Initialize(MessageLoopForUI* message_loop,
 
   // After the IPC server has started we signal that the service process is
   // ready.
-  if (!state->SignalReady(io_thread_->message_loop_proxy(),
-                          NewRunnableMethod(this, &ServiceProcess::Shutdown))) {
+  if (!service_process_state_->SignalReady(
+      io_thread_->message_loop_proxy(),
+      NewRunnableMethod(this, &ServiceProcess::Terminate))) {
     return false;
   }
 
@@ -241,7 +242,23 @@ bool ServiceProcess::Teardown() {
 // This method is called when a shutdown command is received from IPC channel
 // or there was an error in the IPC channel.
 void ServiceProcess::Shutdown() {
-  // Quit the main message loop.
+#if defined(OS_MACOSX)
+  // On MacOS X the service must be removed from the launchd job list.
+  // http://www.chromium.org/developers/design-documents/service-processes
+  // The best way to do that is to go through the ForceServiceProcessShutdown
+  // path. If it succeeds Terminate() will be called from the handler registered
+  // via service_process_state_->SignalReady().
+  // On failure call Terminate() directly to force the process to actually
+  // terminate.
+  if (!ForceServiceProcessShutdown("", 0)) {
+    Terminate();
+  }
+#else
+  Terminate();
+#endif
+}
+
+void ServiceProcess::Terminate() {
   main_message_loop_->PostTask(FROM_HERE, new MessageLoop::QuitTask());
 }
 

chrome/service/service_process.h

@@ -29,6 +29,8 @@ class CommandLine;
 
 // The ServiceProcess does not inherit from ChildProcess because this
 // process can live independently of the browser process.
+// ServiceProcess Design Notes
+// https://sites.google.com/a/chromium.org/dev/developers/design-documents/service-processes
 class ServiceProcess : public CloudPrintProxy::Client {
  public:
   ServiceProcess();
@@ -99,16 +101,22 @@ class ServiceProcess : public CloudPrintProxy::Client {
  private:
   // Schedule a call to ShutdownIfNeeded.
   void ScheduleShutdownCheck();
+
   // Shuts down the process if no services are enabled and no clients are
   // connected.
   void ShutdownIfNeeded();
+
   // Called exactly ONCE per process instance for each service that gets
   // enabled in this process.
   void OnServiceEnabled();
+
   // Called exactly ONCE per process instance for each service that gets
   // disabled in this process (note that shutdown != disabled).
   void OnServiceDisabled();
 
+  // Terminate forces the service process to quit.
+  void Terminate();
+
   scoped_ptr<net::NetworkChangeNotifier> network_change_notifier_;
   scoped_ptr<base::Thread> io_thread_;
   scoped_ptr<base::Thread> file_thread_;