[Merge 116] Add synthetic field trial to track when SB database has b…

…een migrated

This will allow filtering the experiment data by whether the database
was migrated or not, which should help when analyzing the data.

(cherry picked from commit 03c2d24)

Bug: 1380507
Change-Id: I81acb55c5ea6916d21a82ff29776768597907cc4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4754735
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Reviewed-by: Chris Thompson <cthomp@chromium.org>
Reviewed-by: Weilun Shi <sweilun@chromium.org>
Reviewed-by: Daniel Rubery <drubery@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1181209}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4775459
Reviewed-by: Jesse Doherty <jwd@chromium.org>
Commit-Queue: Jesse Doherty <jwd@chromium.org>
Auto-Submit: Clark DuVall <cduvall@chromium.org>
Cr-Commit-Position: refs/branch-heads/5845@{#1461}
Cr-Branched-From: 5a5dff6-refs/heads/main@{#1160321}

chrome/browser/metrics/chrome_metrics_service_accessor.h

@@ -59,6 +59,7 @@ class ChromeCleanerControllerDelegate;
 class ChromeSafeBrowsingUIManagerDelegate;
 class DownloadUrlSBClient;
 class IncidentReportingService;
+class ServicesDelegateDesktop;
 
 namespace internal {
 class ReporterRunner;
@@ -132,6 +133,7 @@ class ChromeMetricsServiceAccessor : public metrics::MetricsServiceAccessor {
   friend class safe_browsing::ChromeSafeBrowsingUIManagerDelegate;
   friend class safe_browsing::DownloadUrlSBClient;
   friend class safe_browsing::IncidentReportingService;
+  friend class safe_browsing::ServicesDelegateDesktop;
   friend class safe_browsing::internal::ReporterRunner;
   friend class segmentation_platform::FieldTrialRegisterImpl;
   friend class ChromeMetricsServiceClient;

chrome/browser/safe_browsing/BUILD.gn

@@ -404,6 +404,7 @@ static_library("safe_browsing") {
         "//components/prefs",
         "//components/safe_browsing/core/browser:verdict_cache_manager",
         "//components/safe_browsing/core/browser/db",
+        "//components/safe_browsing/core/browser/db:v4_store",
         "//components/safe_browsing/core/browser/hashprefix_realtime:hash_realtime_service",
         "//components/safe_browsing/core/browser/hashprefix_realtime:hash_realtime_utils",
         "//components/safe_browsing/core/browser/realtime:policy_engine",

chrome/browser/safe_browsing/services_delegate_desktop.cc

@@ -10,6 +10,7 @@
 #include "base/functional/bind.h"
 #include "base/memory/ptr_util.h"
 #include "base/strings/string_util.h"
+#include "chrome/browser/metrics/chrome_metrics_service_accessor.h"
 #include "chrome/browser/safe_browsing/download_protection/download_protection_service.h"
 #include "chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.h"
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
@@ -22,6 +23,22 @@
 #include "services/preferences/public/mojom/tracked_preference_validation_delegate.mojom.h"
 
 namespace safe_browsing {
+namespace {
+
+const char* MigrateResultToString(HashPrefixMap::MigrateResult result) {
+  switch (result) {
+    case HashPrefixMap::MigrateResult::kUnknown:
+      return "Unknown";
+    case HashPrefixMap::MigrateResult::kSuccess:
+      return "Success";
+    case HashPrefixMap::MigrateResult::kFailure:
+      return "Failure";
+    case HashPrefixMap::MigrateResult::kNotNeeded:
+      return "NotNeeded";
+  }
+}
+
+}  // namespace
 
 // static
 std::unique_ptr<ServicesDelegate> ServicesDelegate::Create(
@@ -134,7 +151,8 @@ ServicesDelegateDesktop::CreateDatabaseManager() {
       content::GetUIThreadTaskRunner({}), content::GetIOThreadTaskRunner({}),
       base::BindRepeating(
           &ServicesDelegateDesktop::GetEstimatedExtendedReportingLevel,
-          base::Unretained(this)));
+          base::Unretained(this)),
+      base::BindOnce(&UpdateSyntheticFieldTrial));
 }
 
 DownloadProtectionService*
@@ -161,4 +179,12 @@ void ServicesDelegateDesktop::OnProfileWillBeDestroyed(Profile* profile) {
   download_service_->RemovePendingDownloadRequests(profile);
 }
 
+// static
+void ServicesDelegateDesktop::UpdateSyntheticFieldTrial(
+    HashPrefixMap::MigrateResult result) {
+  ChromeMetricsServiceAccessor::RegisterSyntheticFieldTrial(
+      "SafeBrowsingMigrateResult", MigrateResultToString(result),
+      variations::SyntheticTrialAnnotationMode::kCurrentLog);
+}
+
 }  // namespace safe_browsing

chrome/browser/safe_browsing/services_delegate_desktop.h

@@ -8,6 +8,7 @@
 #include <memory>
 
 #include "chrome/browser/safe_browsing/services_delegate.h"
+#include "components/safe_browsing/core/browser/db/hash_prefix_map.h"
 #include "components/safe_browsing/core/common/safe_browsing_prefs.h"
 
 namespace safe_browsing {
@@ -64,6 +65,8 @@ class ServicesDelegateDesktop : public ServicesDelegate {
   DownloadProtectionService* CreateDownloadProtectionService();
   IncidentReportingService* CreateIncidentReportingService();
 
+  static void UpdateSyntheticFieldTrial(HashPrefixMap::MigrateResult result);
+
   std::unique_ptr<DownloadProtectionService> download_service_;
   std::unique_ptr<IncidentReportingService> incident_service_;
 

components/safe_browsing/core/browser/db/hash_prefix_map.h

@@ -126,7 +126,7 @@ class HashPrefixMap {
   virtual HashPrefixStr GetMatchingHashPrefix(base::StringPiece full_hash) = 0;
 
   // Migrates the file format between the different types of HashPrefixMap.
-  enum class MigrateResult { kSuccess, kFailure, kNotNeeded };
+  enum class MigrateResult { kUnknown, kSuccess, kFailure, kNotNeeded };
   virtual MigrateResult MigrateFileFormat(const base::FilePath& store_path,
                                           V4StoreFileFormat* file_format) = 0;
 

components/safe_browsing/core/browser/db/v4_database.cc

@@ -388,6 +388,24 @@ void V4Database::RecordFileSizeHistograms() {
                              50);
 }
 
+HashPrefixMap::MigrateResult V4Database::GetMigrateResult() {
+  HashPrefixMap::MigrateResult final_result =
+      HashPrefixMap::MigrateResult::kUnknown;
+  for (const auto& store_map_iter : *store_map_) {
+    auto result = store_map_iter.second->migrate_result();
+    if (result == HashPrefixMap::MigrateResult::kFailure) {
+      return result;
+    }
+
+    if (final_result == HashPrefixMap::MigrateResult::kUnknown) {
+      final_result = result;
+    } else if (result != final_result) {
+      return HashPrefixMap::MigrateResult::kUnknown;
+    }
+  }
+  return final_result;
+}
+
 void V4Database::RecordDatabaseUpdateLatency() {
   if (!last_update_.is_null())
     UmaHistogramCustomTimes(kV4DatabaseUpdateLatency,

components/safe_browsing/core/browser/db/v4_database.h

@@ -179,6 +179,10 @@ class V4Database {
   // with the combined size of all the stores.
   void RecordFileSizeHistograms();
 
+  // Returns the migration result of the stores in this database. If the
+  // migration results for all stores do not match, returns kUnknown.
+  HashPrefixMap::MigrateResult GetMigrateResult();
+
   // Populates the DatabaseInfo message of the safe_browsing_page proto.
   void CollectDatabaseInfo(DatabaseManagerInfo::DatabaseInfo* database_info);
 

components/safe_browsing/core/browser/db/v4_local_database_manager.cc

@@ -347,9 +347,11 @@ scoped_refptr<V4LocalDatabaseManager> V4LocalDatabaseManager::Create(
     const base::FilePath& base_path,
     scoped_refptr<base::SequencedTaskRunner> ui_task_runner,
     scoped_refptr<base::SequencedTaskRunner> io_task_runner,
-    ExtendedReportingLevelCallback extended_reporting_level_callback) {
+    ExtendedReportingLevelCallback extended_reporting_level_callback,
+    RecordMigrationMetricsCallback record_migration_metrics_callback) {
   return base::WrapRefCounted(new V4LocalDatabaseManager(
-      base_path, extended_reporting_level_callback, std::move(ui_task_runner),
+      base_path, extended_reporting_level_callback,
+      std::move(record_migration_metrics_callback), std::move(ui_task_runner),
       std::move(io_task_runner), nullptr));
 }
 
@@ -373,13 +375,16 @@ void V4LocalDatabaseManager::CollectDatabaseManagerInfo(
 V4LocalDatabaseManager::V4LocalDatabaseManager(
     const base::FilePath& base_path,
     ExtendedReportingLevelCallback extended_reporting_level_callback,
+    RecordMigrationMetricsCallback record_migration_metrics_callback,
     scoped_refptr<base::SequencedTaskRunner> ui_task_runner,
     scoped_refptr<base::SequencedTaskRunner> io_task_runner,
     scoped_refptr<base::SequencedTaskRunner> task_runner_for_tests)
     : SafeBrowsingDatabaseManager(std::move(ui_task_runner),
                                   std::move(io_task_runner)),
       base_path_(base_path),
       extended_reporting_level_callback_(extended_reporting_level_callback),
+      record_migration_metrics_callback_(
+          std::move(record_migration_metrics_callback)),
       list_infos_(GetListInfos()),
       task_runner_(task_runner_for_tests
                        ? task_runner_for_tests
@@ -704,6 +709,12 @@ void V4LocalDatabaseManager::DatabaseReadyForChecks(
     v4_database_ = std::move(v4_database);
 
     v4_database_->RecordFileSizeHistograms();
+    if (record_migration_metrics_callback_) {
+      ui_task_runner()->PostTask(
+          FROM_HERE,
+          base::BindOnce(std::move(record_migration_metrics_callback_),
+                         v4_database_->GetMigrateResult()));
+    }
 
     PopulateArtificialDatabase();
 

components/safe_browsing/core/browser/db/v4_local_database_manager.h

@@ -34,13 +34,17 @@ typedef unsigned ThreatSeverity;
 // SafeBrowsing service and interfaces with the protocol manager.
 class V4LocalDatabaseManager : public SafeBrowsingDatabaseManager {
  public:
+  using RecordMigrationMetricsCallback =
+      base::OnceCallback<void(HashPrefixMap::MigrateResult)>;
+
   // Create and return an instance of V4LocalDatabaseManager, if Finch trial
   // allows it; nullptr otherwise.
   static scoped_refptr<V4LocalDatabaseManager> Create(
       const base::FilePath& base_path,
       scoped_refptr<base::SequencedTaskRunner> ui_task_runner,
       scoped_refptr<base::SequencedTaskRunner> io_task_runner,
-      ExtendedReportingLevelCallback extended_reporting_level_callback);
+      ExtendedReportingLevelCallback extended_reporting_level_callback,
+      RecordMigrationMetricsCallback record_migration_metrics_callback);
 
   V4LocalDatabaseManager(const V4LocalDatabaseManager&) = delete;
   V4LocalDatabaseManager& operator=(const V4LocalDatabaseManager&) = delete;
@@ -113,6 +117,7 @@ class V4LocalDatabaseManager : public SafeBrowsingDatabaseManager {
   V4LocalDatabaseManager(
       const base::FilePath& base_path,
       ExtendedReportingLevelCallback extended_reporting_level_callback,
+      RecordMigrationMetricsCallback record_migration_metrics_callback,
       scoped_refptr<base::SequencedTaskRunner> ui_task_runner,
       scoped_refptr<base::SequencedTaskRunner> io_task_runner,
       scoped_refptr<base::SequencedTaskRunner> task_runner_for_tests);
@@ -434,6 +439,9 @@ class V4LocalDatabaseManager : public SafeBrowsingDatabaseManager {
   // manager.
   ExtendedReportingLevelCallback extended_reporting_level_callback_;
 
+  // Callback to record metrics on database migration after initialization.
+  RecordMigrationMetricsCallback record_migration_metrics_callback_;
+
   // The client_state of each list currently being synced. This is updated each
   // time a database update completes, and used to send list client_state
   // information in the full hash request.

components/safe_browsing/core/browser/db/v4_local_database_manager_unittest.cc

@@ -363,6 +363,7 @@ class FakeV4LocalDatabaseManager : public V4LocalDatabaseManager {
       scoped_refptr<base::SequencedTaskRunner> task_runner)
       : V4LocalDatabaseManager(base_path,
                                extended_reporting_level_callback,
+                               RecordMigrationMetricsCallback(),
                                base::SequencedTaskRunner::GetCurrentDefault(),
                                base::SequencedTaskRunner::GetCurrentDefault(),
                                task_runner),
@@ -415,6 +416,7 @@ class V4LocalDatabaseManagerTest : public PlatformTest {
     v4_local_database_manager_ =
         base::WrapRefCounted(new V4LocalDatabaseManager(
             base_dir_.GetPath(), erl_callback_,
+            V4LocalDatabaseManager::RecordMigrationMetricsCallback(),
             base::SequencedTaskRunner::GetCurrentDefault(),
             base::SequencedTaskRunner::GetCurrentDefault(), task_runner_));
 
@@ -486,6 +488,7 @@ class V4LocalDatabaseManagerTest : public PlatformTest {
     v4_local_database_manager_ =
         base::WrapRefCounted(new V4LocalDatabaseManager(
             base_dir_.GetPath(), erl_callback_,
+            V4LocalDatabaseManager::RecordMigrationMetricsCallback(),
             base::SequencedTaskRunner::GetCurrentDefault(),
             base::SequencedTaskRunner::GetCurrentDefault(), task_runner_));
     StartLocalDatabaseManager();

components/safe_browsing/core/browser/db/v4_store.cc

@@ -900,10 +900,10 @@ StoreReadResult V4Store::ReadFromDisk() {
     return HASH_PREFIX_INFO_MISSING_FAILURE;
   }
 
-  HashPrefixMap::MigrateResult migrate_result =
-      MigrateFileFormatIfNeeded(&file_format);
-  if (migrate_result == HashPrefixMap::MigrateResult::kFailure)
+  migrate_result_ = MigrateFileFormatIfNeeded(&file_format);
+  if (migrate_result_ == HashPrefixMap::MigrateResult::kFailure) {
     return MIGRATION_FAILURE;
+  }
 
   ApplyUpdateResult apply_update_result =
       hash_prefix_map_->ReadFromDisk(file_format);
@@ -922,7 +922,7 @@ StoreReadResult V4Store::ReadFromDisk() {
   }
 
   // If a migration happened, we already updated file size.
-  if (migrate_result != HashPrefixMap::MigrateResult::kSuccess) {
+  if (migrate_result_ != HashPrefixMap::MigrateResult::kSuccess) {
     // Update |file_size_| now because we parsed the file correctly.
     file_size_ = file_size;
     for (const auto& hash_file : file_format.hash_files())

components/safe_browsing/core/browser/db/v4_store.h

@@ -176,6 +176,10 @@ class V4Store {
       DatabaseManagerInfo::DatabaseInfo::StoreInfo* store_info,
       const std::string& base_metric);
 
+  HashPrefixMap::MigrateResult migrate_result() const {
+    return migrate_result_;
+  }
+
  protected:
   std::unique_ptr<HashPrefixMap> hash_prefix_map_;
 
@@ -411,6 +415,8 @@ class V4Store {
   std::string state_;
   const base::FilePath store_path_;
   const scoped_refptr<base::SequencedTaskRunner> task_runner_;
+  HashPrefixMap::MigrateResult migrate_result_ =
+      HashPrefixMap::MigrateResult::kUnknown;
 };
 
 std::ostream& operator<<(std::ostream& os, const V4Store& store);

ios/components/security_interstitials/safe_browsing/safe_browsing_service_impl.mm

@@ -81,7 +81,8 @@ void StartSafeBrowsingDBManagerInternal(
   safe_browsing_db_manager_ = safe_browsing::V4LocalDatabaseManager::Create(
       safe_browsing_data_path, web::GetUIThreadTaskRunner({}),
       web::GetIOThreadTaskRunner({}),
-      safe_browsing::ExtendedReportingLevelCallback());
+      safe_browsing::ExtendedReportingLevelCallback(),
+      safe_browsing::V4LocalDatabaseManager::RecordMigrationMetricsCallback());
 
   io_thread_enabler_ =
       base::MakeRefCounted<IOThreadEnabler>(safe_browsing_db_manager_);