AW UMA: fix force-enable switch

The --force-enable-metrics-reporting switch was previously implemented
to override sampling, but didn't correctly override user consent. This
fixes that, and modifies the unittests to verify correct behavior by
checking the metrics service actual starts up.

To accommodate privacy concerns, this flag does not override app
consent. We separate user and app consent to plumb both values down, and
we add a test case to verify the flag respects app consent.

This also splits the previous test case into multiple cases, which is
necessary in order to initialize the client under two different sets of
conditions.

Bug: 994418
Test: run_android_webview_unittests --gtest_filter=AwMetricsServiceClientTest.*
Test: Opt-out of metrics in settings, AwMetricsLogUploader is called
Test: Opt-out WebView shell in manifest, AwMetricsLogUploader is not called
Change-Id: I1e177bd5e3a5622c694e2275f18408d946b69397
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1815545Reviewed-by: Tao Bai <michaelbai@chromium.org>
Commit-Queue: Nate Fischer <ntfschr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#702154}

android_webview/browser/aw_metrics_service_client.cc

@@ -149,8 +149,12 @@ void AwMetricsServiceClient::Initialize(PrefService* pref_service) {
 
 void AwMetricsServiceClient::MaybeStartMetrics() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  // Treat the debugging flag the same as user consent because the user set it,
+  // but keep app_consent_ separate so we never persist data from an opted-out
+  // app.
+  bool user_consent_or_flag = user_consent_ || IsMetricsReportingForceEnabled();
   if (init_finished_ && set_consent_finished_) {
-    if (user_and_app_consent_) {
+    if (app_consent_ && user_consent_or_flag) {
       metrics_service_ = CreateMetricsService(metrics_state_manager_.get(),
                                               this, pref_service_);
       metrics_state_manager_->ForceClientIdCreation();
@@ -166,10 +170,12 @@ void AwMetricsServiceClient::MaybeStartMetrics() {
   }
 }
 
-void AwMetricsServiceClient::SetHaveMetricsConsent(bool consent) {
+void AwMetricsServiceClient::SetHaveMetricsConsent(bool user_consent,
+                                                   bool app_consent) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   set_consent_finished_ = true;
-  user_and_app_consent_ = consent;
+  user_consent_ = user_consent;
+  app_consent_ = app_consent;
   MaybeStartMetrics();
 }
 
@@ -181,11 +187,13 @@ AwMetricsServiceClient::CreateLowEntropyProvider() {
 
 bool AwMetricsServiceClient::IsConsentGiven() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  return user_and_app_consent_;
+  return user_consent_ && app_consent_;
 }
 
 bool AwMetricsServiceClient::IsReportingEnabled() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  if (!app_consent_)
+    return false;
   return IsMetricsReportingForceEnabled() ||
          (EnabledStateProvider::IsReportingEnabled() && is_in_sample_);
 }
@@ -264,10 +272,11 @@ bool AwMetricsServiceClient::IsInSample() {
 }
 
 // static
-void JNI_AwMetricsServiceClient_SetHaveMetricsConsent(
-    JNIEnv* env,
-    jboolean consent) {
-  AwMetricsServiceClient::GetInstance()->SetHaveMetricsConsent(consent);
+void JNI_AwMetricsServiceClient_SetHaveMetricsConsent(JNIEnv* env,
+                                                      jboolean user_consent,
+                                                      jboolean app_consent) {
+  AwMetricsServiceClient::GetInstance()->SetHaveMetricsConsent(user_consent,
+                                                               app_consent);
 }
 
 }  // namespace android_webview

android_webview/browser/aw_metrics_service_client.h

@@ -31,8 +31,8 @@ namespace android_webview {
 //  - The user has not opted out (controlled by GMS).
 //  - The app has not opted out (controlled by manifest tag).
 //  - This client is in the 2% sample (controlled by client ID hash).
-// The first two are recorded in |user_and_app_consent_|, which is set by
-// SetHaveMetricsConsent(). The last is recorded in |is_in_sample_|.
+// The first two are recorded in |user_consent_| and |app_consent_|, which are
+// set by SetHaveMetricsConsent(). The last is recorded in |is_in_sample_|.
 //
 // Metrics are pseudonymously identified by a randomly-generated "client ID".
 // WebView stores this in prefs, written to the app's data directory. There's a
@@ -89,7 +89,7 @@ class AwMetricsServiceClient : public metrics::MetricsServiceClient,
   ~AwMetricsServiceClient() override;
 
   void Initialize(PrefService* pref_service);
-  void SetHaveMetricsConsent(bool consent);
+  void SetHaveMetricsConsent(bool user_consent, bool app_consent);
   std::unique_ptr<const base::FieldTrial::EntropyProvider>
       CreateLowEntropyProvider();
 
@@ -127,7 +127,8 @@ class AwMetricsServiceClient : public metrics::MetricsServiceClient,
   PrefService* pref_service_ = nullptr;
   bool init_finished_ = false;
   bool set_consent_finished_ = false;
-  bool user_and_app_consent_ = false;
+  bool user_consent_ = false;
+  bool app_consent_ = false;
   bool is_in_sample_ = false;
 
   // AwMetricsServiceClient may be created before the UI thread is promoted to

android_webview/browser/aw_metrics_service_client_unittest.cc

@@ -78,7 +78,7 @@ class AwMetricsServiceClientTest : public testing::Test {
 TEST_F(AwMetricsServiceClientTest, TestSetConsentTrueBeforeInit) {
   auto prefs = CreateTestPrefs();
   auto client = std::make_unique<TestClient>();
-  client->SetHaveMetricsConsent(true);
+  client->SetHaveMetricsConsent(true, true);
   client->Initialize(prefs.get());
   ASSERT_TRUE(client->IsRecordingActive());
   ASSERT_TRUE(prefs->HasPrefPath(metrics::prefs::kMetricsClientID));
@@ -87,7 +87,7 @@ TEST_F(AwMetricsServiceClientTest, TestSetConsentTrueBeforeInit) {
 TEST_F(AwMetricsServiceClientTest, TestSetConsentFalseBeforeInit) {
   auto prefs = CreateTestPrefs();
   auto client = std::make_unique<TestClient>();
-  client->SetHaveMetricsConsent(false);
+  client->SetHaveMetricsConsent(false, false);
   client->Initialize(prefs.get());
   ASSERT_FALSE(client->IsRecordingActive());
   ASSERT_FALSE(prefs->HasPrefPath(metrics::prefs::kMetricsClientID));
@@ -96,7 +96,7 @@ TEST_F(AwMetricsServiceClientTest, TestSetConsentFalseBeforeInit) {
 TEST_F(AwMetricsServiceClientTest, TestSetConsentTrueAfterInit) {
   auto prefs = CreateTestPrefs();
   auto client = CreateAndInitTestClient(prefs.get());
-  client->SetHaveMetricsConsent(true);
+  client->SetHaveMetricsConsent(true, true);
   ASSERT_TRUE(client->IsRecordingActive());
   ASSERT_TRUE(prefs->HasPrefPath(metrics::prefs::kMetricsClientID));
 }
@@ -104,7 +104,7 @@ TEST_F(AwMetricsServiceClientTest, TestSetConsentTrueAfterInit) {
 TEST_F(AwMetricsServiceClientTest, TestSetConsentFalseAfterInit) {
   auto prefs = CreateTestPrefs();
   auto client = CreateAndInitTestClient(prefs.get());
-  client->SetHaveMetricsConsent(false);
+  client->SetHaveMetricsConsent(false, false);
   ASSERT_FALSE(client->IsRecordingActive());
   ASSERT_FALSE(prefs->HasPrefPath(metrics::prefs::kMetricsClientID));
 }
@@ -114,7 +114,7 @@ TEST_F(AwMetricsServiceClientTest, TestKeepExistingClientId) {
   auto prefs = CreateTestPrefs();
   prefs->SetString(metrics::prefs::kMetricsClientID, kTestClientId);
   auto client = CreateAndInitTestClient(prefs.get());
-  client->SetHaveMetricsConsent(true);
+  client->SetHaveMetricsConsent(true, true);
   ASSERT_TRUE(client->IsRecordingActive());
   ASSERT_TRUE(prefs->HasPrefPath(metrics::prefs::kMetricsClientID));
   ASSERT_EQ(kTestClientId, prefs->GetString(metrics::prefs::kMetricsClientID));
@@ -124,27 +124,59 @@ TEST_F(AwMetricsServiceClientTest, TestSetConsentFalseClearsClientId) {
   auto prefs = CreateTestPrefs();
   prefs->SetString(metrics::prefs::kMetricsClientID, kTestClientId);
   auto client = CreateAndInitTestClient(prefs.get());
-  client->SetHaveMetricsConsent(false);
+  client->SetHaveMetricsConsent(false, false);
   ASSERT_FALSE(client->IsRecordingActive());
   ASSERT_FALSE(prefs->HasPrefPath(metrics::prefs::kMetricsClientID));
 }
 
 TEST_F(AwMetricsServiceClientTest, TestCanForceEnableMetrics) {
-  auto prefs = CreateTestPrefs();
-  auto client = CreateAndInitTestClient(prefs.get());
-
   base::CommandLine::ForCurrentProcess()->AppendSwitch(
       metrics::switches::kForceEnableMetricsReporting);
 
-  // Flip everything to false; flag should have higher precedence.
-  client->SetHaveMetricsConsent(false);
+  auto prefs = CreateTestPrefs();
+  auto client = std::make_unique<TestClient>();
+
+  // Flag should have higher precedence than sampling or user consent (but not
+  // app consent, so we set that to 'true' for this case).
+  client->SetHaveMetricsConsent(false, /* app_consent */ true);
   client->SetInSample(false);
+  client->Initialize(prefs.get());
+
   ASSERT_TRUE(client->IsReportingEnabled());
+  ASSERT_TRUE(client->IsRecordingActive());
+}
+
+TEST_F(AwMetricsServiceClientTest, TestCanForceEnableMetricsIfAlreadyEnabled) {
+  base::CommandLine::ForCurrentProcess()->AppendSwitch(
+      metrics::switches::kForceEnableMetricsReporting);
+
+  auto prefs = CreateTestPrefs();
+  auto client = std::make_unique<TestClient>();
 
-  // Flip things to true just to double-check everything still works.
-  client->SetHaveMetricsConsent(true);
+  // This is a sanity check: flip consent and sampling to true, just to make
+  // sure the flag continues to work.
+  client->SetHaveMetricsConsent(true, true);
   client->SetInSample(true);
+  client->Initialize(prefs.get());
+
   ASSERT_TRUE(client->IsReportingEnabled());
+  ASSERT_TRUE(client->IsRecordingActive());
+}
+
+TEST_F(AwMetricsServiceClientTest, TestCannotForceEnableMetricsIfAppOptsOut) {
+  base::CommandLine::ForCurrentProcess()->AppendSwitch(
+      metrics::switches::kForceEnableMetricsReporting);
+
+  auto prefs = CreateTestPrefs();
+  auto client = std::make_unique<TestClient>();
+
+  // Even with the flag, app consent should be respected.
+  client->SetHaveMetricsConsent(true, /* app_consent */ false);
+  client->SetInSample(true);
+  client->Initialize(prefs.get());
+
+  ASSERT_FALSE(client->IsReportingEnabled());
+  ASSERT_FALSE(client->IsRecordingActive());
 }
 
 }  // namespace android_webview

android_webview/java/src/org/chromium/android_webview/AwMetricsServiceClient.java

@@ -57,7 +57,7 @@ public class AwMetricsServiceClient {
 
     public static void setConsentSetting(Context ctx, boolean userConsent) {
         ThreadUtils.assertOnUiThread();
-        AwMetricsServiceClientJni.get().setHaveMetricsConsent(userConsent && !isAppOptedOut(ctx));
+        AwMetricsServiceClientJni.get().setHaveMetricsConsent(userConsent, !isAppOptedOut(ctx));
     }
 
     @CalledByNative
@@ -68,6 +68,6 @@ public class AwMetricsServiceClient {
 
     @NativeMethods
     interface Natives {
-        void setHaveMetricsConsent(boolean enabled);
+        void setHaveMetricsConsent(boolean userConsent, boolean appConsent);
     }
 }