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 277 matching lines @@
 #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);
+    settings->SetUploadsEnabled(
+        enable_uploads && GetCrashReporterClient()->GetCollectStatsInSample());

[robertshield, 2016/08/12 17:49:25]

The logic for whether uploads are enabled or not is now split between
SetUploadsEnabled() and InitializeCrashpadImpl(). I think it should all be in
InitializeCrashpadImpl().

It's surprising that SetUploadsEnabled(true) might not actually enable uploads.

[jwd, 2016/08/12 20:04:31]

I don't disagree. But, keeping SetUploadsEnabled as is, would mean anyone
calling it needs to know about sampling to decide the value passed. Mac calls it
when the consent setting is changed in settings.

[Mark Mentovai, 2016/08/12 20:16:27]

The intent is to do this on Windows too. https://crbug.com/629618.

[robertshield, 2016/08/12 21:04:43]

If this method is to contain logic that can cause it to ignore the value of
|enable_uploads|, then it shouldn't be called SetUploadsEnabled(), it should be
called MaybeSetUploadsEnabled() or TryToEnableUploads() or some such.

As is, the implementation is surprising.

[Mark Mentovai, 2016/08/12 22:01:57]

Changing the name to something more suitable is fine.
SetUploadsEnabledUserPreference()?

   }
 }
 
 bool GetUploadsEnabled() {
   if (g_database) {
     crashpad::Settings* settings = g_database->GetSettings();
     bool enable_uploads;
     if (settings->GetUploadsEnabled(&enable_uploads)) {
       return enable_uploads;
     }
@@ skipping 144 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::SetUploadsEnabled for effects.
+void __declspec(dllexport) __cdecl SetUploadsEnabledImpl(bool enable_uploads) {
+  crash_reporter::SetUploadsEnabled(enable_uploads);

[Alexei Svitkine (slow), 2016/08/12 17:47:48]

So this will be called as a result of UpdateRunningServices() in metrics code.

At which point, it should override whatever was done by line 278, right?

Could there be an inconsistency there?

For example, I see the line 278 code has some extra checks, like:
  - should_initialize_database_and_set_upload_policy
  - crash_reporter_client->ReportingIsEnforcedByPolicy()
  - crash_reporter_client->IsRunningUnattended()

Seems like all those things that may have caused line 278 previously to not
enable uploads may suddenly get overridden here. Am I reading it right?

If so, it seems like an issue. Can we make the two go through the same code path
that checks all these things?

[jwd, 2016/08/12 20:04:31]

Yes, you're right. This is already the case, since SetUploadsEnabled is being
called on Mac through GoogleUpdateSettings::SetCollectStatsConsent. This extern
was just intended to make SetUploadsEnabled available to windows in the same way
that it is on Mac.

+}
+
 // 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