wv/wl: makes AndroidMetricsServiceClient clean up state

If consent is not given, then the code paths are never
entered that delete state.

BUG=1164104
TEST=AndroidMetricsServiceClientTest*

Change-Id: Iaf5a750888b3e0727cad41e02ba10150f2d4c0f5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2617238Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Michael Bai <michaelbai@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#841709}

components/embedder_support/android/metrics/android_metrics_service_client.cc

@@ -147,8 +147,8 @@ std::unique_ptr<metrics::FileMetricsProvider> CreateFileMetricsProvider(
     PrefService* pref_service,
     bool metrics_reporting_enabled) {
   // Create an object to monitor files of metrics and include them in reports.
-  std::unique_ptr<metrics::FileMetricsProvider> file_metrics_provider(
-      new metrics::FileMetricsProvider(pref_service));
+  std::unique_ptr<metrics::FileMetricsProvider> file_metrics_provider =
+      std::make_unique<metrics::FileMetricsProvider>(pref_service);
 
   base::FilePath user_data_dir;
   base::PathService::Get(base::DIR_ANDROID_APP_DATA, &user_data_dir);
@@ -264,34 +264,46 @@ void AndroidMetricsServiceClient::Initialize(PrefService* pref_service) {
 // TODO:(crbug.com/1148351) Make the initialization consistent with Chrome.
 void AndroidMetricsServiceClient::MaybeStartMetrics() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  if (!IsConsentDetermined())
+    return;
+
+#if DCHECK_IS_ON()
+  // This function should be called only once after consent has been determined.
+  DCHECK(!did_start_metrics_with_consent_);
+  did_start_metrics_with_consent_ = true;
+#endif
+
   // 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 (IsConsentDetermined()) {
-    if (app_consent_ && user_consent_or_flag) {
-      did_start_metrics_ = true;
-      // Make GetSampleBucketValue() work properly.
-      metrics_state_manager_->ForceClientIdCreation();
-      is_client_id_forced_ = true;
-      RegisterMetricsProvidersAndInitState();
-      // Register for notifications so we can detect when the user or app are
-      // interacting with the embedder. We use these as signals to wake up the
-      // MetricsService.
-      RegisterForNotifications();
-      OnMetricsStart();
-
-      if (IsReportingEnabled()) {
-        // We assume the embedder has no shutdown sequence, so there's no need
-        // for a matching Stop() call.
-        metrics_service_->Start();
-      }
-
-      CreateUkmService();
-    } else {
-      OnMetricsNotStarted();
-      pref_service_->ClearPref(prefs::kMetricsClientID);
+  if (app_consent_ && user_consent_or_flag) {
+    did_start_metrics_ = true;
+    // Make GetSampleBucketValue() work properly.
+    metrics_state_manager_->ForceClientIdCreation();
+    is_client_id_forced_ = true;
+    RegisterMetricsProvidersAndInitState();
+    // Register for notifications so we can detect when the user or app are
+    // interacting with the embedder. We use these as signals to wake up the
+    // MetricsService.
+    RegisterForNotifications();
+    OnMetricsStart();
+
+    if (IsReportingEnabled()) {
+      // We assume the embedder has no shutdown sequence, so there's no need
+      // for a matching Stop() call.
+      metrics_service_->Start();
     }
+
+    CreateUkmService();
+  } else {
+    // Even though reporting is not enabled, CreateFileMetricsProvider() is
+    // called. This ensures on disk state is removed.
+    metrics_service_->RegisterMetricsProvider(CreateFileMetricsProvider(
+        pref_service_, /* metrics_reporting_enabled */ false));
+    OnMetricsNotStarted();
+    pref_service_->ClearPref(prefs::kMetricsClientID);
   }
 }
 

components/embedder_support/android/metrics/android_metrics_service_client.h

@@ -263,6 +263,10 @@ class AndroidMetricsServiceClient : public MetricsServiceClient,
   base::OnceClosure collect_final_metrics_for_log_closure_;
   base::RepeatingClosure on_final_metrics_collected_listener_;
 
+#if DCHECK_IS_ON()
+  bool did_start_metrics_with_consent_ = false;
+#endif
+
   // MetricsServiceClient may be created before the UI thread is promoted to
   // BrowserThread::UI. Use |sequence_checker_| to enforce that the
   // MetricsServiceClient is used on a single thread.

components/embedder_support/android/metrics/android_metrics_service_client_unittest.cc

@@ -402,6 +402,30 @@ TEST_F(AndroidMetricsServiceClientTest,
   EXPECT_FALSE(base::PathExists(upload_dir));
 }
 
+TEST_F(AndroidMetricsServiceClientTest,
+       TestBrowserMetricsDirClearedIfNoConsent) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeatureWithParameters(
+      base::kPersistentHistogramsFeature, {{"storage", "MappedFile"}});
+
+  base::FilePath metrics_dir;
+  ASSERT_TRUE(base::PathService::Get(base::DIR_ANDROID_APP_DATA, &metrics_dir));
+  InstantiatePersistentHistograms(metrics_dir);
+  base::FilePath upload_dir = metrics_dir.AppendASCII(kBrowserMetricsName);
+  ASSERT_TRUE(base::PathExists(upload_dir));
+
+  auto prefs = CreateTestPrefs();
+  auto client = std::make_unique<TestClient>();
+
+  // Setup the client isn't in sample.
+  client->SetHaveMetricsConsent(/* user_consent= */ false,
+                                /* app_consent= */ false);
+  client->Initialize(prefs.get());
+  task_environment()->RunUntilIdle();
+
+  EXPECT_FALSE(base::PathExists(upload_dir));
+}
+
 TEST_F(AndroidMetricsServiceClientTest,
        TestBrowserMetricsDirExistsIfReportingEnabled) {
   base::test::ScopedFeatureList scoped_feature_list;