core: spruce up TakeNamedLock and friends

TakeNamedLock has a "waiting" mode that works like this:

  for (int i = 0; i < 10; i++)
    if (TryLock())
      break;
    usleep(i * 100);

which means that in "waiting" mode, if the lock is unavailable
for the duration, this function will block the calling thread
(with the message loop not spinning) for up to 4.5 seconds. However,
there are two problems with this design:

* The caller has no way to know whether "waiting" is appropriate or not
* The caller has to handle failure to acquire the lock either way

If retries are needed to deal with external conditions changing that might
have freed the lock up (another process exiting, e.g.) those retries should
be done at higher levels with more context.

More concretely, the only actual production use of this function is in
CloudPrintServiceProcessMain:

    std::unique_ptr<ServiceProcessState> state(new ServiceProcessState);
    if (!state->Initialize())
      return 0;

This change has TakeNamedLock make a single non-blocking attempt to
take the lock, and moves the retry logic up into
CloudPrintServiceProcessMain.

Since I was here anyway, I converted the API to use unique_ptr, since it
returns ownership of the lock if it succeeds.

Bug: 1052430
Change-Id: Ib87b23cc821fe59e69b76ffb3875917027bfdc4e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2057530Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#744283}

chrome/common/mac/mock_launchd.mm

@@ -108,8 +108,7 @@ MockLaunchd::~MockLaunchd() {}
 bool MockLaunchd::GetJobInfo(const std::string& label,
                              mac::services::JobInfo* info) {
   if (!as_service_) {
-    std::unique_ptr<MultiProcessLock> running_lock(
-        TakeNamedLock(pipe_name_, false));
+    std::unique_ptr<MultiProcessLock> running_lock = TakeNamedLock(pipe_name_);
     if (running_lock.get())
       return false;
   }
@@ -164,5 +163,5 @@ bool MockLaunchd::DeletePlist(Domain domain, Type type, CFStringRef name) {
 
 void MockLaunchd::SignalReady() {
   ASSERT_TRUE(as_service_);
-  running_lock_.reset(TakeNamedLock(pipe_name_, true));
+  running_lock_ = TakeNamedLock(pipe_name_);
 }

chrome/common/service_process_util.h

@@ -21,14 +21,6 @@
 
 class MultiProcessLock;
 
-#if defined(OS_MACOSX)
-#ifdef __OBJC__
-@class NSString;
-#else
-class NSString;
-#endif
-#endif
-
 namespace base {
 class CommandLine;
 }
@@ -47,10 +39,9 @@ std::string GetServiceProcessScopedVersionedName(const std::string& append_str);
 #endif  // !OS_MACOSX
 
 #if defined(OS_POSIX)
-// Attempts to take a lock named |name|. If |waiting| is true then this will
-// make multiple attempts to acquire the lock.
-// Caller is responsible for ownership of the MultiProcessLock.
-MultiProcessLock* TakeNamedLock(const std::string& name, bool waiting);
+// Attempts to take a session-wide lock named name. Returns a non-null lock if
+// successful.
+std::unique_ptr<MultiProcessLock> TakeNamedLock(const std::string& name);
 #endif
 
 // The following method is used in a process that acts as a client to the

chrome/common/service_process_util_linux.cc

@@ -20,12 +20,6 @@
 
 namespace {
 
-MultiProcessLock* TakeServiceInitializingLock(bool waiting) {
-  std::string lock_name =
-      GetServiceProcessScopedName("_service_initializing");
-  return TakeNamedLock(lock_name, waiting);
-}
-
 std::string GetBaseDesktopName() {
 #if BUILDFLAG(GOOGLE_CHROME_BRANDING)
   return "google-chrome-service.desktop";
@@ -35,10 +29,10 @@ std::string GetBaseDesktopName() {
 }
 }  // namespace
 
-MultiProcessLock* TakeServiceRunningLock(bool waiting) {
+std::unique_ptr<MultiProcessLock> TakeServiceRunningLock() {
   std::string lock_name =
       GetServiceProcessScopedName("_service_running");
-  return TakeNamedLock(lock_name, waiting);
+  return TakeNamedLock(lock_name);
 }
 
 bool ForceServiceProcessShutdown(const std::string& version,
@@ -60,12 +54,13 @@ mojo::NamedPlatformChannel::ServerName GetServiceProcessServerName() {
 }
 
 bool CheckServiceProcessReady() {
-  std::unique_ptr<MultiProcessLock> running_lock(TakeServiceRunningLock(false));
-  return running_lock.get() == NULL;
+  std::unique_ptr<MultiProcessLock> running_lock(TakeServiceRunningLock());
+  return !running_lock.get();
 }
 
 bool ServiceProcessState::TakeSingletonLock() {
-  state_->initializing_lock.reset(TakeServiceInitializingLock(true));
+  state_->initializing_lock =
+      TakeNamedLock(GetServiceProcessScopedName("_service_initializing"));
   return state_->initializing_lock.get();
 }
 

chrome/common/service_process_util_posix.cc

@@ -172,25 +172,13 @@ bool ServiceProcessState::DeleteServiceProcessDataRegion() {
 
 #endif  // !defined(OS_MACOSX)
 
-// Attempts to take a lock named |name|. If |waiting| is true then this will
-// make multiple attempts to acquire the lock.
-// Caller is responsible for ownership of the MultiProcessLock.
-MultiProcessLock* TakeNamedLock(const std::string& name, bool waiting) {
+// Attempts to take a lock named |name|. Returns the lock if successful, or
+// nullptr if not.
+std::unique_ptr<MultiProcessLock> TakeNamedLock(const std::string& name) {
   std::unique_ptr<MultiProcessLock> lock = MultiProcessLock::Create(name);
-  if (lock == NULL) return NULL;
-  bool got_lock = false;
-  for (int i = 0; i < 10; ++i) {
-    if (lock->TryLock()) {
-      got_lock = true;
-      break;
-    }
-    if (!waiting) break;
-    base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(100 * i));
-  }
-  if (!got_lock) {
+  if (!lock->TryLock())
     lock.reset();
-  }
-  return lock.release();
+  return lock;
 }
 
 ServiceProcessTerminateMonitor::ServiceProcessTerminateMonitor(
@@ -324,8 +312,8 @@ bool ServiceProcessState::SignalReady(
   DCHECK(state_);
 
 #if !defined(OS_MACOSX)
-  state_->running_lock.reset(TakeServiceRunningLock(true));
-  if (state_->running_lock.get() == NULL) {
+  state_->running_lock = TakeServiceRunningLock();
+  if (!state_->running_lock.get()) {
     return false;
   }
 #endif

chrome/common/service_process_util_posix.h

@@ -19,7 +19,7 @@
 
 #if defined(OS_POSIX) && !defined(OS_MACOSX)
 #include "chrome/common/multi_process_lock.h"
-MultiProcessLock* TakeServiceRunningLock(bool waiting);
+std::unique_ptr<MultiProcessLock> TakeServiceRunningLock();
 #endif
 
 #if defined(OS_MACOSX)

chrome/service/service_main.cc

@@ -38,9 +38,21 @@ int CloudPrintServiceProcessMain(
   VLOG(1) << "Service process launched: "
           << parameters.command_line.GetCommandLineString();
 
-  // If there is already a service process running, quit now.
-  std::unique_ptr<ServiceProcessState> state(new ServiceProcessState);
-  if (!state->Initialize())
+  auto initialize_service = [](ServiceProcessState* service) {
+    for (int i = 0; i < 10; ++i) {
+      if (service->Initialize())
+        return true;
+      base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(i * 100));
+    }
+    return false;
+  };
+
+  // If there is already a service process running, quit now. Retry a few times
+  // in case the running service is busy exiting.
+  // TODO(ellyjones): Are these retries actually necessary / can this case
+  // happen in practice?
+  auto state = std::make_unique<ServiceProcessState>();
+  if (!initialize_service(state.get()))
     return 0;
 
   base::RunLoop run_loop;