Revise user-data-dir switch handling; avoid crashes.

chrome/browser/user_data_dir_extractor.cc

Index: chrome/browser/user_data_dir_extractor.cc
diff --git a/chrome/browser/user_data_dir_extractor.cc b/chrome/browser/user_data_dir_extractor.cc
index 45084c3a7b42c3c6e4388d9e31cf383d8accfa9d..3c7e9cbd1166f4fbd1194a6da026149f6993b2af 100644
--- a/chrome/browser/user_data_dir_extractor.cc
+++ b/chrome/browser/user_data_dir_extractor.cc
@@ -4,20 +4,105 @@
 
 #include "chrome/browser/user_data_dir_extractor.h"
 
+#include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/logging.h"
 #include "base/path_service.h"
+#include "chrome/browser/ui/simple_message_box.h"
 #include "chrome/common/chrome_paths.h"
+#include "chrome/common/chrome_paths_internal.h"
+#include "chrome/common/chrome_switches.h"
+#include "components/startup_metric_utils/startup_metric_utils.h"
+#include "content/public/common/main_function_params.h"
+#include "grit/chromium_strings.h"
+#include "grit/generated_resources.h"
+#include "ui/base/l10n/l10n_util.h"
+#include "ui/base/resource/resource_bundle.h"
+
+#if defined(OS_LINUX)
+#include "base/environment.h"
+#endif
+
+#if defined(OS_MACOSX) || defined(OS_WIN)
+#include "chrome/browser/policy/policy_path_parser.h"
+#endif
 
 namespace chrome {
 
+namespace {
+
+GetUserDataDirCallback* custom_get_user_data_dir_callback = NULL;
+
+void ShowUserDataDirWarning(const base::FilePath& user_data_dir) {
+  startup_metric_utils::SetNonBrowserUIDisplayed();
+
+  // Ensure the ResourceBundle is initialized for string resource access.
+  bool cleanup_resource_bundle = false;
+  if (!ResourceBundle::HasSharedInstance()) {
+    cleanup_resource_bundle = true;
+    std::string locale = l10n_util::GetApplicationLocale(std::string());
+    const char kUserDataDirDialogFallbackLocale[] = "en-US";
+    if (locale.empty())
+      locale = kUserDataDirDialogFallbackLocale;
+    ResourceBundle::InitSharedInstanceWithLocale(locale, NULL);
+  }
+
+  const base::string16& title =
+      l10n_util::GetStringUTF16(IDS_CANT_WRITE_USER_DIRECTORY_TITLE);
+  const base::string16& message =
+      l10n_util::GetStringFUTF16(IDS_CANT_WRITE_USER_DIRECTORY_SUMMARY,
+                                 user_data_dir.LossyDisplayName());
+
+  if (cleanup_resource_bundle)
+    ResourceBundle::CleanupSharedInstance();

[sky, 2014/02/13 01:35:07]

I'm not sure we've ever done this outside of tests. Doing this in production
makes me slightly nervous.

[msw, 2014/02/13 01:49:33]

Skipping this causes a DCHECK and memory leak in
ResourceBundle::InitSharedInstance when
ChromeBrowserMainParts::PreCreateThreadsImpl re-initializes the bundle. Would
you prefer this call or the DCHECK + leak (+ maybe leak suppressions?)?

[msw, 2014/02/13 01:49:33]

Seems like it's called in ChromeMainDelegate::ProcessExiting, and
remoting::UnloadResources but those are on shutdown and this is on startup, so
it might have unforeseen consequences...

Skipping this cleanup causes a DCHECK and memory leak in
ResourceBundle::InitSharedInstance when
ChromeBrowserMainParts::PreCreateThreadsImpl re-initializes the bundle. Leaving
it in doesn't seem to have any adverse effects from my local testing. Would you
prefer this call or the DCHECK + leak (+ maybe leak suppressions?)?

[sky, 2014/02/13 18:01:42]

Leave what you have in.

+
+  // More complex dialogs cannot be shown before the earliest calls here.
+  ShowMessageBox(NULL, title, message, chrome::MESSAGE_BOX_TYPE_WARNING);
+}
+
+}  // namespace
+
+void InstallCustomGetUserDataDirCallbackForTest(
+    GetUserDataDirCallback* callback) {
+  custom_get_user_data_dir_callback = callback;
+}
+
 base::FilePath GetUserDataDir(const content::MainFunctionParams& parameters) {
-  base::FilePath user_data_dir;
+  if (custom_get_user_data_dir_callback)
+    return custom_get_user_data_dir_callback->Run();
+
+  base::FilePath user_data_dir =
+      parameters.command_line.GetSwitchValuePath(switches::kUserDataDir);
+  std::string process_type =
+      parameters.command_line.GetSwitchValueASCII(switches::kProcessType);
+
+#if defined(OS_LINUX)
+  // On Linux, Chrome does not support running multiple copies under different
+  // DISPLAYs, so the profile directory can be specified in the environment to
+  // support the virtual desktop use-case.
+  if (user_data_dir.empty()) {
+    std::string user_data_dir_string;
+    scoped_ptr<base::Environment> environment(base::Environment::Create());
+    if (environment->GetVar("CHROME_USER_DATA_DIR", &user_data_dir_string) &&
+        IsStringUTF8(user_data_dir_string)) {
+      user_data_dir = base::FilePath::FromUTF8Unsafe(user_data_dir_string);
+    }
+  }
+#endif
+#if defined(OS_MACOSX) || defined(OS_WIN)
+  policy::path_parser::CheckUserDataDirPolicy(&user_data_dir);
+#endif
+
+  // If the directory does not exist and cannot be created, prompt the user.
+  if (!user_data_dir.empty() &&
+      !PathService::OverrideAndCreateIfNeeded(chrome::DIR_USER_DATA,
+          user_data_dir, chrome::ProcessNeedsProfileDir(process_type))) {
+    ShowUserDataDirWarning(user_data_dir);
+  }
 
-  // Getting the user data dir can fail if the directory isn't creatable, for
-  // example: on Windows we bring up a dialog prompting the user to pick a
-  // different directory. However, ProcessSingleton needs a real user_data_dir
-  // on Mac/Linux, so it's better to fail here than fail mysteriously elsewhere.
+  // Getting the user data directory can fail if the directory isn't creatable.
+  // ProcessSingleton needs a real user data directory on Mac/Linux, so it's
+  // better to fail here than fail mysteriously elsewhere.
   CHECK(PathService::Get(chrome::DIR_USER_DATA, &user_data_dir))
       << "Must be able to get user data directory!";
   return user_data_dir;