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

apps/app_shim/app_shim_host_manager_mac.mm

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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;
 
 namespace {
 
 void CreateAppShimHost(const IPC::ChannelHandle& handle) {
   // AppShimHost takes ownership of itself.
   (new AppShimHost)->ServeChannel(handle);
 }
 
+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);
+}
+
 }  // namespace
 
-const base::FilePath* AppShimHostManager::g_override_user_data_dir_ = NULL;
-
 AppShimHostManager::AppShimHostManager() {}
 
 void AppShimHostManager::Init() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   apps::AppShimHandler::SetDefaultHandler(&extension_app_shim_handler_);
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
       base::Bind(&AppShimHostManager::InitOnFileThread, this));
 }
 
 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;
+
+  // 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);
-  // 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)) {
-    return;
-  }
+  base::DeleteFile(symlink_path, false);
+  base::CreateSymbolicLink(socket_path, symlink_path);
 
-  factory_.reset(new IPC::ChannelFactory(socket_path, this));
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       base::Bind(&AppShimHostManager::ListenOnIOThread, this));
 }
 
 void AppShimHostManager::ListenOnIOThread() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   if (!factory_->Listen()) {
     BrowserThread::PostTask(
         BrowserThread::UI, FROM_HERE,
         base::Bind(&AppShimHostManager::OnListenError, this));
   }
 }
 
 void AppShimHostManager::OnClientConnected(
     const IPC::ChannelHandle& handle) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       base::Bind(&CreateAppShimHost, handle));
 }
 
 void AppShimHostManager::OnListenError() {
   // TODO(tapted): Set a timeout and attempt to reconstruct the channel. Until
   // cases where the error could occur are better known, just reset the factory
   // to allow failure to be communicated via the test API.
   factory_.reset();
 }