Adding support for sampling crashes in Chrome on Windows.

components/crash/content/app/crashpad.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 "components/crash/content/app/crashpad.h"
 
 #include <stddef.h>
 #include <string.h>
 
 #if BUILDFLAG(ENABLE_KASKO)
@@ skipping 246 matching lines @@
 #elif defined(OS_WIN)
   // On Windows, we want both the browser process and the installer and any
   // other "main, first process" to initialize things. There is no "relauncher"
   // on Windows, so this is synonymous with initial_client.
   const bool should_initialize_database_and_set_upload_policy = initial_client;
 #endif
   if (should_initialize_database_and_set_upload_policy) {
     g_database =
         crashpad::CrashReportDatabase::Initialize(database_path).release();
 
-    bool enable_uploads = false;
-    if (!crash_reporter_client->ReportingIsEnforcedByPolicy(&enable_uploads)) {
-      // Breakpad provided a --disable-breakpad switch to disable crash dumping
-      // (not just uploading) here. Crashpad doesn't need it: dumping is enabled
-      // unconditionally and uploading is gated on consent, which tests/bots
-      // shouldn't have. As a precaution, uploading is also disabled on bots
-      // even if consent is present.
-      enable_uploads = crash_reporter_client->GetCollectStatsConsent() &&
-                       !crash_reporter_client->IsRunningUnattended();
-    }
-
-    SetUploadsEnabled(enable_uploads);
+    SetUploadConsent(crash_reporter_client->GetCollectStatsConsent());
   }
 }
 
 }  // namespace
 
 void InitializeCrashpad(bool initial_client, const std::string& process_type) {
   InitializeCrashpadImpl(initial_client, process_type, false);
 }
 
 #if defined(OS_WIN)
 void InitializeCrashpadWithEmbeddedHandler(bool initial_client,
                                            const std::string& process_type) {
   InitializeCrashpadImpl(initial_client, process_type, true);
 }
 #endif  // OS_WIN
 
-void SetUploadsEnabled(bool enable_uploads) {
-  if (g_database) {
-    crashpad::Settings* settings = g_database->GetSettings();
-    settings->SetUploadsEnabled(enable_uploads);
+void SetUploadConsent(bool consent) {
+  if (!g_database)
+    return;
+
+  bool enable_uploads = false;
+  CrashReporterClient* crash_reporter_client = GetCrashReporterClient();
+  if (!crash_reporter_client->ReportingIsEnforcedByPolicy(&enable_uploads)) {
+    // Breakpad provided a --disable-breakpad switch to disable crash dumping
+    // (not just uploading) here. Crashpad doesn't need it: dumping is enabled
+    // unconditionally and uploading is gated on consent, which tests/bots
+    // shouldn't have. As a precaution, uploading is also disabled on bots even
+    // if consent is present.
+    enable_uploads = consent && !crash_reporter_client->IsRunningUnattended();
   }
+
+  crashpad::Settings* settings = g_database->GetSettings();
+  settings->SetUploadsEnabled(enable_uploads &&
+                              crash_reporter_client->GetCollectStatsInSample());
 }
 
 bool GetUploadsEnabled() {
   if (g_database) {
     crashpad::Settings* settings = g_database->GetSettings();
     bool enable_uploads;
     if (settings->GetUploadsEnabled(&enable_uploads)) {
       return enable_uploads;
     }
   }
@@ skipping 143 matching lines @@
 }
 
 #endif  // BUILDFLAG(ENABLE_KASKO)
 
 }  // namespace crash_reporter
 
 #if defined(OS_WIN)
 
 extern "C" {
 
+// This function is used in chrome_metrics_services_manager_client.cc to trigger
+// changes to the upload-enabled state. This is done when the metrics services
+// are initialized, and when the user changes their consent for uploads. See
+// crash_reporter::SetUploadConsent for effects.
+void __declspec(dllexport) __cdecl SetUploadConsentImpl(bool consent) {

[scottmg, 2016/08/18 19:49:10]

I'm sorry, I feel like I missed this along the way, but could you explain
(probably again) why this takes a bool instead of calling
crash_reporter_client->GetCollectStatsConsent()?

[jwd, 2016/08/18 20:21:33]

The reason I kept the argument is to avoid making it a requirement that the
consent value be fully persisted before calling this. On mac,
GetCollectStatsConsent relies on the consent file being written, on windows it
will be the registry values. The other parts of the logic don't really change
during a session (does policy?), and wouldn't trigger a call to this if they
did. 

It wouldn't be the biggest deal for Mac, just moving the call to the bottom of
GoogleUpdateSettings::SetCollectStatsConsent instead of the top, and my windows
code wouldn't need to change, it's already called after that point. 

The downsides are:
We wouldn't get crash coverage for the 21 lines of
GoogleUpdateSettings::SetCollectStatsConsent on Mac, and the windows code
doesn't make it exactly clear that it IS doing the right thing. And of course it
seemed a bit messy to me to have the required two steps.

[scottmg, 2016/08/18 20:33:18]

OK, I hadn't been looking at Mac. I sort of prefer one place that knows how to
read the consent bit, but your argument makes sense too.

Since this function is Windows-only, could you add DCHECK_EQ(consent,
crash_reporter_client->GetCollectStatsConsent()) here?

[jwd, 2016/08/18 21:07:34]

Done.

+  crash_reporter::SetUploadConsent(consent);
+}
+
 // NOTE: This function is used by SyzyASAN to annotate crash reports. If you
 // change the name or signature of this function you will break SyzyASAN
 // instrumented releases of Chrome. Please contact syzygy-team@chromium.org
 // before doing so! See also http://crbug.com/567781.
 void __declspec(dllexport) __cdecl SetCrashKeyValueImpl(const wchar_t* key,
                                                         const wchar_t* value) {
   crash_reporter::SetCrashKeyValue(base::UTF16ToUTF8(key),
                                    base::UTF16ToUTF8(value));
 }
 
 void __declspec(dllexport) __cdecl ClearCrashKeyValueImpl(const wchar_t* key) {
   crash_reporter::ClearCrashKey(base::UTF16ToUTF8(key));
 }
 
 }  // extern "C"
 
 #endif  // OS_WIN