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..6cabf58e53c523931274ac79e95359edd162afd8 100644
--- a/chrome/browser/metrics/chrome_metrics_services_manager_client.cc
+++ b/chrome/browser/metrics/chrome_metrics_services_manager_client.cc
@@ -25,8 +25,32 @@
 #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 used to communicate with crashpad.

[Mark Mentovai, 2016/08/11 18:32:24]

Capital C here and on line 213 too.

[scottmg, 2016/08/11 21:38:03]

and line 202

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

Done.

[jwd, 2016/08/11 22:02:52]

Done.

+const wchar_t kRegUsageStatsInSample[] = L"UsageStatsInSample";
+
+// The name of the function used to set the uploads enabled state in
+// components/crash/content/app/crashpad.cc. This is used to call the function
+// through the chrome_elf dll.
+const char kCrashpadSetEnabledFunctionName[] = "SetUploadsEnabledImpl";
+#endif  // OS_WIN
+
 // Name of the variations param that defines the sampling rate.
 const char kRateParamName[] = "sampling_rate_per_mille";
 
@@ -171,6 +195,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;
+  if (reg_key.Create(HKEY_CURRENT_USER, registry_path.c_str(), KEY_SET_VALUE) ==
+      ERROR_SUCCESS)

[scottmg, 2016/08/11 21:38:03]

nit; {} around multiline if

[jwd, 2016/08/11 22:02:53]

Done.

+    reg_key.WriteValue(kRegUsageStatsInSample, IsClientInSample() ? 1 : 0);
+
+  // Next, get crashpad to pickup the sampling state for this session.

[scottmg, 2016/08/11 21:38:03]

pick up

[jwd, 2016/08/11 22:02:53]

Done.

+  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, kCrashpadSetEnabledFunctionName));
+  }();
+
+  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.

[scottmg, 2016/08/11 21:38:03]

It seems like instead of this comment, it'd be better just to tell the
chrome_elf side to "check again", so that we don't need to retrieve and then
send over the value that we expect to be the same. Would that work?

[jwd, 2016/08/11 22:02:53]

Not too sure exactly what you mean. The value passed to set_crash_enabled won't
always be the same as the sampling value, it also has the consent baked in. It
just happens that, by this point, MetricsServicesManager[Client] expects
may_record to be consent given && in sample.

Are you suggesting a new "check again" function in crashpad.cc and not call
SetUploadsEnabled here?
To be strictly right, it would need to apply all the same logic that
InitializeCrashpadImpl does for the enable_uploads value. This seems like more
work since it will have to retrieve the consent bit from registry and check
policy and such.

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