Make Crashpad use the user data dir, rather than always default location

chrome/install_static/install_util.cc

Index: chrome/install_static/install_util.cc
diff --git a/chrome/install_static/install_util.cc b/chrome/install_static/install_util.cc
index fc7f95fd5a00f97bd023a78bf474340bac53b538..e05b610d4ae8cd0590565b74f40885ad147ec6f6 100644
--- a/chrome/install_static/install_util.cc
+++ b/chrome/install_static/install_util.cc
@@ -12,6 +12,7 @@
 #include <memory>
 #include <sstream>
 
+#include "chrome/install_static/policy_path_parser.h"
 #include "chrome_elf/nt_registry/nt_registry.h"
 
 namespace install_static {
@@ -61,12 +62,19 @@ const wchar_t kShowRestart[] = L"CHROME_CRASHED";
 const wchar_t kRestartInfo[] = L"CHROME_RESTART";
 const wchar_t kRtlLocale[] = L"RIGHT_TO_LEFT";
 
-const char kGpuProcess[] = "gpu-process";
-const char kPpapiPluginProcess[] = "ppapi";
-const char kRendererProcess[] = "renderer";
-const char kUtilityProcess[] = "utility";
-const char kProcessType[] = "type";
-const char kCrashpadHandler[] = "crashpad-handler";
+const wchar_t kCrashpadHandler[] = L"crashpad-handler";
+const wchar_t kProcessType[] = L"type";
+const wchar_t kUserDataDirSwitch[] = L"user-data-dir";
+const wchar_t kUtilityProcess[] = L"utility";
+
+#if defined(GOOGLE_CHROME_BUILD)
+const wchar_t kRegistryChromePolicyKey[] =
+    L"SOFTWARE\\Policies\\Google\\Chrome";
+#else
+const wchar_t kRegistryChromePolicyKey[] =
+    L"SOFTWARE\\Policies\\Chromium";
+#endif
+const wchar_t kUserDataDirRegKey[] = L"UserDataDir";
 
 namespace {
 
@@ -376,6 +384,33 @@ std::vector<StringType> TokenizeStringT(
   return tokens;
 }
 
+// Checks if the key exists in the given hive and expands any string variables.
+bool LoadUserDataDirPolicyFromRegistry(nt::ROOT_KEY hive,
+                                       const wchar_t* key_name,
+                                       std::wstring* dir) {
+  std::wstring value;
+  bool result = false;
+  HANDLE key;
+  if (nt::OpenRegKey(hive, kRegistryChromePolicyKey, KEY_READ, &key, nullptr)) {
+    if (nt::QueryRegValueSZ(key, key_name, &value)) {
+      *dir = install_static::ExpandPathVariables(value);
+      result = true;
+    }
+    nt::CloseRegKey(key);

[Mark Mentovai, 2016/11/10 22:06:17]

I’m surprised that OpenRegKey doesn’t return a key and provide a ScopedRegKey to
deal with the close. Looks like that would improve every single call site
including this one.

[scottmg, 2016/11/15 23:23:15]

Agreed. Filed https://bugs.chromium.org/p/chromium/issues/detail?id=665635 for a
followup.

+  }
+  return result;
+}
+
+void CheckUserDataDirPolicy(std::wstring* user_data_dir) {
+  assert(user_data_dir);
+  // Policy from the HKLM hive has precedence over HKCU.
+  if (!LoadUserDataDirPolicyFromRegistry(nt::HKLM, kUserDataDirRegKey,
+                                         user_data_dir)) {
+    LoadUserDataDirPolicyFromRegistry(nt::HKCU, kUserDataDirRegKey,
+                                      user_data_dir);
+  }
+}
+
 }  // namespace
 
 bool IsSxSChrome(const wchar_t* exe_path) {
@@ -531,11 +566,45 @@ bool GetDefaultUserDataDirectory(std::wstring* result) {
   return true;
 }
 
-bool GetDefaultCrashDumpLocation(std::wstring* crash_dir) {
-  // In order to be able to start crash handling very early, we do not rely on
-  // chrome's PathService entries (for DIR_CRASH_DUMPS) being available on
-  // Windows. See https://crbug.com/564398.
-  if (!GetDefaultUserDataDirectory(crash_dir))
+bool GetUserDataDirectory(std::wstring* result) {
+  // WARNING! This has to mirror the logic in chrome/app/chrome_main_delegate.cc
+  // in InitializeUserDataDir(). This function derives the path in the same way,
+  // but without using base, nor reporting on failure as the Chrome
+  // implementation does.
+  std::wstring user_data_dir = GetSwitchValueFromCommandLine(
+      ::GetCommandLine(), install_static::kUserDataDirSwitch);
+
+  CheckUserDataDirPolicy(&user_data_dir);
+
+  // On Windows, trailing separators leave Chrome in a bad state. See
+  // crbug.com/464616.
+  while (!user_data_dir.empty() &&
+         (user_data_dir.back() == '\\' || user_data_dir.back() == '/')) {
+    user_data_dir = user_data_dir.substr(0, user_data_dir.size() - 1);

[Mark Mentovai, 2016/11/10 22:06:17]

If C++11 is guaranteed (and it’s 2016, it should be, right?), just .pop_back()?

Otherwise, .erase(use_data_dir.size() - 1, 1) or .resize(user_data_dir.size() -
1) ought to be better as they work with the already-existing string.

[scottmg, 2016/11/15 23:23:15]

Done.

+  }
+
+  const bool specified_directory_was_invalid =

[Mark Mentovai, 2016/11/10 22:06:17]

This says: If user_data_dir is empty, stop and set was_invalid to false.
Otherwise, set was_invalid to !RecursiveDirectoryCreate().

Is that really what you want? It’s compatible (and bug-compatible) with the
other file, but wouldn’t we want to try GetDefaultUserDatadirectory() if
user_data_dir is empty?

was_INvalid and the !s make this pretty double-negative-y, which probably
contributed to the confusion. It should read more cleanly and be less bug-prone
if you have a more positive attitude.

  was_invalid = empty() || !RecursiveDirectoryCreate()
or
  was_valid = !empty() && RecursiveDirectoryCreate()

…if this behavior is really intended. If not, the rationale for basically
failing if user_data_dir is empty is lost on me, and someone needs a comment.

[scottmg, 2016/11/11 00:24:46]

Yeah, you're right. I just copied and adapted the other function into this one.
I didn't really process it too critically and assumed it was doing what you said
above (i.e. if one wasn't supplied, use the default and create it if necessary).

I'm not sure what the correct behaviour is now that I've started writing some
tests for it. The original implementation alludes to the browser process
handling it "later", but I'm not sure how/where that happens yet. Hmm...

[scottmg, 2016/11/15 23:23:15]

I think the behaviour is semi-rational now.

+      !user_data_dir.empty() && !RecursiveDirectoryCreate(user_data_dir);
+
+  if (specified_directory_was_invalid) {
+    if (!GetDefaultUserDataDirectory(&user_data_dir)) {

[Mark Mentovai, 2016/11/10 22:06:17]

This can be folded into the if that contains it.

[scottmg, 2016/11/15 23:23:15]

Done.

+      // The Chrome implementation CHECKs() here in the browser process. We
+      // don't as this function is used to initialize crash reporting, so
+      // we would get no report of this failure.
+      assert(false);
+      return false;
+    }
+  }
+
+  *result = user_data_dir;
+  return true;
+}
+
+bool GetCrashDumpLocation(std::wstring* crash_dir) {
+  // In order to be able to start crash handling very early and in chrome_elf,
+  // we cannot rely on chrome's PathService entries (for DIR_CRASH_DUMPS) being
+  // available on Windows. See https://crbug.com/564398.
+  if (!GetUserDataDirectory(crash_dir))
     return false;
 
   // We have to make sure the user data dir exists on first run. See
@@ -844,45 +913,45 @@ bool CompareVersionStrings(const std::string& version1,
   return true;
 }
 
-std::string GetSwitchValueFromCommandLine(const std::string& command_line,
-                                          const std::string& switch_name) {
+std::wstring GetSwitchValueFromCommandLine(const std::wstring& command_line,
+                                           const std::wstring& switch_name) {
   assert(!command_line.empty());
   assert(!switch_name.empty());
 
-  std::string command_line_copy = command_line;
+  std::wstring command_line_copy = command_line;
   // Remove leading and trailing spaces.
-  TrimT<std::string>(&command_line_copy);
+  TrimT<std::wstring>(&command_line_copy);
 
   // Find the switch in the command line. If we don't find the switch, return
   // an empty string.
-  std::string switch_token = "--";
+  std::wstring switch_token = L"--";
   switch_token += switch_name;
-  switch_token += "=";
+  switch_token += L"=";
   size_t switch_offset = command_line_copy.find(switch_token);
   if (switch_offset == std::string::npos)
-    return std::string();
+    return std::wstring();
 
   // The format is "--<switch name>=blah". Look for a space after the
   // "--<switch name>=" string. If we don't find a space assume that the switch
   // value ends at the end of the command line.
   size_t switch_value_start_offset = switch_offset + switch_token.length();
-  if (std::string(kWhiteSpaces).find(
-          command_line_copy[switch_value_start_offset]) != std::string::npos) {
+  if (std::wstring(kWhiteSpaces16).find(
+          command_line_copy[switch_value_start_offset]) != std::wstring::npos) {
     switch_value_start_offset = command_line_copy.find_first_not_of(
-        GetWhiteSpacesForType<std::string>(), switch_value_start_offset);
-    if (switch_value_start_offset == std::string::npos)
-      return std::string();
+        GetWhiteSpacesForType<std::wstring>(), switch_value_start_offset);
+    if (switch_value_start_offset == std::wstring::npos)
+      return std::wstring();
   }
   size_t switch_value_end_offset =
-      command_line_copy.find_first_of(GetWhiteSpacesForType<std::string>(),
+      command_line_copy.find_first_of(GetWhiteSpacesForType<std::wstring>(),
                                       switch_value_start_offset);
-  if (switch_value_end_offset == std::string::npos)
+  if (switch_value_end_offset == std::wstring::npos)
     switch_value_end_offset = command_line_copy.length();
 
-  std::string switch_value = command_line_copy.substr(
+  std::wstring switch_value = command_line_copy.substr(
       switch_value_start_offset,
       switch_value_end_offset - (switch_offset + switch_token.length()));
-  TrimT<std::string>(&switch_value);
+  TrimT<std::wstring>(&switch_value);
   return switch_value;
 }