Enabling sampling of UMA and crash reports on Android.

This has a side effect of causing metrics reporting to be enabled during the session that the pref is changed.

BUG=609987

Review-Url: https://codereview.chromium.org/2248243002
Cr-Commit-Position: refs/heads/master@{#414482}

chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadCallable.java

@@ -56,13 +56,15 @@ public class MinidumpUploadCallable implements Callable<Integer> {
         UPLOAD_SUCCESS,
         UPLOAD_FAILURE,
         UPLOAD_USER_DISABLED,
-        UPLOAD_COMMANDLINE_DISABLED
+        UPLOAD_COMMANDLINE_DISABLED,
+        UPLOAD_DISABLED_BY_SAMPLING
     })
     public @interface MinidumpUploadStatus {}
     public static final int UPLOAD_SUCCESS = 0;
     public static final int UPLOAD_FAILURE = 1;
     public static final int UPLOAD_USER_DISABLED = 2;
     public static final int UPLOAD_COMMANDLINE_DISABLED = 3;
+    public static final int UPLOAD_DISABLED_BY_SAMPLING = 4;
 
     private final File mFileToUpload;
     private final File mLogfile;
@@ -102,6 +104,13 @@ public class MinidumpUploadCallable implements Callable<Integer> {
                 return UPLOAD_USER_DISABLED;
             }
 
+            if (!mPermManager.isClientInMetricsSample()) {
+                Log.i(TAG, "Minidump upload skipped due to sampling.  Marking file as uploaded for "
+                                + "cleanup to prevent future uploads.");
+                cleanupMinidumpFile();
+                return UPLOAD_DISABLED_BY_SAMPLING;
+            }
+
             boolean isLimited = mPermManager.isUploadLimited();
             if (isLimited || !mPermManager.isUploadPermitted()) {
                 Log.i(TAG, "Minidump cannot currently be uploaded due to constraints.");

chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunGlueImpl.java

@@ -8,6 +8,7 @@ import android.app.Fragment;
 import android.content.Context;
 import android.text.TextUtils;
 
+import org.chromium.chrome.browser.metrics.UmaSessionStats;
 import org.chromium.chrome.browser.preferences.PrefServiceBridge;
 import org.chromium.chrome.browser.preferences.privacy.PrivacyPreferencesManager;
 import org.chromium.chrome.browser.signin.AccountAdder;
@@ -32,7 +33,7 @@ public class FirstRunGlueImpl implements FirstRunGlue {
 
     @Override
     public void acceptTermsOfService(Context appContext, boolean allowCrashUpload) {
-        PrivacyPreferencesManager.getInstance().initCrashUploadPreference(allowCrashUpload);
+        UmaSessionStats.changeMetricsReportingConsent(allowCrashUpload);
         PrefServiceBridge.getInstance().setEulaAccepted();
     }
 

chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaSessionStats.java

@@ -155,15 +155,37 @@ public class UmaSessionStats implements NetworkChangeNotifier.ConnectionTypeObse
     }
 
     /**
-     * Updates the state of the MetricsService to account for the user's preferences.
+     * Updates the metrics services based on a change of consent. This can happen during first-run
+     * flow, and when the user changes their preferences.
      */
-    public void updateMetricsServiceState() {
-        boolean mayRecordStats = !PrivacyPreferencesManager.getInstance()
-                .isNeverUploadCrashDump();
-        boolean mayUploadStats = mReportingPermissionManager.isUmaUploadPermitted();
+    public static void changeMetricsReportingConsent(boolean consent) {
+        PrivacyPreferencesManager privacyManager = PrivacyPreferencesManager.getInstance();
+        // Update the new two-choice Android preference.
+        privacyManager.setUsageAndCrashReporting(consent);
+        // Update the old three-choice Android preference, and synchronize with Chrome local state
+        // preferences. Note, this forces the three-choice preferences to be either never upload,
+        // or only upload on WiFi.
+        privacyManager.initCrashUploadPreference(consent);
+
+        // Perform native changes needed to reflect the new consent value.
+        nativeChangeMetricsReportingConsent(consent);
 
-        // Re-start the MetricsService with the given parameters.
-        nativeUpdateMetricsServiceState(mayRecordStats, mayUploadStats);
+        updateMetricsServiceState();
+    }
+
+    /**
+     * Updates the state of MetricsService to account for the user's preferences.
+     */
+    public static void updateMetricsServiceState() {
+        PrivacyPreferencesManager privacyManager = PrivacyPreferencesManager.getInstance();
+
+        // Ensure Android and Chrome local state prefs are in sync.
+        privacyManager.syncUsageAndCrashReportingPrefs();
+
+        boolean mayUploadStats = privacyManager.isUmaUploadPermitted();
+
+        // Re-start the MetricsService with the given parameter, and current consent.
+        nativeUpdateMetricsServiceState(mayUploadStats);
     }
 
     /**
@@ -194,6 +216,10 @@ public class UmaSessionStats implements NetworkChangeNotifier.ConnectionTypeObse
             prefManager.setUsageAndCrashReporting(prefBridge.isMetricsReportingEnabled());
         }
 
+        // Update the metrics sampling state so it's available before the native feature list is
+        // available.
+        prefManager.setClientInMetricsSample(UmaUtils.isClientInMetricsReportingSample());
+
         // Make sure preferences are in sync.
         prefManager.syncUsageAndCrashReportingPrefs();
     }
@@ -212,7 +238,8 @@ public class UmaSessionStats implements NetworkChangeNotifier.ConnectionTypeObse
     }
 
     private static native long nativeInit();
-    private native void nativeUpdateMetricsServiceState(boolean mayRecord, boolean mayUpload);
+    private static native void nativeChangeMetricsReportingConsent(boolean consent);
+    private static native void nativeUpdateMetricsServiceState(boolean mayUpload);
     private native void nativeUmaResumeSession(long nativeUmaSessionStats);
     private native void nativeUmaEndSession(long nativeUmaSessionStats);
     private static native void nativeLogRendererCrash();

chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaUtils.java

@@ -54,6 +54,14 @@ public class UmaUtils {
         sRunningApplicationStart = isAppStart;
     }
 
+    /**
+     * Determines if this client is eligible to send metrics and crashes based on sampling. If it
+     * is, and there was user consent, then metrics and crashes would be reported
+     */
+    public static boolean isClientInMetricsReportingSample() {
+        return nativeIsClientInMetricsReportingSample();
+    }
+
     /**
      * Sets whether metrics reporting was opt-in or not. If it was opt-in, then the enable checkbox
      * on first-run was default unchecked. If it was opt-out, then the checkbox was default checked.
@@ -73,5 +81,6 @@ public class UmaUtils {
         return sForegroundStartTimeMs;
     }
 
+    private static native boolean nativeIsClientInMetricsReportingSample();
     private static native void nativeRecordMetricsReportingDefaultOptIn(boolean optIn);
 }

chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/CrashReportingPermissionManager.java

@@ -8,6 +8,14 @@ package org.chromium.chrome.browser.preferences.privacy;
  * Interface for crash reporting permissions.
  */
 public interface CrashReportingPermissionManager {
+    /**
+     * Checks whether this client is in-sample for crashes. See
+     * {@link org.chromium.chrome.browser.metrics.UmaUtils#isClientInMetricsSample} for details.
+     *
+     * @returns boolean Whether client is in-sample.
+     */
+    public boolean isClientInMetricsSample();
+
     /**
      * Check whether to allow uploading crash dump now based on user consent and connectivity.
      *

chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManager.java

@@ -26,6 +26,7 @@ public class PrivacyPreferencesManager implements CrashReportingPermissionManage
     static final String PREF_CRASH_DUMP_UPLOAD = "crash_dump_upload";
     static final String PREF_CRASH_DUMP_UPLOAD_NO_CELLULAR = "crash_dump_upload_no_cellular";
     static final String PREF_METRICS_REPORTING = "metrics_reporting";
+    private static final String PREF_METRICS_IN_SAMPLE = "in_metrics_sample";
     private static final String PREF_NETWORK_PREDICTIONS = "network_predictions";
     private static final String PREF_BANDWIDTH_OLD = "prefetch_bandwidth";
     private static final String PREF_BANDWIDTH_NO_CELLULAR_OLD = "prefetch_bandwidth_no_cellular";
@@ -276,6 +277,27 @@ public class PrivacyPreferencesManager implements CrashReportingPermissionManage
         return mSharedPreferences.getBoolean(PREF_CELLULAR_EXPERIMENT, false);
     }
 
+    /**
+     * Sets whether this client is in-sample. See
+     * {@link org.chromium.chrome.browser.metrics.UmaUtils#isClientInMetricsSample} for details.
+     */
+    public void setClientInMetricsSample(boolean inSample) {
+        mSharedPreferences.edit().putBoolean(PREF_METRICS_IN_SAMPLE, inSample).apply();
+    }
+
+    /**
+     * Checks whether this client is in-sample. See
+     * {@link org.chromium.chrome.browser.metrics.UmaUtils#isClientInMetricsSample} for details.
+     *
+     * @returns boolean Whether client is in-sample.
+     */
+    public boolean isClientInMetricsSample() {
+        // The default value is true to avoid sampling out crashes that occur before native code has
+        // been initialized on first run. We'd rather have some extra crashes than none from that
+        // time.
+        return mSharedPreferences.getBoolean(PREF_METRICS_IN_SAMPLE, true);
+    }
+
     /**
      * Sets the crash upload preference, which determines whether crash dumps will be uploaded
      * always, never, or only on wifi.
@@ -325,8 +347,7 @@ public class PrivacyPreferencesManager implements CrashReportingPermissionManage
     }
 
     /**
-     * Sets the initial value for whether crash stacks may be uploaded.
-     * This should be called only once, the first time Chrome is launched.
+     * Sets the value for whether crash stacks may be uploaded.
      */
     public void initCrashUploadPreference(boolean allowCrashUpload) {
         SharedPreferences.Editor ed = mSharedPreferences.edit();

chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/UsageAndCrashReportsPreferenceFragment.java

@@ -10,6 +10,7 @@ import android.preference.Preference.OnPreferenceChangeListener;
 import android.preference.PreferenceFragment;
 
 import org.chromium.chrome.R;
+import org.chromium.chrome.browser.metrics.UmaSessionStats;
 import org.chromium.chrome.browser.preferences.ChromeSwitchPreference;
 import org.chromium.chrome.browser.preferences.ManagedPreferenceDelegate;
 import org.chromium.chrome.browser.preferences.PrefServiceBridge;
@@ -39,16 +40,7 @@ public class UsageAndCrashReportsPreferenceFragment extends PreferenceFragment {
         usageAndCrashReportsSwitch.setOnPreferenceChangeListener(new OnPreferenceChangeListener() {
             @Override
             public boolean onPreferenceChange(Preference preference, Object newValue) {
-                boolean enabled = (boolean) newValue;
-                PrivacyPreferencesManager privacyManager = PrivacyPreferencesManager.getInstance();
-
-                // Update new two-choice android and chromium preferences.
-                PrefServiceBridge.getInstance().setMetricsReportingEnabled(enabled);
-                privacyManager.setUsageAndCrashReporting(enabled);
-
-                // Update old three-choice android and chromium preference.
-                PrefServiceBridge.getInstance().setCrashReportingEnabled(enabled);
-                privacyManager.initCrashUploadPreference(enabled);
+                UmaSessionStats.changeMetricsReportingConsent((boolean) newValue);
                 return true;
             }
         });

chrome/android/javatests/src/org/chromium/chrome/browser/crash/MinidumpUploadCallableTest.java

@@ -113,19 +113,18 @@ public class MinidumpUploadCallableTest extends CrashTestCase {
 
     private static class MockCrashReportingPermissionManager
             implements CrashReportingPermissionManager {
-        private final boolean mIsPermitted;
-        private final boolean mIsUserPermitted;
-        private final boolean mIsCommandLineDisabled;
-        private final boolean mIsLimited;
-        private final boolean mIsEnabledForTests;
-
-        MockCrashReportingPermissionManager(boolean isPermitted, boolean isUserPermitted,
-                boolean isCommandLineDisabled, boolean isLimited, boolean isEnabledForTests) {
-            mIsPermitted = isPermitted;
-            mIsUserPermitted = isUserPermitted;
-            mIsCommandLineDisabled = isCommandLineDisabled;
-            mIsLimited = isLimited;
-            mIsEnabledForTests = isEnabledForTests;
+        protected boolean mIsInSample;
+        protected boolean mIsPermitted;
+        protected boolean mIsUserPermitted;
+        protected boolean mIsCommandLineDisabled;
+        protected boolean mIsLimited;
+        protected boolean mIsEnabledForTests;
+
+        MockCrashReportingPermissionManager() {}
+
+        @Override
+        public boolean isClientInMetricsSample() {
+            return mIsInSample;
         }
 
         @Override
@@ -200,7 +199,16 @@ public class MinidumpUploadCallableTest extends CrashTestCase {
     @Feature({"Android-AppBase"})
     public void testCallWhenCurrentlyPermitted() throws Exception {
         CrashReportingPermissionManager testPermManager =
-                new MockCrashReportingPermissionManager(true, true, false, false, false);
+                new MockCrashReportingPermissionManager() {
+                    {
+                        mIsInSample = true;
+                        mIsPermitted = true;
+                        mIsUserPermitted = true;
+                        mIsCommandLineDisabled = false;
+                        mIsLimited = false;
+                        mIsEnabledForTests = false;
+                    }
+                };
 
         HttpURLConnectionFactory httpURLConnectionFactory = new TestHttpURLConnectionFactory();
 
@@ -216,7 +224,16 @@ public class MinidumpUploadCallableTest extends CrashTestCase {
     @Feature({"Android-AppBase"})
     public void testCallNotPermittedByUser() throws Exception {
         CrashReportingPermissionManager testPermManager =
-                new MockCrashReportingPermissionManager(false, false, false, false, false);
+                new MockCrashReportingPermissionManager() {
+                    {
+                        mIsInSample = true;
+                        mIsPermitted = false;
+                        mIsUserPermitted = false;
+                        mIsCommandLineDisabled = false;
+                        mIsLimited = false;
+                        mIsEnabledForTests = false;
+                    }
+                };
 
         HttpURLConnectionFactory httpURLConnectionFactory = new FailHttpURLConnectionFactory();
 
@@ -231,7 +248,16 @@ public class MinidumpUploadCallableTest extends CrashTestCase {
     @Feature({"Android-AppBase"})
     public void testCallNotPermittedByCommandLine() throws Exception {
         CrashReportingPermissionManager testPermManager =
-                new MockCrashReportingPermissionManager(true, true, true, false, false);
+                new MockCrashReportingPermissionManager() {
+                    {
+                        mIsInSample = true;
+                        mIsPermitted = true;
+                        mIsUserPermitted = true;
+                        mIsCommandLineDisabled = true;
+                        mIsLimited = false;
+                        mIsEnabledForTests = false;
+                    }
+                };
 
         HttpURLConnectionFactory httpURLConnectionFactory = new FailHttpURLConnectionFactory();
 
@@ -242,11 +268,44 @@ public class MinidumpUploadCallableTest extends CrashTestCase {
         assertFalse(mExpectedFileAfterUpload.exists());
     }
 
+    @SmallTest
+    @Feature({"Android-AppBase"})
+    public void testCallPermittedButNotInSample() throws Exception {
+        CrashReportingPermissionManager testPermManager =
+                new MockCrashReportingPermissionManager() {
+                    {
+                        mIsInSample = false;
+                        mIsPermitted = true;
+                        mIsUserPermitted = true;
+                        mIsCommandLineDisabled = false;
+                        mIsLimited = false;
+                        mIsEnabledForTests = false;
+                    }
+                };
+
+        HttpURLConnectionFactory httpURLConnectionFactory = new TestHttpURLConnectionFactory();
+
+        MinidumpUploadCallable minidumpUploadCallable =
+                new MockMinidumpUploadCallable(httpURLConnectionFactory, testPermManager);
+        assertEquals(MinidumpUploadCallable.UPLOAD_DISABLED_BY_SAMPLING,
+                minidumpUploadCallable.call().intValue());
+        assertTrue(mExpectedFileAfterUpload.exists());
+    }
+
     @SmallTest
     @Feature({"Android-AppBase"})
     public void testCallPermittedButNotUnderCurrentCircumstances() throws Exception {
         CrashReportingPermissionManager testPermManager =
-                new MockCrashReportingPermissionManager(false, true, false, false, false);
+                new MockCrashReportingPermissionManager() {
+                    {
+                        mIsInSample = true;
+                        mIsPermitted = false;
+                        mIsUserPermitted = true;
+                        mIsCommandLineDisabled = false;
+                        mIsLimited = false;
+                        mIsEnabledForTests = false;
+                    }
+                };
 
         HttpURLConnectionFactory httpURLConnectionFactory = new FailHttpURLConnectionFactory();
 
@@ -261,7 +320,16 @@ public class MinidumpUploadCallableTest extends CrashTestCase {
     @Feature({"Android-AppBase"})
     public void testCrashUploadConstrainted() throws Exception {
         CrashReportingPermissionManager testPermManager =
-                new MockCrashReportingPermissionManager(true, true, false, true, false);
+                new MockCrashReportingPermissionManager() {
+                    {
+                        mIsInSample = true;
+                        mIsPermitted = true;
+                        mIsUserPermitted = true;
+                        mIsCommandLineDisabled = false;
+                        mIsLimited = true;
+                        mIsEnabledForTests = false;
+                    }
+                };
 
         HttpURLConnectionFactory httpURLConnectionFactory = new TestHttpURLConnectionFactory();
 
@@ -276,7 +344,16 @@ public class MinidumpUploadCallableTest extends CrashTestCase {
     @Feature({"Android-AppBase"})
     public void testCrashUploadEnabledForTestsDespiteConstraints() throws Exception {
         CrashReportingPermissionManager testPermManager =
-                new MockCrashReportingPermissionManager(false, false, false, true, true);
+                new MockCrashReportingPermissionManager() {
+                    {
+                        mIsInSample = true;
+                        mIsPermitted = false;
+                        mIsUserPermitted = false;
+                        mIsCommandLineDisabled = false;
+                        mIsLimited = true;
+                        mIsEnabledForTests = true;
+                    }
+                };
 
         HttpURLConnectionFactory httpURLConnectionFactory = new TestHttpURLConnectionFactory();
 

chrome/browser/android/metrics/uma_session_stats.cc

@@ -93,34 +93,46 @@ void UmaSessionStats::RegisterSyntheticMultiGroupFieldTrial(
       trial_name, group_name_hashes);
 }
 
-// Starts/stops the MetricsService when permissions have changed.
+// Updates metrics reporting state managed by native code. This should only be
+// called when consent is changing, and UpdateMetricsServiceState() should be
+// called immediately after for metrics services to be started or stopped as
+// needed. This is enforced by UmaSessionStats.changeMetricsReportingConsent on
+// the Java side.
+static void ChangeMetricsReportingConsent(JNIEnv*,
+                                          const JavaParamRef<jclass>&,
+                                          jboolean consent) {
+  UpdateMetricsPrefsOnPermissionChange(consent);
+
+  // This function ensures a consent file in the data directory is either
+  // created, or deleted, depending on consent. Starting up metrics services
+  // will ensure that the consent file contains the ClientID. The ID is passed
+  // to the renderer for crash reporting when things go wrong.
+  content::BrowserThread::GetBlockingPool()->PostTask(
+      FROM_HERE, base::Bind(base::IgnoreResult(
+                                GoogleUpdateSettings::SetCollectStatsConsent),
+                            consent));
+}
+
+// Starts/stops the MetricsService based on existing consent and upload
+// preferences.
 // There are three possible states:
 // * Logs are being recorded and being uploaded to the server.
 // * Logs are being recorded, but not being uploaded to the server.
 //   This happens when we've got permission to upload on Wi-Fi but we're on a
 //   mobile connection (for example).
 // * Logs are neither being recorded or uploaded.
-static void UpdateMetricsServiceState(JNIEnv* env,
-                                      const JavaParamRef<jobject>& obj,
-                                      jboolean may_record,
+// If logs aren't being recorded, then |may_upload| is ignored.
+//
+// This can be called at any time when consent hasn't changed, such as
+// connection type change, or start up. If consent has changed, then
+// ChangeMetricsReportingConsent() should be called first.
+static void UpdateMetricsServiceState(JNIEnv*,
+                                      const JavaParamRef<jclass>&,
                                       jboolean may_upload) {
-  metrics::MetricsService* metrics = g_browser_process->metrics_service();
-  DCHECK(metrics);
-
-  if (metrics->recording_active() != may_record) {
-    UpdateMetricsPrefsOnPermissionChange(may_record);
-
-    // This function puts a consent file with the ClientID in the
-    // data directory. The ID is passed to the renderer for crash
-    // reporting when things go wrong.
-    content::BrowserThread::GetBlockingPool()->PostTask(FROM_HERE,
-        base::Bind(
-            base::IgnoreResult(GoogleUpdateSettings::SetCollectStatsConsent),
-            may_record));
-  }
-
-  g_browser_process->GetMetricsServicesManager()->UpdatePermissions(
-      may_record, may_upload);
+  // This will also apply the consent state, taken from Chrome Local State
+  // prefs.
+  g_browser_process->GetMetricsServicesManager()->UpdateUploadPermissions(
+      may_upload);
 }
 
 // Renderer process crashed in the foreground.

chrome/browser/android/metrics/uma_utils.cc

@@ -7,6 +7,7 @@
 #include <stdint.h>
 
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/metrics/chrome_metrics_services_manager_client.h"
 #include "components/metrics/metrics_reporting_default_state.h"
 #include "jni/UmaUtils_jni.h"
 
@@ -24,6 +25,12 @@ base::Time GetMainEntryPointTime() {
          base::TimeDelta::FromMilliseconds(startTimeUnixMs);
 }
 
+static jboolean IsClientInMetricsReportingSample(
+    JNIEnv* env,
+    const JavaParamRef<jclass>& obj) {
+  return ChromeMetricsServicesManagerClient::IsClientInSample();
+}
+
 static void RecordMetricsReportingDefaultOptIn(JNIEnv* env,
                                                const JavaParamRef<jclass>& obj,
                                                jboolean opt_in) {