Adding support for sampling crashes in Chrome on Windows.

chrome/browser/metrics/chrome_metrics_services_manager_client.cc

Index: chrome/browser/metrics/chrome_metrics_services_manager_client.cc
diff --git a/chrome/browser/metrics/chrome_metrics_services_manager_client.cc b/chrome/browser/metrics/chrome_metrics_services_manager_client.cc
index c1224a28180afad63684aa45e5b12ca5efec652b..c588a22008d41f3f18ea88f4779a1b7f0d50ae7c 100644
--- a/chrome/browser/metrics/chrome_metrics_services_manager_client.cc
+++ b/chrome/browser/metrics/chrome_metrics_services_manager_client.cc
@@ -25,8 +25,27 @@
 #include "components/variations/variations_associated_data.h"
 #include "content/public/browser/browser_thread.h"
 
+#if defined(OS_WIN)
+#include "base/win/registry.h"
+#include "chrome/common/chrome_constants.h"
+#include "chrome/installer/util/browser_distribution.h"
+#include "components/crash/content/app/crashpad.h"
+#endif  // OS_WIN
+
 namespace {
 
+#if defined(OS_WIN)
+// Type for the function pointer to enable and disable crash reporting on
+// windows. Needed because the function is loaded from chrome_elf.
+typedef void (*SetCrashUploadsEnabledPointer)(bool);
+
+// Registry path for metrics registry values.
+const wchar_t kChromeMetricsRegPath[] = L"\\MetricsReporting";
+
+// Registry key for sampling state. This is uesd to communicate with crashpad.

[rkaplow, 2016/08/10 20:16:28]

used

[jwd, 2016/08/10 22:10:02]

Done.

+const wchar_t kRegUsageStatsInSample[] = L"usagestatsinsample";

[rkaplow, 2016/08/10 20:16:28]

is lowercasestyle the right style for keys? Poking around, it seems to mostly be
UpperCamelCase... ex

https://codesearch.chromium.org/chromium/src/components/startup_metric_utils/...

or
https://codesearch.chromium.org/chromium/src/chrome/browser/metrics/chrome_me...

[jwd, 2016/08/10 22:10:02]

hm, I was using the usagestats key as example.
Done.

+#endif  // OS_WIN
+
 // Name of the variations param that defines the sampling rate.
 const char kRateParamName[] = "sampling_rate_per_mille";
 
@@ -171,6 +190,38 @@ bool ChromeMetricsServicesManagerClient::OnlyDoMetricsRecording() {
          cmdline->HasSwitch(switches::kEnableBenchmarking);
 }
 
+#if defined(OS_WIN)
+void ChromeMetricsServicesManagerClient::UpdateRunningServices(
+    bool may_record,
+    bool may_upload) {
+  // First, set the registry value so that crashpad will have the sampling state
+  // now and for subsequent runs.
+  const base::string16 registry_path =
+      BrowserDistribution::GetDistribution()->GetRegistryPath() +
+      kChromeMetricsRegPath;
+
+  base::win::RegKey reg_key(HKEY_CURRENT_USER, registry_path.c_str(),

[rkaplow, 2016/08/10 20:16:28]

it looks like we can check for creation success of a regkey, ex.

https://codesearch.chromium.org/chromium/src/components/metrics/clean_exit_be...

maybe we can use that sample pattern here for safety

[jwd, 2016/08/10 22:10:02]

Done.

+                            KEY_SET_VALUE);
+
+  reg_key.WriteValue(kRegUsageStatsInSample, IsClientInSample() ? 1 : 0);

[Alexei Svitkine (slow), 2016/08/10 21:49:33]

Can the registry code be a helper function in installer_util so that the code
that gets and sets the key is in the same place?

[jwd, 2016/08/11 21:41:02]

Done.

+
+  // Next, get crashpad to pickup the sampling state for this run.

[rkaplow, 2016/08/10 20:16:28]

what do you mean here 'this run'? Do you mean session?

[jwd, 2016/08/10 22:10:02]

yes, went with 'this run' as in the same vein as "first run". Changed to
session.

+  static SetCrashUploadsEnabledPointer set_crash_enabled = []() {
+    // The crash reporting is handled by chrome_elf.dll.
+    HMODULE elf_module = GetModuleHandle(chrome::kChromeElfDllName);
+    return reinterpret_cast<SetCrashUploadsEnabledPointer>(
+        GetProcAddress(elf_module, "SetUploadsEnabledImpl"));

[rkaplow, 2016/08/10 20:16:28]

should we define this as a const and make a note how this is synced with the
string in crashpad?

[jwd, 2016/08/10 22:10:02]

Sure, why not.

+  }();
+
+  if (set_crash_enabled) {
+    // Crashpad will use the kRegUsageStatsInSample registry value to apply
+    // sampling correctly, but may_record already reflects the sampling state.
+    // This isn't a problem though, since they will be consistent.

[rkaplow, 2016/08/10 20:16:28]

hm... not fully following the point of the comment.

We're setting the reg key to store the value - so why do we need to pass the
bool in the call here?

[jwd, 2016/08/10 22:10:02]

set_crash_enabled needs a bool argument to be able to turn crash off, mirroring
the SetUploadsEnabled function in crashpad.cc. The value being passed here
doesn't need to take the sampling state into account, it just so happens that it
does, and it's simpler to use it as is.

The comment was intended to point out the redundancy, and say it's ok.

+    set_crash_enabled(may_record && may_upload);
+  }
+}
+#endif  // defined(OS_WIN)
+
 metrics::MetricsStateManager*
 ChromeMetricsServicesManagerClient::GetMetricsStateManager() {
   DCHECK(thread_checker_.CalledOnValidThread());