Adding support for sampling crashes in Chrome on Windows.

chrome/browser/metrics/chrome_metrics_services_manager_client.cc

 // Copyright 2015 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 "chrome/browser/metrics/chrome_metrics_services_manager_client.h"
 
 #include "base/command_line.h"
 #include "base/feature_list.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/metrics/chrome_metrics_service_accessor.h"
 #include "chrome/browser/metrics/chrome_metrics_service_client.h"
 #include "chrome/browser/metrics/variations/chrome_variations_service_client.h"
 #include "chrome/browser/metrics/variations/ui_string_overrider_factory.h"
 #include "chrome/browser/ui/browser_otr_state.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/installer/util/google_update_settings.h"
 #include "components/metrics/enabled_state_provider.h"
 #include "components/metrics/metrics_state_manager.h"
 #include "components/prefs/pref_service.h"
 #include "components/rappor/rappor_service.h"
 #include "components/variations/service/variations_service.h"
 #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";
 
 // Metrics reporting feature. This feature, along with user consent, controls if
 // recording and reporting are enabled. If the feature is enabled, but no
 // consent is given, then there will be no recording or reporting.
 const base::Feature kMetricsReportingFeature{"MetricsReporting",
                                              base::FEATURE_ENABLED_BY_DEFAULT};
 
 // Posts |GoogleUpdateSettings::StoreMetricsClientInfo| on blocking pool thread
@@ skipping 124 matching lines @@
 bool ChromeMetricsServicesManagerClient::IsMetricsReportingEnabled() {
   return enabled_state_provider_->IsReportingEnabled();
 }
 
 bool ChromeMetricsServicesManagerClient::OnlyDoMetricsRecording() {
   const base::CommandLine* cmdline = base::CommandLine::ForCurrentProcess();
   return cmdline->HasSwitch(switches::kMetricsRecordingOnly) ||
          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());
   if (!metrics_state_manager_) {
     metrics_state_manager_ = metrics::MetricsStateManager::Create(
         local_state_, enabled_state_provider_.get(),
         base::Bind(&PostStoreMetricsClientInfo),
         base::Bind(&GoogleUpdateSettings::LoadMetricsClientInfo));
   }
   return metrics_state_manager_.get();
 }