ServiceProcess clean up

I'm about to start into fixing a race condition that I introduced, but wanted to get this cleanup in as a separate CL.
Simplifies some interfaces.
Combines two separate task postings into one.
Adds some comments.

BUG=NONE
TEST=BUILD

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

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

6 files changed, 57 insertions, 56 deletions

diff --git a/chrome/common/service_process_util.cc b/chrome/common/service_process_util.cc
index a12a592..42f5da6 100644
--- a/chrome/common/service_process_util.cc
+++ b/chrome/common/service_process_util.cc
@@ -162,6 +162,7 @@ IPC::ChannelHandle GetServiceProcessChannel() {
 
 ServiceProcessState::ServiceProcessState() : state_(NULL) {
   CreateAutoRunCommandLine();
+  CreateState();
 }
 
 ServiceProcessState::~ServiceProcessState() {
@@ -180,9 +181,6 @@ void ServiceProcessState::SignalStopped() {
 
 #if !defined(OS_MACOSX)
 bool ServiceProcessState::Initialize() {
-  if (!CreateState()) {
-    return false;
-  }
   if (!TakeSingletonLock()) {
     return false;
   }
diff --git a/chrome/common/service_process_util.h b/chrome/common/service_process_util.h
index 3906601..5f2e1c1 100644
--- a/chrome/common/service_process_util.h
+++ b/chrome/common/service_process_util.h
@@ -102,7 +102,7 @@ class ServiceProcessState {
 #endif  // !OS_MACOSX
 
   // Creates the platform specific state.
-  bool CreateState();
+  void CreateState();
 
   // Tear down the platform specific state.
   void TearDownState();
diff --git a/chrome/common/service_process_util_mac.mm b/chrome/common/service_process_util_mac.mm
index abb0229..3ad15a6 100644
--- a/chrome/common/service_process_util_mac.mm
+++ b/chrome/common/service_process_util_mac.mm
@@ -71,16 +71,25 @@ bool GetParentFSRef(const FSRef& child, FSRef* parent) {
   return FSGetCatalogInfo(&child, 0, NULL, NULL, NULL, parent) == noErr;
 }
 
+bool RemoveFromLaunchd() {
+  // We're killing a file.
+  base::ThreadRestrictions::AssertIOAllowed();
+  base::mac::ScopedCFTypeRef<CFStringRef> name(CopyServiceProcessLaunchDName());
+  return Launchd::GetInstance()->DeletePlist(Launchd::User,
+                                             Launchd::Agent,
+                                             name);
+}
+
 class ExecFilePathWatcherDelegate : public FilePathWatcher::Delegate {
  public:
-  ExecFilePathWatcherDelegate() : process_state_(NULL) { }
-  bool Init(const FilePath& path, ServiceProcessState *process_state);
+  ExecFilePathWatcherDelegate() { }
   virtual ~ExecFilePathWatcherDelegate() { }
+
+  bool Init(const FilePath& path);
   virtual void OnFilePathChanged(const FilePath& path) OVERRIDE;
 
  private:
   FSRef executable_fsref_;
-  ServiceProcessState* process_state_;
 };
 
 }  // namespace
@@ -160,9 +169,6 @@ bool GetServiceProcessData(std::string* version, base::ProcessId* pid) {
 }
 
 bool ServiceProcessState::Initialize() {
-  if (!CreateState()) {
-    return false;
-  }
   CFErrorRef err = NULL;
   CFDictionaryRef dict =
       Launchd::GetInstance()->CopyDictionaryByCheckingIn(&err);
@@ -298,48 +304,38 @@ bool ServiceProcessState::AddToAutoRun() {
 }
 
 bool ServiceProcessState::RemoveFromAutoRun() {
-  // We're killing a file.
-  base::ThreadRestrictions::AssertIOAllowed();
-  base::mac::ScopedCFTypeRef<CFStringRef> name(CopyServiceProcessLaunchDName());
-  return Launchd::GetInstance()->DeletePlist(Launchd::User,
-                                             Launchd::Agent,
-                                             name);
+  return RemoveFromLaunchd();
 }
 
-void ServiceProcessState::StateData::WatchExecutable() {
+bool ServiceProcessState::StateData::WatchExecutable() {
   base::mac::ScopedNSAutoreleasePool pool;
   NSDictionary* ns_launchd_conf = base::mac::CFToNSCast(launchd_conf_);
   NSString* exe_path = [ns_launchd_conf objectForKey:@ LAUNCH_JOBKEY_PROGRAM];
   if (!exe_path) {
     LOG(ERROR) << "No " LAUNCH_JOBKEY_PROGRAM;
-    return;
+    return false;
   }
 
   FilePath executable_path = FilePath([exe_path fileSystemRepresentation]);
   scoped_ptr<ExecFilePathWatcherDelegate> delegate(
       new ExecFilePathWatcherDelegate);
-  if (!delegate->Init(executable_path, state_)) {
+  if (!delegate->Init(executable_path)) {
     LOG(ERROR) << "executable_watcher_.Init " << executable_path.value();
-    return;
+    return false;
   }
   if (!executable_watcher_.Watch(executable_path,
                                  delegate.release(),
                                  ui_message_loop_)) {
     LOG(ERROR) << "executable_watcher_.watch " << executable_path.value();
-    return;
-  }
-}
-
-bool ExecFilePathWatcherDelegate::Init(const FilePath& path,
-                                       ServiceProcessState *process_state) {
-  if (!process_state ||
-      !base::mac::FSRefFromPath(path.value(), &executable_fsref_)) {
     return false;
   }
-  process_state_ = process_state;
   return true;
 }
 
+bool ExecFilePathWatcherDelegate::Init(const FilePath& path) {
+  return base::mac::FSRefFromPath(path.value(), &executable_fsref_);
+}
+
 void ExecFilePathWatcherDelegate::OnFilePathChanged(const FilePath& path) {
   base::mac::ScopedNSAutoreleasePool pool;
   bool needs_shutdown = false;
@@ -418,8 +414,8 @@ void ExecFilePathWatcherDelegate::OnFilePathChanged(const FilePath& path) {
       }
     }
     if (needs_shutdown) {
-      if (!process_state_->RemoveFromAutoRun()) {
-        LOG(ERROR) << "Unable to RemoveFromAutoRun.";
+      if (!RemoveFromLaunchd()) {
+        LOG(ERROR) << "Unable to RemoveFromLaunchd.";
       }
     }
 
diff --git a/chrome/common/service_process_util_posix.cc b/chrome/common/service_process_util_posix.cc
index 771494e..e15d1df 100644
--- a/chrome/common/service_process_util_posix.cc
+++ b/chrome/common/service_process_util_posix.cc
@@ -5,6 +5,7 @@
 #include "chrome/common/service_process_util_posix.h"
 
 #include "base/basictypes.h"
+#include "base/eintr_wrapper.h"
 #include "base/message_loop_proxy.h"
 
 namespace {
@@ -77,18 +78,43 @@ void ServiceProcessState::StateData::SignalReady() {
 #if defined(OS_LINUX)
     initializing_lock_.reset();
 #endif  // OS_LINUX
+#if defined(OS_MACOSX)
+    if (!WatchExecutable()) {
+      LOG(ERROR) << "WatchExecutable";
+    }
+#endif  // OS_MACOSX
   } else {
     PLOG(ERROR) << "sigaction";
   }
 }
 
-ServiceProcessState::StateData::~StateData() {}
+ServiceProcessState::StateData::~StateData() {
+  if (sockets_[0] != -1) {
+    if (HANDLE_EINTR(close(sockets_[0]))) {
+      PLOG(ERROR) << "close";
+    }
+  }
+  if (sockets_[1] != -1) {
+    if (HANDLE_EINTR(close(sockets_[1]))) {
+      PLOG(ERROR) << "close";
+    }
+  }
+  if (set_action_) {
+    if (sigaction(SIGTERM, &old_action_, NULL) < 0) {
+      PLOG(ERROR) << "sigaction";
+    }
+  }
+}
 
-bool ServiceProcessState::CreateState() {
+void ServiceProcessState::CreateState() {
   CHECK(!state_);
   state_ = new StateData;
+
+  // Explicitly adding a reference here (and removing it in TearDownState)
+  // because StateData is refcounted on Mac and Linux so that methods can
+  // be called on other threads.
+  // It is not refcounted on Windows at this time.
   state_->AddRef();
-  return true;
 }
 
 bool ServiceProcessState::SignalReady(
@@ -109,12 +135,6 @@ bool ServiceProcessState::SignalReady(
     PLOG(ERROR) << "pipe";
     return false;
   }
-#if defined(OS_MACOSX)
-  state_->state_ = this;
-  message_loop_proxy->PostTask(FROM_HERE,
-      NewRunnableMethod(state_,
-                        &ServiceProcessState::StateData::WatchExecutable));
-#endif  // OS_MACOSX
   message_loop_proxy->PostTask(FROM_HERE,
       NewRunnableMethod(state_, &ServiceProcessState::StateData::SignalReady));
   return true;
@@ -123,17 +143,6 @@ bool ServiceProcessState::SignalReady(
 void ServiceProcessState::TearDownState() {
   g_signal_socket = -1;
   if (state_) {
-    if (state_->sockets_[0] != -1) {
-      close(state_->sockets_[0]);
-    }
-    if (state_->sockets_[1] != -1) {
-      close(state_->sockets_[1]);
-    }
-    if (state_->set_action_) {
-      if (sigaction(SIGTERM, &state_->old_action_, NULL) < 0) {
-        PLOG(ERROR) << "sigaction";
-      }
-    }
     state_->Release();
     state_ = NULL;
   }
diff --git a/chrome/common/service_process_util_posix.h b/chrome/common/service_process_util_posix.h
index 0eb29e8..d053de4 100644
--- a/chrome/common/service_process_util_posix.h
+++ b/chrome/common/service_process_util_posix.h
@@ -64,12 +64,11 @@ struct ServiceProcessState::StateData
   // variables to remove the trailing underscore.
 
 #if defined(OS_MACOSX)
-  void WatchExecutable();
+  bool WatchExecutable();
 
   base::mac::ScopedCFTypeRef<CFDictionaryRef> launchd_conf_;
   FilePathWatcher executable_watcher_;
   scoped_refptr<base::MessageLoopProxy> ui_message_loop_;
-  ServiceProcessState* state_;
 #endif  // OS_MACOSX
 #if defined(OS_LINUX)
   scoped_ptr<MultiProcessLock> initializing_lock_;
diff --git a/chrome/common/service_process_util_win.cc b/chrome/common/service_process_util_win.cc
index a6385e2..8f1ce62 100644
--- a/chrome/common/service_process_util_win.cc
+++ b/chrome/common/service_process_util_win.cc
@@ -103,10 +103,9 @@ struct ServiceProcessState::StateData {
   scoped_ptr<ServiceProcessShutdownMonitor> shutdown_monitor;
 };
 
-bool ServiceProcessState::CreateState() {
-  DCHECK(!state_);
+void ServiceProcessState::CreateState() {
+  CHECK(!state_);
   state_ = new StateData;
-  return true;
 }
 
 bool ServiceProcessState::TakeSingletonLock() {