Stability instrumentation Crashpad integration

chrome/app/chrome_crash_reporter_client_win.cc

Index: chrome/app/chrome_crash_reporter_client_win.cc
diff --git a/chrome/app/chrome_crash_reporter_client_win.cc b/chrome/app/chrome_crash_reporter_client_win.cc
index 166a13df213ba18e27c5b895c40dd31f932cb6e6..9feae96bfbed6e58a9dce6f8ccd189b66b57fb3a 100644
--- a/chrome/app/chrome_crash_reporter_client_win.cc
+++ b/chrome/app/chrome_crash_reporter_client_win.cc
@@ -14,6 +14,7 @@
 #include <iterator>
 #include <memory>
 #include <string>
+#include <vector>
 
 #include "base/command_line.h"
 #include "base/debug/crash_logging.h"
@@ -240,13 +241,17 @@ void ChromeCrashReporterClient::InitializeCrashReportingForProcess() {
 
   std::wstring process_type = install_static::GetSwitchValueFromCommandLine(
       ::GetCommandLine(), install_static::kProcessType);
+  std::wstring user_data_dir;
+  // DO NOT SUBMIT: validate use of CHECK.
+  CHECK(install_static::GetUserDataDirectory(&user_data_dir, nullptr));

[grt (UTC plus 2), 2017/05/11 21:08:16]

i think this is wrong. the one true user data dir is cached in chrome_elf, so
you should to thunk your way to it like
https://cs.chromium.org/chromium/src/chrome/app/chrome_main_delegate.cc?type=....

that's a bit clumsy.

an alternative would be to add the user data dir to InstallDetails::Payload and
InstallDetails and compute it in install_static::MakeProductDetails. callers in
chrome.exe and chrome.dll could then do:

install_static::InstallDetails::Get().user_data_dir();

and get the one true user data dir cached by chrome_elf.

[manzagop (departed), 2017/05/12 19:27:32]

I tried the second approach. 
- I needed to make the mode a parameter to install_static::GetUserDataDirectory
to avoid the recursion in InstallDetails.
- I put a few "DO NO SUBMIT" in places that still need to be revised: 1)
ChromeCrashReporterClient::GetCrashMetricsLocation is called outside elf and
calls GetUserDataDirectory.
2) Same with GetCrashDumpLocation.
- I wanted your input on what to do if the call fails before changing those
other places.

[grt (UTC plus 2), 2017/05/15 13:03:18]

Ah, I see now that the user data dir is only computed/needed in the browser
process. Since MakeProductDetails is run for all process types (something I
would like to change one day), this maybe isn't the right thing after all.
Otherwise, there would be one field in InstallDetails that is only
initialized/valid in the browser process, which seems a little icky too. I fear
I sent you down the garden path. The thunk may be the best for now.

I'm not sure why the thunk is resolved manually at runtime rather than using the
linker -- note how GetInstallDetailsPayload is exported from chrome_elf and used
in other modules via an explicit dllimport in initialize_from_primary_module.cc.
You could simplify the use of the thunk like this by adding a function to
//chrome/install_static:secondary_module that is used by chrome.dll to get the
paths. the user data dir getter should be in
//chrome/install_static:primary_module so that it isn't at risk of being called
directly from other modules.

apologies for leading you down the garden path with that other idea.

[manzagop (departed), 2017/05/15 13:43:45]

Why do you say the user-data-dir would be not init / invalid in other processes?
Do you mean instead that it's useless to compute it in other processes?

[manzagop (departed), 2017/05/15 14:21:35]

As per offline discussion, the concern is wasted work or work that shouldn't /
can't be done (e.g. because of sandboxing).

   // Don't set up Crashpad crash reporting in the Crashpad handler itself, nor
   // in the fallback crash handler for the Crashpad handler process.
   if (process_type != install_static::kCrashpadHandler &&
       process_type != install_static::kFallbackHandler) {
     crash_reporter::SetCrashReporterClient(instance);
     crash_reporter::InitializeCrashpadWithEmbeddedHandler(
-        process_type.empty(), install_static::UTF16ToUTF8(process_type));
+        process_type.empty(), install_static::UTF16ToUTF8(process_type),
+        install_static::UTF16ToUTF8(user_data_dir));
   }
 }
 #endif  // NACL_WIN64