Stability instrumentation Crashpad integration

chrome/app/chrome_crash_reporter_client_win.cc

 // 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.
 
 // TODO(ananta/scottmg)
 // Add test coverage for Crashpad.
 #include "chrome/app/chrome_crash_reporter_client_win.h"
 
 #include <windows.h>
 
 #include <assert.h>
 #include <shellapi.h>
 
 #include <iterator>
 #include <memory>
 #include <string>
+#include <vector>
 
 #include "base/command_line.h"
 #include "base/debug/crash_logging.h"
 #include "base/debug/leak_annotations.h"
 #include "base/format_macros.h"
 #include "base/rand_util.h"
 #include "chrome/common/chrome_result_codes.h"
 #include "chrome/install_static/install_util.h"
 #include "chrome/install_static/user_data_dir.h"
 #include "components/crash/content/app/crashpad.h"
@@ skipping 206 matching lines @@
 void ChromeCrashReporterClient::InitializeCrashReportingForProcess() {
   static ChromeCrashReporterClient* instance = nullptr;
   if (instance)
     return;
 
   instance = new ChromeCrashReporterClient();
   ANNOTATE_LEAKING_OBJECT_PTR(instance);
 
   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
 
 bool ChromeCrashReporterClient::GetAlternativeCrashDumpLocation(
     base::string16* crash_dir) {
   // By setting the BREAKPAD_DUMP_LOCATION environment variable, an alternate
   // location to write breakpad crash dumps can be set.
   *crash_dir =
       install_static::GetEnvironmentString16(L"BREAKPAD_DUMP_LOCATION");
@@ skipping 148 matching lines @@
 
   return base::RandDouble() < probability;
 }
 
 bool ChromeCrashReporterClient::EnableBreakpadForProcess(
     const std::string& process_type) {
   // This is not used by Crashpad (at least on Windows).
   NOTREACHED();
   return true;
 }