Cloud Print Policy - service side enforcement, Mac browser tests.

chrome/common/mac/mock_launchd.cc

Index: chrome/common/mac/mock_launchd.cc
diff --git a/chrome/common/mac/mock_launchd.cc b/chrome/common/mac/mock_launchd.cc
index f9310df6a06984253413d445c252d2338173a4c4..4b31ff597c11fb6705891694763bc9ed0453a658 100644
--- a/chrome/common/mac/mock_launchd.cc
+++ b/chrome/common/mac/mock_launchd.cc
@@ -4,15 +4,26 @@
 
 #include "chrome/common/mac/mock_launchd.h"
 
+#include <CoreFoundation/CoreFoundation.h>
+

[Mark Mentovai, 2012/01/20 20:41:35]

No blank line here.

[Scott Byer, 2012/01/20 23:13:37]

Done.

+#include <sys/socket.h>
+#include <sys/un.h>
+
 #include "base/file_path.h"
 #include "base/file_util.h"
+#include "base/mac/foundation_util.h"
 #include "base/mac/scoped_cftyperef.h"
 #include "base/message_loop.h"
+#include "base/process_util.h"
 #include "base/stringprintf.h"
 #include "base/sys_string_conversions.h"
+#include "chrome/common/chrome_version_info.h"
 #include "chrome/common/mac/launchd.h"
+#include "chrome/common/service_process_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+static const size_t kMaxPipeNameLength = 104;

[Mark Mentovai, 2012/01/20 20:41:35]

Rather than hard-coding 104 again here, can you use sizeof, either here or
inline where you need it?

You could do

static sockaddr_un* throwaway_sockaddr_un;
static const size_t kMaxPipeNameLength =
sizeof(throwaway_sockaddr_un->sun_path);

and the compiler should throw away the unused struct, the only question would be
whether it would warn about it being unused or if it would stay quiet because
you’re using it, just not using its storage.

(This is a language wart, and the solutions are all inelegant, but having a
second 104 hard-code here is also inelegant.)

[Scott Byer, 2012/01/20 23:13:37]

Done.

+
 // static
 bool MockLaunchd::MakeABundle(const FilePath& dst,
                               const std::string& name,
@@ -36,6 +47,8 @@ bool MockLaunchd::MakeABundle(const FilePath& dst,
     return false;
   }
 
+  chrome::VersionInfo version_info;
+
   const char* info_plist_format =
       "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n"
       "<!DOCTYPE plist PUBLIC \"-//Apple//DTD PLIST 1.0//EN\" "
@@ -52,11 +65,15 @@ bool MockLaunchd::MakeABundle(const FilePath& dst,
       "  <string>%s</string>\n"
       "  <key>CFBundleVersion</key>\n"
       "  <string>1</string>\n"
+      "  <key>CFBundleShortVersionString</key>\n"

[Mark Mentovai, 2012/01/20 20:41:35]

The keys in this dict were already sorted, so you should keep ’em that way.

[Scott Byer, 2012/01/20 23:13:37]

They weren't completely, all fixed up.
Done.

+      "  <string>%s</string>\n"
       "</dict>\n"
       "</plist>\n";
-  std::string info_plist_data = base::StringPrintf(info_plist_format,
-                                                   name.c_str(),
-                                                   name.c_str());
+  std::string info_plist_data =
+      base::StringPrintf(info_plist_format,
+                         name.c_str(),
+                         name.c_str(),
+                         version_info.Version().c_str());
   len = info_plist_data.length();
   if (file_util::WriteFile(info_plist, info_plist_data.c_str(), len) != len) {
     return false;
@@ -73,21 +90,79 @@ bool MockLaunchd::MakeABundle(const FilePath& dst,
   return bundle.get();
 }
 
+MockLaunchd::MockLaunchd(const FilePath& file, MessageLoop* loop,
+                         bool create_socket, bool as_service)
+    : file_(file),
+      message_loop_(loop),
+      create_socket_(create_socket),
+      as_service_(as_service),
+      restart_called_(false),
+      remove_called_(false),
+      job_called_(false),
+      checkin_called_(false),
+      write_called_(false),
+      delete_called_(false) {
+  std::string pipe_suffix("_SOCKET");
+  FilePath socket_path = file_;
+  while (socket_path.value().length() + pipe_suffix.length() >

[Mark Mentovai, 2012/01/20 20:41:35]

Shouldn’t this be >= or -2? You’ll need to add a slash to socket_path and also
leave room for the terminating '\0'.

[Scott Byer, 2012/01/20 23:13:37]

Done.

+         kMaxPipeNameLength - 1)

[Mark Mentovai, 2012/01/20 20:41:35]

Can we use {braces} here for readability because the condition is already taking
up more than one line?

[Scott Byer, 2012/01/20 23:13:37]

Done.

+    socket_path = socket_path.DirName();

[Mark Mentovai, 2012/01/20 20:41:35]

I guess this approach is OK for test code. The pathname length restriction in
this API is ridiculous, but it’s what we’ve got to deal with.

+  pipe_name_ = socket_path.value() + pipe_suffix;
+}
+
+MockLaunchd::~MockLaunchd() {
+}
+
 CFDictionaryRef MockLaunchd::CopyExports() {
-  ADD_FAILURE();
-  return NULL;
+  if (!create_socket_) {
+    ADD_FAILURE();
+    return NULL;
+  }
+
+  CFStringRef env_var =
+      base::mac::NSToCFCast(GetServiceProcessLaunchDSocketEnvVar());
+  CFStringRef socket_path = CFStringCreateWithCString(kCFAllocatorDefault,

[Mark Mentovai, 2012/01/20 20:41:35]

Leaked? Use a ScopedCFTypeRef.

[Scott Byer, 2012/01/20 23:13:37]

Done.

+                                                      pipe_name_.c_str(),
+                                                      kCFStringEncodingUTF8);
+  const void *keys[] = { env_var };
+  const void *values[] = { socket_path };

[Mark Mentovai, 2012/01/20 20:41:35]

You can DCHECK (probably statically, even, a la COMPILE_ASSERT) that the two
arraysizes match.

[Scott Byer, 2012/01/20 23:13:37]

Done.

+  return CFDictionaryCreate(kCFAllocatorDefault,
+                            keys,
+                            values,
+                            arraysize(keys),
+                            &kCFTypeDictionaryKeyCallBacks,
+                            &kCFTypeDictionaryValueCallBacks);
 }
 
 CFDictionaryRef MockLaunchd::CopyJobDictionary(CFStringRef label) {
-  ADD_FAILURE();
-  return NULL;
+  if (!as_service_) {
+    scoped_ptr<MultiProcessLock> running_lock(
+        TakeNamedLock(pipe_name_, false));
+    if (running_lock.get())
+      return NULL;
+  }
+
+  CFStringRef program = CFSTR(LAUNCH_JOBKEY_PROGRAM);
+  CFStringRef program_pid = CFSTR(LAUNCH_JOBKEY_PID);
+  const void *keys[] = { program, program_pid };
+  base::mac::ScopedCFTypeRef<CFStringRef> path(
+      base::SysUTF8ToCFStringRef(file_.value()));
+  int process_id = base::GetCurrentProcId();
+  base::mac::ScopedCFTypeRef<CFNumberRef> pid(
+      CFNumberCreate(NULL, kCFNumberIntType, &process_id));
+  const void *values[] = { path, pid };

[Mark Mentovai, 2012/01/20 20:41:35]

Here too? 177 also?

[Scott Byer, 2012/01/20 23:13:37]

Done.

+  return CFDictionaryCreate(kCFAllocatorDefault,
+                            keys,
+                            values,
+                            arraysize(keys),
+                            &kCFTypeDictionaryKeyCallBacks,
+                            &kCFTypeDictionaryValueCallBacks);
 }
 
 CFDictionaryRef MockLaunchd::CopyDictionaryByCheckingIn(CFErrorRef* error) {
   checkin_called_ = true;
   CFStringRef program = CFSTR(LAUNCH_JOBKEY_PROGRAM);
   CFStringRef program_args = CFSTR(LAUNCH_JOBKEY_PROGRAMARGUMENTS);
-  const void *keys[] = { program, program_args };
   base::mac::ScopedCFTypeRef<CFStringRef> path(
       base::SysUTF8ToCFStringRef(file_.value()));
   const void *array_values[] = { path.get() };
@@ -96,7 +171,71 @@ CFDictionaryRef MockLaunchd::CopyDictionaryByCheckingIn(CFErrorRef* error) {
                     array_values,
                     1,
                     &kCFTypeArrayCallBacks));
-  const void *values[] = { path, args };
+
+  if (!create_socket_) {
+    const void *keys[] = { program, program_args };
+    const void *values[] = { path, args };
+    return CFDictionaryCreate(kCFAllocatorDefault,
+                              keys,
+                              values,
+                              arraysize(keys),
+                              &kCFTypeDictionaryKeyCallBacks,
+                              &kCFTypeDictionaryValueCallBacks);
+  }
+
+  CFStringRef socket_key = CFSTR(LAUNCH_JOBKEY_SOCKETS);
+  int local_pipe = -1;
+  EXPECT_TRUE(as_service_);
+
+  // Create unix_addr structure.
+  struct sockaddr_un unix_addr;
+  memset(&unix_addr, 0, sizeof(unix_addr));

[Mark Mentovai, 2012/01/20 20:41:35]

Or just
  struct sockaddr_un unix_addr = {0};

[Scott Byer, 2012/01/20 23:13:37]

Done.

+  unix_addr.sun_family = AF_UNIX;
+  int path_len = snprintf(unix_addr.sun_path, kMaxPipeNameLength,
+                          "%s", pipe_name_.c_str());

[Mark Mentovai, 2012/01/20 20:41:35]

Don’t use snprintf to null-terminate. You can use strlcpy, either the <string.h>
version, or probably better, the base::strlcpy version from string_util.

[Scott Byer, 2012/01/20 23:13:37]

Done.

+  DCHECK_EQ(static_cast<int>(pipe_name_.length()), path_len);
+  size_t unix_addr_len = offsetof(struct sockaddr_un,

[Mark Mentovai, 2012/01/20 20:41:35]

I would prefer to hold off on computing this until you’ve finished populating
unix_addr. That puts the steps in order so the code will read more easily.

[Scott Byer, 2012/01/20 23:13:37]

Moved down to just before use. It's just complicated enough that I think keeping
it as a separate variable assignment is more clear than inlining it into the
function call.
Done.

+                                  sun_path) + path_len + 1;
+  unix_addr.sun_len = SUN_LEN(&unix_addr);
+
+  CFSocketSignature signature;
+  signature.protocolFamily = PF_UNIX;
+  signature.socketType = SOCK_STREAM;
+  signature.protocol = 0;
+  signature.address = CFDataCreate(NULL, reinterpret_cast<UInt8*>(&unix_addr),
+                                   unix_addr_len);
+  CFSocketRef socket =
+      CFSocketCreateWithSocketSignature(NULL, &signature, NULL, NULL, NULL);

[Mark Mentovai, 2012/01/20 20:41:35]

Does this consume the reference to signature.address or do you need to release
it yourself?

[Scott Byer, 2012/01/20 23:13:37]

It's not spelled out explicitly in the docs, but based on the API style, I'd say
it's not, so I've added the CFRelease. (valgrind still isn't working on Lion, so
I'm waiting on the try bot run).
Done.

+
+  local_pipe = CFSocketGetNative(socket);
+  EXPECT_NE(-1, local_pipe);
+  if (local_pipe == -1) {
+    if (error)
+      *error = CFErrorCreate(kCFAllocatorDefault, kCFErrorDomainPOSIX,

[Mark Mentovai, 2012/01/20 20:41:35]

{brace me}

[Scott Byer, 2012/01/20 23:13:37]

Done.

+                             errno, NULL);
+    return NULL;
+  }
+
+  base::mac::ScopedCFTypeRef<CFNumberRef> socket_fd(
+      CFNumberCreate(NULL, kCFNumberIntType, &local_pipe));
+  const void *socket_array_values[] = { socket_fd };
+  base::mac::ScopedCFTypeRef<CFArrayRef> sockets(
+      CFArrayCreate(kCFAllocatorDefault,
+                    socket_array_values,
+                    1,
+                    &kCFTypeArrayCallBacks));
+  CFStringRef socket_dict_key = CFSTR("ServiceProcessSocket");
+  const void *socket_keys[] = { socket_dict_key };
+  const void *socket_values[] = { sockets };

[Mark Mentovai, 2012/01/20 20:41:35]

arraysizes equal? 238 too?

[Scott Byer, 2012/01/20 23:13:37]

Done.

+  base::mac::ScopedCFTypeRef<CFDictionaryRef> socket_dict(
+      CFDictionaryCreate(kCFAllocatorDefault,
+                         socket_keys,
+                         socket_values,
+                         arraysize(socket_keys),
+                         &kCFTypeDictionaryKeyCallBacks,
+                         &kCFTypeDictionaryValueCallBacks));
+  const void *keys[] = { program, program_args, socket_key };
+  const void *values[] = { path, args, socket_dict };
   return CFDictionaryCreate(kCFAllocatorDefault,
                             keys,
                             values,
@@ -143,3 +282,8 @@ bool MockLaunchd::DeletePlist(Domain domain,
   delete_called_ = true;
   return true;
 }
+
+void MockLaunchd::SignalReady() {
+  ASSERT_TRUE(as_service_);
+  running_lock_.reset(TakeNamedLock(pipe_name_, true));
+}