Eliminate another GetModuleHandle/GetProcAddress thunking instance to chrome_elf.

Bug: 
Cq-Include-Trybots: master.tryserver.chromium.win:win10_chromium_x64_rel_ng
Change-Id: Ie52b94fb16a240c5f6549560e6e99982fc28b924
Reviewed-on: https://chromium-review.googlesource.com/596416
Commit-Queue: Sigurður Ásgeirsson <siggi@chromium.org>
Reviewed-by: Robert Shield <robertshield@chromium.org>
Reviewed-by: Greg Thompson <grt@chromium.org>
Reviewed-by: Brett Wilson <brettw@chromium.org>
Reviewed-by: Scott Graham <scottmg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491794}

chrome/app/BUILD.gn

@@ -348,6 +348,7 @@ static_library("test_support") {
     deps += [
       "//chrome:chrome_initial",
       "//chrome/install_static:install_static_util",
+      "//chrome_elf:test_stubs",
       "//sandbox/win:sandbox",
     ]
   }

chrome/app/chrome_crash_reporter_client_win.cc

@@ -239,7 +239,7 @@ void ChromeCrashReporterClient::InitializeCrashReportingForProcess() {
   instance = new ChromeCrashReporterClient();
   ANNOTATE_LEAKING_OBJECT_PTR(instance);
 
-  base::string16 process_type = install_static::GetSwitchValueFromCommandLine(
+  std::wstring process_type = install_static::GetSwitchValueFromCommandLine(
       ::GetCommandLine(), install_static::kProcessType);
   // Don't set up Crashpad crash reporting in the Crashpad handler itself, nor
   // in the fallback crash handler for the Crashpad handler process.
@@ -247,8 +247,7 @@ void ChromeCrashReporterClient::InitializeCrashReportingForProcess() {
       process_type != install_static::kFallbackHandler) {
     crash_reporter::SetCrashReporterClient(instance);
 
-    // Pass along the user data directory from the browser process.
-    base::string16 user_data_dir;
+    std::wstring user_data_dir;
     if (process_type.empty())
       install_static::GetUserDataDirectory(&user_data_dir, nullptr);
 

chrome/app/chrome_main_delegate.cc

@@ -72,6 +72,7 @@
 #include "chrome/browser/downgrade/user_data_downgrade.h"
 #include "chrome/child/v8_breakpad_support_win.h"
 #include "chrome/common/child_process_logging.h"
+#include "chrome_elf/chrome_elf_main.h"
 #include "sandbox/win/src/sandbox.h"
 #include "ui/base/resource/resource_bundle_win.h"
 #endif
@@ -390,38 +391,20 @@ struct MainFunction {
 // Initializes the user data dir. Must be called before InitializeLocalState().
 void InitializeUserDataDir(base::CommandLine* command_line) {
 #if defined(OS_WIN)
+  // Reach out to chrome_elf for the truth on the user data directory.
+  // Note that in tests, this links to chrome_elf_test_stubs.
   wchar_t user_data_dir_buf[MAX_PATH], invalid_user_data_dir_buf[MAX_PATH];
-
-  using GetUserDataDirectoryThunkFunction =
-      void (*)(wchar_t*, size_t, wchar_t*, size_t);
-  HMODULE elf_module = GetModuleHandle(chrome::kChromeElfDllName);
-  if (elf_module) {
-    // If we're in a test, chrome_elf won't be loaded.
-    GetUserDataDirectoryThunkFunction get_user_data_directory_thunk =
-        reinterpret_cast<GetUserDataDirectoryThunkFunction>(
-            GetProcAddress(elf_module, "GetUserDataDirectoryThunk"));
-    get_user_data_directory_thunk(
-        user_data_dir_buf, arraysize(user_data_dir_buf),
-        invalid_user_data_dir_buf, arraysize(invalid_user_data_dir_buf));
-    base::FilePath user_data_dir(user_data_dir_buf);
-    if (invalid_user_data_dir_buf[0] != 0) {
-      chrome::SetInvalidSpecifiedUserDataDir(
-          base::FilePath(invalid_user_data_dir_buf));
-      command_line->AppendSwitchPath(switches::kUserDataDir, user_data_dir);
-    }
-    CHECK(PathService::OverrideAndCreateIfNeeded(chrome::DIR_USER_DATA,
-                                                 user_data_dir, false, true));
-  } else {
-    // In tests, just respect the flag if given.
-    base::FilePath user_data_dir =
-        command_line->GetSwitchValuePath(switches::kUserDataDir);
-    if (!user_data_dir.empty()) {
-      if (user_data_dir.EndsWithSeparator())
-        user_data_dir = user_data_dir.StripTrailingSeparators();
-      CHECK(PathService::OverrideAndCreateIfNeeded(chrome::DIR_USER_DATA,
-                                                   user_data_dir, false, true));
-    }
+  GetUserDataDirectoryThunk(user_data_dir_buf, arraysize(user_data_dir_buf),
+                            invalid_user_data_dir_buf,
+                            arraysize(invalid_user_data_dir_buf));
+  base::FilePath user_data_dir(user_data_dir_buf);
+  if (invalid_user_data_dir_buf[0] != 0) {
+    chrome::SetInvalidSpecifiedUserDataDir(
+        base::FilePath(invalid_user_data_dir_buf));
+    command_line->AppendSwitchPath(switches::kUserDataDir, user_data_dir);
   }
+  CHECK(PathService::OverrideAndCreateIfNeeded(chrome::DIR_USER_DATA,
+                                               user_data_dir, false, true));
 #else  // OS_WIN
   base::FilePath user_data_dir =
       command_line->GetSwitchValuePath(switches::kUserDataDir);

chrome/installer/setup/installer_crash_reporting.cc

@@ -93,8 +93,8 @@ void ConfigureCrashReporting(const InstallerState& installer_state) {
     }
   }
 
-  crash_reporter::InitializeCrashpadWithEmbeddedHandler(
-      true, "Chrome Installer", std::string());
+  crash_reporter::InitializeCrashpadWithEmbeddedHandler(true,
+                                                        "Chrome Installer", "");
 
   // Set up the metrics client id (a la child_process_logging::Init()).
   std::unique_ptr<metrics::ClientInfo> client_info =

chrome_elf/BUILD.gn

@@ -66,6 +66,22 @@ shared_library("chrome_elf") {
   }
 }
 
+# For code that isn't Chrome-the browser, like test binaries, these stubs stand
+# in for chrome_elf.;
+static_library("test_stubs") {
+  testonly = true
+
+  sources = [
+    "chrome_elf_main.h",
+    "chrome_elf_test_stubs.cc",
+  ]
+
+  deps = [
+    "//base",
+    "//chrome/common:constants",
+  ]
+}
+
 ##------------------------------------------------------------------------------
 ## source sets
 ##------------------------------------------------------------------------------
@@ -144,7 +160,7 @@ static_library("crash") {
   ]
   deps = [
     ":hook_util",
-    "//base:base",  # This needs to go.  DEP of app, crash_keys, client.
+    "//base",  # This needs to go.  DEP of app, crash_keys, client.
     "//base:base_static",  # pe_image
     "//chrome/install_static:install_static_util",
     "//components/crash/content/app:app",

chrome_elf/DEPS

@@ -2,6 +2,7 @@ include_rules = [
   "+sandbox",
   "+breakpad/src/client",
   "+chrome/app/chrome_crash_reporter_client_win.h",
+  "+chrome/common/chrome_switches.h",
   "+chrome/common/chrome_version.h",
   "+chrome/install_static",
   "+third_party/crashpad/crashpad/client/crashpad_client.h",

chrome_elf/chrome_elf_main.cc

@@ -29,10 +29,10 @@ void SignalChromeElf() {
   blacklist::ResetBeacon();
 }
 
-extern "C" void GetUserDataDirectoryThunk(wchar_t* user_data_dir,
-                                          size_t user_data_dir_length,
-                                          wchar_t* invalid_user_data_dir,
-                                          size_t invalid_user_data_dir_length) {
+void GetUserDataDirectoryThunk(wchar_t* user_data_dir,
+                               size_t user_data_dir_length,
+                               wchar_t* invalid_user_data_dir,
+                               size_t invalid_user_data_dir_length) {
   std::wstring user_data_dir_str, invalid_user_data_dir_str;
   bool ret = install_static::GetUserDataDirectory(&user_data_dir_str,
                                                   &invalid_user_data_dir_str);

chrome_elf/chrome_elf_main.h

@@ -5,8 +5,23 @@
 #ifndef CHROME_ELF_CHROME_ELF_MAIN_H_
 #define CHROME_ELF_CHROME_ELF_MAIN_H_
 
-extern "C" void SignalInitializeCrashReporting();
-extern "C" void SignalChromeElf();
-extern "C" void DumpProcessWithoutCrash();
+// These functions are the cross-module import interface to chrome_elf.dll.
+// It is used chrome.exe, chrome.dll and other clients of chrome_elf.
+// In tests, these functions are stubbed by implementations in
+// chrome_elf_test_stubs.cc.
+extern "C" {
+
+void DumpProcessWithoutCrash();
+void GetUserDataDirectoryThunk(wchar_t* user_data_dir,
+                               size_t user_data_dir_length,
+                               wchar_t* invalid_user_data_dir,
+                               size_t invalid_user_data_dir_length);
+// This function is a temporary workaround for https://crbug.com/655788. We
+// need to come up with a better way to initialize crash reporting that can
+// happen inside DllMain().
+void SignalInitializeCrashReporting();
+void SignalChromeElf();
+
+}  // extern "C"
 
 #endif  // CHROME_ELF_CHROME_ELF_MAIN_H_

chrome_elf/chrome_elf_test_stubs.cc

+// Copyright 2017 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 "base/command_line.h"
+#include "base/files/file_path.h"
+#include "base/path_service.h"
+#include "chrome/common/chrome_switches.h"
+#include "chrome_elf/chrome_elf_main.h"
+
+// This function is a temporary workaround for https://crbug.com/655788. We
+// need to come up with a better way to initialize crash reporting that can
+// happen inside DllMain().
+void SignalInitializeCrashReporting() {}
+
+void SignalChromeElf() {}
+
+void GetUserDataDirectoryThunk(wchar_t* user_data_dir,
+                               size_t user_data_dir_length,
+                               wchar_t* invalid_user_data_dir,
+                               size_t invalid_user_data_dir_length) {
+  // In tests, just respect the user-data-dir switch if given.
+  base::FilePath user_data_dir_path =
+      base::CommandLine::ForCurrentProcess()->GetSwitchValuePath(
+          switches::kUserDataDir);
+  if (!user_data_dir_path.empty() && user_data_dir_path.EndsWithSeparator())
+    user_data_dir_path = user_data_dir_path.StripTrailingSeparators();
+
+  wcsncpy_s(user_data_dir, user_data_dir_length,
+            user_data_dir_path.value().c_str(), _TRUNCATE);
+  wcsncpy_s(invalid_user_data_dir, invalid_user_data_dir_length, L"",
+            _TRUNCATE);
+}

components/crash/content/app/crashpad.cc

@@ -119,18 +119,8 @@ void InitializeCrashpadImpl(bool initial_client,
   }
 
   // database_path is only valid in the browser process.
-#if defined(OS_WIN)
-  internal::PlatformCrashpadInitializationOptions init_options = {};
-  init_options.user_data_dir = user_data_dir;
-
-  base::FilePath database_path = internal::PlatformCrashpadInitialization(
-      initial_client, browser_process, embedded_handler, &init_options);
-#else
-  DCHECK(user_data_dir.empty());
-
   base::FilePath database_path = internal::PlatformCrashpadInitialization(
-      initial_client, browser_process, embedded_handler, nullptr);
-#endif
+      initial_client, browser_process, embedded_handler, user_data_dir);
 
   crashpad::CrashpadInfo* crashpad_info =
       crashpad::CrashpadInfo::GetCrashpadInfo();

components/crash/content/app/crashpad.h

@@ -120,21 +120,14 @@ void GetPlatformCrashpadAnnotations(
     std::map<std::string, std::string>* annotations);
 #endif  // defined(OS_WIN)
 
-struct PlatformCrashpadInitializationOptions {
-#if defined(OS_WIN)
-  // On Windows, if |user_data_dir| is non-empty, it will be passed to the
-  // handler process for use by Chrome Crashpad extensions.
-  std::string user_data_dir;
-#endif
-};
-
-// The platform-specific portion of InitializeCrashpad().
+// The platform-specific portion of InitializeCrashpad(). On windows, if
+// user_data_dir is non-empty, the user data directory will be passed to the
+// handler process for use by Chrome Crashpad extensions.
 // Returns the database path, if initializing in the browser process.
-base::FilePath PlatformCrashpadInitialization(
-    bool initial_client,
-    bool browser_process,
-    bool embedded_handler,
-    PlatformCrashpadInitializationOptions* init_options);
+base::FilePath PlatformCrashpadInitialization(bool initial_client,
+                                              bool browser_process,
+                                              bool embedded_handler,
+                                              const std::string& user_data_dir);
 
 }  // namespace internal
 

components/crash/content/app/crashpad_mac.mm

@@ -35,7 +35,7 @@ base::FilePath PlatformCrashpadInitialization(
     bool initial_client,
     bool browser_process,
     bool embedded_handler,
-    PlatformCrashpadInitializationOptions* init_options) {
+    const std::string& user_data_dir) {
   base::FilePath database_path;  // Only valid in the browser process.
   base::FilePath metrics_path;  // Only valid in the browser process.
   DCHECK(!embedded_handler);  // This is not used on Mac.

components/crash/content/app/crashpad_win.cc

@@ -56,7 +56,7 @@ base::FilePath PlatformCrashpadInitialization(
     bool initial_client,
     bool browser_process,
     bool embedded_handler,
-    PlatformCrashpadInitializationOptions* init_options) {
+    const std::string& user_data_dir) {
   base::FilePath database_path;  // Only valid in the browser process.
   base::FilePath metrics_path;  // Only valid in the browser process.
 
@@ -107,14 +107,11 @@ base::FilePath PlatformCrashpadInitialization(
     // standalone crashpad_handler.exe (for tests, etc.).
     std::vector<std::string> start_arguments;
     if (embedded_handler) {
-      // The command line manipulations here want to be refactored into the
-      // crash_reporter::CrashReporterClient interface.
-      // See https://crbug.com/752102.
       start_arguments.push_back(std::string("--type=") +
                                 switches::kCrashpadHandler);
-      if (init_options && !init_options->user_data_dir.empty()) {
-        start_arguments.push_back("--user-data-dir=" +
-                                  init_options->user_data_dir);
+      if (!user_data_dir.empty()) {
+        start_arguments.push_back(std::string("--user-data-dir=") +
+                                  user_data_dir);
       }
       // The prefetch argument added here has to be documented in
       // chrome_switches.cc, below the kPrefetchArgument* constants. A constant