Getting service process on Mac to handle having things moved/changed underneath it.

chrome/common/service_process_util_mac.mm

Index: chrome/common/service_process_util_mac.mm
diff --git a/chrome/common/service_process_util_mac.mm b/chrome/common/service_process_util_mac.mm
index 0f5746fa002c2e01d6d70600531c78f1d29a6cd1..3a9589746af64f79bb4d3e6332a1b2931285e7b2 100644
--- a/chrome/common/service_process_util_mac.mm
+++ b/chrome/common/service_process_util_mac.mm
@@ -6,14 +6,18 @@
 
 #import <Foundation/Foundation.h>
 #include <launch.h>
+#include <syslog.h>

[Mark Mentovai, 2011/03/11 20:13:52]

Why?

 
 #include "base/command_line.h"
 #include "base/file_path.h"
+#include "base/file_util.h"
 #include "base/mac/foundation_util.h"
 #include "base/mac/mac_util.h"
 #include "base/mac/scoped_nsautorelease_pool.h"
 #include "base/path_service.h"
+#include "base/process_util.h"
 #include "base/scoped_nsobject.h"
+#include "base/stringprintf.h"
 #include "base/string_util.h"
 #include "base/sys_string_conversions.h"
 #include "base/threading/thread_restrictions.h"
@@ -26,6 +30,8 @@
 
 namespace {
 
+#define kServiceProcessSessionType "Background"
+
 NSString* GetServiceProcessLaunchDFileName() {
   NSString *bundle_id = [base::mac::MainAppBundle() bundleIdentifier];
   NSString *label = [bundle_id stringByAppendingPathExtension:@"plist"];
@@ -59,7 +65,7 @@ NSString* GetServiceProcessLaunchDSocketEnvVar() {
 }
 
 // Creates the path that it returns. Must be called on the FILE thread.
-NSURL* GetUserAgentPath() {
+NSURL* GetUserAgentURL() {
   NSArray* library_paths = NSSearchPathForDirectoriesInDomains(
       NSLibraryDirectory, NSUserDomainMask, true);
   DCHECK_EQ([library_paths count], 1U);
@@ -72,7 +78,7 @@ NSURL* GetUserAgentPath() {
                                  withIntermediateDirectories:YES
                                                   attributes:nil
                                                        error:&err]) {
-    LOG(ERROR) << "GetUserAgentPath: " << err;
+    LOG(ERROR) << "GetUserAgentURL: " << err;
   }
 
   NSString* plist_file_path =
@@ -81,7 +87,24 @@ NSURL* GetUserAgentPath() {
   return [NSURL fileURLWithPath:plist_file_path isDirectory:NO];
 }
 
-}
+class ExecPathComponentWatcherDelegate
+    : public base::FilePathComponentWatcher::Delegate {
+ public:
+  ExecPathComponentWatcherDelegate(CFDictionaryRef launchd_conf,
+                                   ServiceProcessState *process_state)
+      : launchd_conf_(launchd_conf), process_state_(process_state) { }
+  virtual ~ExecPathComponentWatcherDelegate();
+  virtual void OnFilePathComponentsChanged(
+      base::FilePathComponentWatcher* watcher,
+      const FilePath& old_path,
+      const FilePath& new_path) OVERRIDE;
+
+private:
+  base::mac::ScopedCFTypeRef<CFDictionaryRef> launchd_conf_;
+  ServiceProcessState* process_state_;
+};
+
+}  // namespace
 
 // Gets the name of the service process IPC channel.
 IPC::ChannelHandle GetServiceProcessChannel() {
@@ -166,6 +189,22 @@ bool ServiceProcessState::Initialize() {
     CFRelease(err);
     return false;
   }
+
+
+  return true;
+}
+
+bool ServiceProcessState::SignalReadyPlatformSpecific() {

[sanjeevr, 2011/03/11 08:26:42]

It seems kind of cheating to use the SignalReady to do something that has
nothing to do with signalling ready. Perhaps call this Initialize() or something
like that?

+  CFStringRef exe_path = reinterpret_cast<CFStringRef>(CFDictionaryGetValue(
+      state_->launchd_conf_, CFSTR(LAUNCH_JOBKEY_PROGRAM)));
+  if (!exe_path) {
+    return false;
+  }
+  FilePath executable_path = FilePath(base::SysCFStringRefToUTF8(exe_path));

[Mark Mentovai, 2011/03/11 20:13:52]

This is wrong. Use CFStringGetFileSystemRepresentation or -[NSString
fileSystemRepresentation]. The NSString method is less cumbersome and looks like
it’d be easy to use here.

+  if (!state_->executable_watcher_.Watch(executable_path,
+      new ExecPathComponentWatcherDelegate(state_->launchd_conf_, this))) {
+    return false;
+  }
   return true;
 }
 
@@ -266,7 +305,7 @@ CFDictionaryRef CreateServiceProcessLaunchdPlist(CommandLine* cmd_line,
       [[NSDictionary alloc] initWithObjectsAndKeys:
         [NSNumber numberWithBool:YES], @ LAUNCH_JOBKEY_RUNATLOAD,
         keep_alive, @ LAUNCH_JOBKEY_KEEPALIVE,
-        @"Background", @ LAUNCH_JOBKEY_LIMITLOADTOSESSIONTYPE,
+        @ kServiceProcessSessionType, @ LAUNCH_JOBKEY_LIMITLOADTOSESSIONTYPE,
         nil];
     [launchd_plist addEntriesFromDictionary:auto_launchd_plist];
   }
@@ -285,7 +324,7 @@ bool ServiceProcessState::AddToAutoRun() {
   scoped_nsobject<NSDictionary> plist(
       base::mac::CFToNSCast(CreateServiceProcessLaunchdPlist(
                                 autorun_command_line_.get(), true)));
-  NSURL* plist_url = GetUserAgentPath();
+  NSURL* plist_url = GetUserAgentURL();
   return [plist writeToURL:plist_url atomically:YES];
 }
 
@@ -294,7 +333,7 @@ bool ServiceProcessState::RemoveFromAutoRun() {
   base::ThreadRestrictions::AssertIOAllowed();
 
   base::mac::ScopedNSAutoreleasePool pool;
-  NSURL* plist_url = GetUserAgentPath();
+  NSURL* plist_url = GetUserAgentURL();
   SInt32 error = 0;
   if (!CFURLDestroyResource(reinterpret_cast<CFURLRef>(plist_url), &error)) {
     LOG(ERROR) << "RemoveFromAutoRun: " << error;
@@ -302,3 +341,89 @@ bool ServiceProcessState::RemoveFromAutoRun() {
   }
   return true;
 }
+
+ExecPathComponentWatcherDelegate::~ExecPathComponentWatcherDelegate() {
+}
+
+void ExecPathComponentWatcherDelegate::OnFilePathComponentsChanged(
+    base::FilePathComponentWatcher* watcher, const FilePath& old_path,
+    const FilePath& new_path) {
+  base::mac::ScopedNSAutoreleasePool pool;
+  bool needs_shutdown = false;
+  bool needs_restart = false;
+  bool needs_plist_rewrite = false;
+  bool needs_plist_deletion = false;
+  if (access(new_path.value().c_str(), W_OK) != 0) {
+    // Can we execute it?

[Mark Mentovai, 2011/03/11 20:13:52]

Remember, no “we.” This is better:
    // Is it executable?

+    needs_shutdown = true;
+    needs_plist_deletion = true;
+  } else if (file_util::IsInTrash(new_path)) {
+    // Is it in the trash?
+    needs_shutdown = true;
+    needs_plist_deletion = true;
+  } else if (!file_util::AreReferringToSameObject(old_path, new_path)) {
+    // Did it get moved?
+    needs_restart = true;
+    needs_plist_rewrite = true;

[Mark Mentovai, 2011/03/11 20:13:52]

Judging from this logic, it seems that

  needs shutdown == needs_plist_deletion
and
  needs_restart == needs_plist_rewrite

True?

+  } else {
+    // Nothing exciting happened. Let's continue to ask for events.

[Mark Mentovai, 2011/03/11 20:13:52]

s/Let's c/C/

+    if (!watcher->Watch(new_path, this)) {

[Mark Mentovai, 2011/03/11 20:13:52]

OK, so it *is* possible to call Watch on an existing object, but only after it’s
called out?

?

Is it a better design (potentially losing fewer events) if the watcher is
allowed to keep watching until cancelled?

+      LOG(ERROR) << "Watch failed: " << new_path.value();
+    }
+  }
+  if (needs_plist_rewrite) {
+    NSURL* plist_url = GetUserAgentURL();
+    NSMutableDictionary* plist
+      = [NSMutableDictionary dictionaryWithContentsOfURL:plist_url];

[Mark Mentovai, 2011/03/11 20:13:52]

Put the = on the preceding line, and indent this one properly for a continuation
line.

+    if (plist) {
+      NSString* ns_new_path = base::SysUTF8ToNSString(new_path.value());

[Mark Mentovai, 2011/03/11 20:13:52]

Why not just +[NSString stringWithUTF8String:]?

+      [plist setObject:ns_new_path forKey:@ LAUNCH_JOBKEY_PROGRAM];
+      scoped_nsobject<NSMutableArray> args(
+          [[plist objectForKey:@ LAUNCH_JOBKEY_PROGRAMARGUMENTS] mutableCopy]);
+      [args replaceObjectAtIndex:0 withObject:ns_new_path];
+      [plist setObject:args forKey:@ LAUNCH_JOBKEY_PROGRAMARGUMENTS];
+      if (![plist writeToURL:plist_url atomically:YES]) {
+        LOG(ERROR) << "Unable to rewrite plist.";
+      }
+    } else {
+      LOG(ERROR) << "Unable to read plist.";
+    }
+  } else if (needs_plist_deletion) {
+    if (!process_state_->RemoveFromAutoRun()) {
+      LOG(ERROR) << "Unable to RemoveFromAutoRun.";
+    }
+  }
+  if (needs_shutdown || needs_restart) {
+    NSURL *plist_url = GetUserAgentURL();
+    std::string plist_path(base::SysNSStringToUTF8([plist_url path]));

[Mark Mentovai, 2011/03/11 20:13:52]

No, use [plist_url fileSystemRepresentation].

+    std::vector<std::string> argv;
+
+    if (needs_shutdown) {
+      VLOG(1) << "Shutting down using launchctl";
+      argv.push_back("/bin/launchctl");
+      argv.push_back("unload");
+      argv.push_back("-S");
+      argv.push_back(kServiceProcessSessionType);
+      argv.push_back(plist_path);
+    } else if (needs_restart) {

[Mark Mentovai, 2011/03/11 20:13:52]

This can just be |else|, look at the enclosing conditional.

+      VLOG(1) << "Restarting using launchctl";

[Mark Mentovai, 2011/03/11 20:13:52]

I assume that as soon as you run launchctl unload, this process will be killed?
Worth a comment.

I assume that’s also why you’re using a one-liner shell script to do this and
starting it in a new process group? Worth a comment.

+      argv.push_back("/bin/sh");

[Mark Mentovai, 2011/03/11 20:13:52]

/bin/bash if you’re using --noprofile.

+      argv.push_back("--noprofile");
+      argv.push_back("-c");
+      std::string command = base::StringPrintf(
+          "/bin/launchctl unload -S %s '%s';/bin/launchctl load -S %s '%s';",

[Mark Mentovai, 2011/03/11 20:13:52]

The only way I’ll let you pass arbitrary arguments to a shell script is if you
properly sanitize them. Using '%s' protects you against most nasties, but what
if plist_path winds up with a ' in it?

+           kServiceProcessSessionType, plist_path.c_str(),
+           kServiceProcessSessionType, plist_path.c_str());
+      argv.push_back(command);
+    }
+    base::ProcessHandle handle;
+    if (!base::LaunchAppInNewProcessGroup(argv,
+                                          base::environment_vector(),
+                                          base::file_handle_mapping_vector(),
+                                          NO,
+                                          &handle)) {
+      LOG(ERROR) << "LaunchAppInNewProcessGroup";
+    }

[Mark Mentovai, 2011/03/11 20:13:52]

What are you expecting to happen here? Be killed? What do you do? Return to the
run loop?

Do you have any cleanup that needs to be done?

Can you just exit?

Can you sleep forever expecting to be killed?

+  }
+}
+

[Mark Mentovai, 2011/03/11 20:13:52]

Blank line at EOF.