Adding support for sampling crashes in Chrome on Windows.

components/crash/content/app/crashpad.cc

Index: components/crash/content/app/crashpad.cc
diff --git a/components/crash/content/app/crashpad.cc b/components/crash/content/app/crashpad.cc
index 85afe46af675145d2b1251ea5a3b9e5989123eb2..37aeb4c5403700f1c513831f5d772b9ca1fbb90e 100644
--- a/components/crash/content/app/crashpad.cc
+++ b/components/crash/content/app/crashpad.cc
@@ -295,7 +295,8 @@ void InitializeCrashpadWithEmbeddedHandler(bool initial_client,
 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()?

   }
 }
 
@@ -460,6 +461,14 @@ void ReadMainModuleAnnotationsForKasko(
 
 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