Put app shim IPC socket in a temporary directory. (Mac)

apps/app_shim/app_shim_host_manager_mac.mm

Index: apps/app_shim/app_shim_host_manager_mac.mm
diff --git a/apps/app_shim/app_shim_host_manager_mac.mm b/apps/app_shim/app_shim_host_manager_mac.mm
index eeaa114adea0e03b5921483ca98379a4ffeee43b..4d8a7d29d54f3e8aee52b1758f4745db7ccfc446 100644
--- a/apps/app_shim/app_shim_host_manager_mac.mm
+++ b/apps/app_shim/app_shim_host_manager_mac.mm
@@ -4,18 +4,23 @@
 
 #include "apps/app_shim/app_shim_host_manager_mac.h"
 
+#include <unistd.h>

[tapted, 2014/01/03 13:06:54]

(remind me..) what's unistd for? I think mkdtemp is in stdlib.h, but I'm not in
front of a Mac right now.

[jackhou1, 2014/01/06 05:29:52]

The man page says to include unistd.h (unless I'm reading it wrong).
https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPag...

[tapted, 2014/01/06 07:31:28]

right you are - must be a BSD thing :/

+
 #include "apps/app_shim/app_shim_handler_mac.h"
 #include "apps/app_shim/app_shim_host_mac.h"
+#include "base/base64.h"
 #include "base/bind.h"
 #include "base/command_line.h"
+#include "base/file_util.h"
 #include "base/files/file_path.h"
 #include "base/logging.h"
 #include "base/path_service.h"
+#include "base/sha1.h"
+#include "base/strings/string_util.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/mac/app_mode_common.h"
-#include "ipc/unix_domain_socket_util.h"
 
 using content::BrowserThread;
 
@@ -26,9 +31,21 @@ void CreateAppShimHost(const IPC::ChannelHandle& handle) {
   (new AppShimHost)->ServeChannel(handle);
 }
 
-}  // namespace
+base::FilePath GetDirectoryInTmpTemplate(const base::FilePath& user_data_dir) {
+  std::string udd_hash;
+  base::Base64Encode(base::SHA1HashString(user_data_dir.value()), &udd_hash);
+  base::ReplaceChars(udd_hash, "/", "_", &udd_hash);
+  DCHECK_EQ(28u, udd_hash.length());
+  return base::FilePath("/tmp")
+      .Append("chrome-" + udd_hash + ".XXXXXX");

[tapted, 2014/01/03 13:06:54]

nit: pretty sure this will fit on the line above

[jackhou1, 2014/01/06 05:29:52]

Done.

+}
+
+void DeleteFile(base::FilePath path) {

[tapted, 2014/01/03 13:06:54]

nit: const reference for |path|?

[jackhou1, 2014/01/06 05:29:52]

Done.

+  if (!path.empty())
+    base::DeleteFile(path, true);
+}
 
-const base::FilePath* AppShimHostManager::g_override_user_data_dir_ = NULL;
+}  // namespace
 
 AppShimHostManager::AppShimHostManager() {}
 
@@ -42,31 +59,47 @@ void AppShimHostManager::Init() {
 
 AppShimHostManager::~AppShimHostManager() {
   apps::AppShimHandler::SetDefaultHandler(NULL);
+  factory_.reset();
+  BrowserThread::PostTask(
+      BrowserThread::FILE, FROM_HERE,
+      base::Bind(&DeleteFile, directory_in_tmp_));

[tapted, 2014/01/03 13:06:54]

We should delete the symlink in the user data dir too, so we don't leave it
hanging.

Maybe a new (static!) function taking two arguments (the symlink could probably
be derived again from PathService, but feels safer to have it passed in since
this happens during shutdown.

[jackhou1, 2014/01/06 05:29:52]

Done.

 }
 
 void AppShimHostManager::InitOnFileThread() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   base::FilePath user_data_dir;
-  if (g_override_user_data_dir_) {
-    user_data_dir = *g_override_user_data_dir_;
-  } else if (!PathService::Get(chrome::DIR_USER_DATA, &user_data_dir)) {
-    LOG(ERROR) << "Couldn't get user data directory while creating App Shim "
-               << "Host manager.";
+  if (!PathService::Get(chrome::DIR_USER_DATA, &user_data_dir))
     return;
-  }
 
-  base::FilePath socket_path =
-      user_data_dir.Append(app_mode::kAppShimSocketName);
-  // This mirrors a check in unix_domain_socket_util.cc which will guarantee
-  // failure and spam log files on bots because they have deeply nested paths to
-  // |user_data_dir| when swarming. See http://crbug.com/240554. Shim tests that
-  // run on the bots must override the path using AppShimHostManagerTestApi.
-  if (socket_path.value().length() >= IPC::kMaxSocketNameLength &&
-      CommandLine::ForCurrentProcess()->HasSwitch(switches::kTestType)) {
+  // The socket path must be shorter than 104 chars (IPC::kMaxSocketNameLength).
+  // To accommodate this, we use a short path in /tmp/ that is generated from a
+  // hash of the user data dir.
+  std::string directory_string =
+      GetDirectoryInTmpTemplate(user_data_dir).value();
+
+  // |mkdtemp| replaces trailing X's randomly and creates the directory.

[tapted, 2014/01/03 13:06:54]

nit: |mkdtemp| -> mkdtemp().

[jackhou1, 2014/01/06 05:29:52]

Done.

+  if (!mkdtemp(const_cast<char*>(directory_string.c_str())))

[tapted, 2014/01/03 13:06:54]

nit: if you use directory_string.data() (or &directory_string[0] if we still
have an ancient compiler that doesn't have .data() yet), you should be able to
avoid the const_cast

[jackhou1, 2014/01/06 05:29:52]

Using &directory_string[0] as .data() also returns a const char*.

+    return;

[tapted, 2014/01/03 13:06:54]

Maybe `PLOG(ERROR) << directory_string` before this? Seems like an appropriate
case for logging. Otherwise, perhaps some comment, since it's not too clear that
a failure here is an unlikely system error we don't really have control over.

[jackhou1, 2014/01/06 05:29:52]

Done.

+
+  directory_in_tmp_ = base::FilePath(directory_string);
+  // Check that the directory was created with the correct permissions.
+  int dir_mode = 0;
+  if (!base::GetPosixFilePermissions(directory_in_tmp_, &dir_mode) ||
+      base::FILE_PERMISSION_USER_MASK != dir_mode) {

[tapted, 2014/01/03 13:06:54]

nit: I'd maybe swap dir_mode and FILE_PERMISSION_USER_MASK to avoid yoda-style
(which tends to only get used for things like CHECK_EQ). (and.. somewhat weird
checking equality against something called FOO_MASK rather than using it as a
mask, but I think it's fine here).

[jackhou1, 2014/01/06 05:29:52]

Done.

     return;

[tapted, 2014/01/03 13:06:54]

This could perhaps have a NOTREACHED() before it -- mkdtemp should have already
returned NULL unless "something weird" happened.

[jackhou1, 2014/01/06 05:29:52]

Done.

   }
 
+  // IPC::ChannelFactory creates the socket immediately.
+  base::FilePath socket_path =
+      directory_in_tmp_.Append(app_mode::kAppShimSocketName);
   factory_.reset(new IPC::ChannelFactory(socket_path, this));
+
+  // Create a symlink to the socket in the UDD.

[tapted, 2014/01/03 13:06:54]

This is a bit subtle - maybe a wordier comment, like "This lets the shim process
started from Finder find the actual socket path by following the symlink with
::readlink()" (also nit: UDD -> user data dir.)

[jackhou1, 2014/01/06 05:29:52]

Done.

+  base::FilePath symlink_path =
+      user_data_dir.Append(app_mode::kAppShimSocketName);
+  base::DeleteFile(symlink_path, false);
+  base::CreateSymbolicLink(socket_path, symlink_path);
+
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       base::Bind(&AppShimHostManager::ListenOnIOThread, this));