[AW] Cache the apps package names allowlist version

Don't lookup the allowlist if the loaded allowlist version is the same
as the cached one.

Passing the cached values in the component loader policy ctor for
cleaner tests and to avoid the class being dependant on
AwMetricsServiceClient. Also keep AwMetricsServiceClient the responsible
for accessing and updating the cached values.

Moved the cached values to a dictionary in the pref services instead of
storing each value independently. There is no need to migrate this code
or to handle backward compatibility because this feature isn't enabled
on any channel yet, including prestable channels.

Bug: 1216202
Test: bin/run_android_webview_unittests
Change-Id: I9f431b7c28ced90717ac46f52de39290a158b9dd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3010257
Commit-Queue: Hazem Ashmawy <hazems@chromium.org>
Reviewed-by: Michael Bai <michaelbai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#900954}

android_webview/browser/aw_feature_list_creator.cc

@@ -90,10 +90,9 @@ const char* const kPersistentPrefsAllowlist[] = {
     variations::prefs::kVariationsLastFetchTime,
     variations::prefs::kVariationsSeedDate,
 
-    // Cache the expiry date of the list of apps whose package names are allowed
+    // A dictionary that caches 'AppPackageNameLoggingRule' object which decides
-    // to be recorded in UMA. This will have a valid value (not
+    // whether the app package name should be recorded in UMA or not.
-    // base::Time::Min()) only if the app is in the allowlist.
+    prefs::kMetricsAppPackageNameLoggingRule,
-    prefs::kMetricsShouldRecordAppPackageNameExpiryDate,
 };
 
 void HandleReadError(PersistentPrefStore::PrefReadError error) {}

android_webview/browser/component_updater/loader_policies/aw_apps_package_names_allowlist_component_loader_policy.cc

@@ -16,6 +16,7 @@
 #include "android_webview/browser/metrics/aw_metrics_service_client.h"
 #include "android_webview/common/aw_features.h"
 #include "android_webview/common/components/aw_apps_package_names_allowlist_component_utils.h"
+#include "android_webview/common/metrics/app_package_name_logging_rule.h"
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/containers/flat_map.h"
@@ -38,52 +39,69 @@ namespace {
 
 constexpr int kBitsPerByte = 8;
 
-// It returns an empty (null) absl::optional<base::Time> if loading of the
+// Lookup the allowlist in `allowlist_fd`, returns null if it fails.
-// allowlist file fails. Otherwise it returns:
+//
-// - `expiry_date` if the given `package_name` is in the allowlist.
+// `allowlist_fd` the fd of a file the contain a bloomfilter that represents an
-// - `base::Time::Min()` if the given `package_name` isn't in the allowlist.
+//                allowlist of apps package names.
-absl::optional<base::Time> GetExpiryTimeIfPackageNameLoggable(
+// `num_hash`     the number of hash functions to use in the
+//                `optimization_guide::BloomFilter`.
+// `num_bits`     the number of bits in the `optimization_guide::BloomFilter`.
+// `package_name` the app package name to look up in the allowlist.
+// `version`      the allowlist version.
+// `expiry_date`  the expiry date of the allowlist, i.e the date after which
+//                this allowlist shouldn't be used.
+absl::optional<AppPackageNameLoggingRule> GetAppPackageNameLoggingRule(
     base::ScopedFD allowlist_fd,
     int num_hash,
     int num_bits,
     const std::string& package_name,
+    const base::Version& version,
     const base::Time& expiry_date) {
   // Transfer the ownership of the file from `allowlist_fd` to `file_stream`.
   base::ScopedFILE file_stream(fdopen(allowlist_fd.release(), "r"));
   if (!file_stream.get())
-    return absl::optional<base::Time>();
+    return absl::optional<AppPackageNameLoggingRule>();
 
   // TODO(https://crbug.com/1219496): use mmap instead of reading the whole
   // file.
   std::string bloom_filter_data;
   if (!base::ReadStreamToString(file_stream.get(), &bloom_filter_data) ||
       bloom_filter_data.empty()) {
-    return absl::optional<base::Time>();
+    return absl::optional<AppPackageNameLoggingRule>();
   }
 
   // Make sure the bloomfilter binary data is of the correct length.
   if (bloom_filter_data.size() !=
       size_t((num_bits + kBitsPerByte - 1) / kBitsPerByte)) {
-    return absl::optional<base::Time>();
+    return absl::optional<AppPackageNameLoggingRule>();
   }
 
-  return optimization_guide::BloomFilter(num_hash, num_bits, bloom_filter_data)
+  if (optimization_guide::BloomFilter(num_hash, num_bits, bloom_filter_data)
-                 .Contains(package_name)
+          .Contains(package_name)) {
-             ? expiry_date
+    return AppPackageNameLoggingRule(version, expiry_date);
-             : base::Time::Min();
+  } else {
+    return AppPackageNameLoggingRule(version, base::Time::Min());
+  }
 }
 
-void SetShouldRecordPackageName(absl::optional<base::Time> expiry_date) {
+void SetAppPackageNameLoggingRule(
+    absl::optional<AppPackageNameLoggingRule> record) {
   auto* metrics_service_client = AwMetricsServiceClient::GetInstance();
   DCHECK(metrics_service_client);
-  metrics_service_client->SetShouldRecordPackageName(expiry_date);
+  metrics_service_client->SetAppPackageNameLoggingRule(record);
 
-  if (expiry_date.has_value()) {
+  if (!record.has_value()) {
-    VLOG(2) << "WebView apps package allowlist is loaded, expiry_date ="
-            << (expiry_date.value() - base::Time::UnixEpoch()).InMilliseconds();
-  } else {
     VLOG(2) << "Failed to load WebView apps package allowlist";
   }
+
+  VLOG(2) << "WebView apps package allowlist version "
+          << record.value().GetVersion() << " is loaded";
+  if (record.value().IsAppPackageNameAllowed()) {
+    VLOG(2) << "App package name should be recorded until "
+            << record.value().GetExpiryDate();
+  } else {
+    VLOG(2) << "App package name shouldn't be recorded";
+  }
 }
 
 }  // namespace
@@ -91,8 +109,10 @@ void SetShouldRecordPackageName(absl::optional<base::Time> expiry_date) {
 AwAppsPackageNamesAllowlistComponentLoaderPolicy::
     AwAppsPackageNamesAllowlistComponentLoaderPolicy(
         std::string app_package_name,
+        absl::optional<AppPackageNameLoggingRule> cached_record,
         AllowListLookupCallback lookup_callback)
     : app_package_name_(std::move(app_package_name)),
+      cached_record_(cached_record),
       lookup_callback_(std::move(lookup_callback)) {
   DCHECK(!app_package_name_.empty());
   DCHECK(lookup_callback_);
@@ -123,8 +143,7 @@ void AwAppsPackageNamesAllowlistComponentLoaderPolicy::ComponentLoaded(
     const base::Version& version,
     base::flat_map<std::string, base::ScopedFD>& fd_map,
     std::unique_ptr<base::DictionaryValue> manifest) {
-  // TODO(https://crbug.com/1216202): store the allowlist version in the local
+  DCHECK(version.IsValid());
-  // cache, don't lookup the allowlist if it's the same version.
 
   // Have to use double because base::DictionaryValue doesn't support int64
   // values.
@@ -148,6 +167,13 @@ void AwAppsPackageNamesAllowlistComponentLoaderPolicy::ComponentLoaded(
     return;
   }
 
+  if (cached_record_.has_value() &&
+      cached_record_.value().GetVersion() == version) {
+    DCHECK(lookup_callback_);
+    std::move(lookup_callback_).Run(cached_record_);
+    return;
+  }
+
   auto allowlist_iterator = fd_map.find(kAllowlistBloomFilterFileName);
   if (allowlist_iterator == fd_map.end()) {
     ComponentLoadFailed();
@@ -156,17 +182,16 @@ void AwAppsPackageNamesAllowlistComponentLoaderPolicy::ComponentLoaded(
 
   base::ThreadPool::PostTaskAndReplyWithResult(
       FROM_HERE, {base::MayBlock(), base::TaskPriority::BEST_EFFORT},
-      base::BindOnce(&GetExpiryTimeIfPackageNameLoggable,
+      base::BindOnce(&GetAppPackageNameLoggingRule,
                      std::move(allowlist_iterator->second), num_hash.value(),
-                     num_bits.value(), std::move(app_package_name_),
+                     num_bits.value(), std::move(app_package_name_), version,
                      expiry_date),
       std::move(lookup_callback_));
 }
 
 void AwAppsPackageNamesAllowlistComponentLoaderPolicy::ComponentLoadFailed() {
   DCHECK(lookup_callback_);
-  std::move(lookup_callback_)
+  std::move(lookup_callback_).Run(absl::optional<AppPackageNameLoggingRule>());
-      .Run(/* expiry_date= */ absl::optional<base::Time>());
 }
 
 void AwAppsPackageNamesAllowlistComponentLoaderPolicy::GetHash(
@@ -180,10 +205,14 @@ void LoadPackageNamesAllowlistComponent(
           android_webview::features::kWebViewAppsPackageNamesAllowlist)) {
     return;
   }
+
+  auto* metrics_service_client = AwMetricsServiceClient::GetInstance();
+  DCHECK(metrics_service_client);
   policies.push_back(
       std::make_unique<AwAppsPackageNamesAllowlistComponentLoaderPolicy>(
-          AwMetricsServiceClient::GetInstance()->GetAppPackageName(),
+          metrics_service_client->GetAppPackageName(),
-          base::BindOnce(&SetShouldRecordPackageName)));
+          metrics_service_client->GetCachedAppPackageNameLoggingRule(),
+          base::BindOnce(&SetAppPackageNameLoggingRule)));
 }
 
 }  // namespace android_webview

android_webview/browser/component_updater/loader_policies/aw_apps_package_names_allowlist_component_loader_policy.h

@@ -11,6 +11,7 @@
 #include <string>
 #include <vector>
 
+#include "android_webview/common/metrics/app_package_name_logging_rule.h"
 #include "base/callback.h"
 #include "base/containers/flat_map.h"
 #include "base/files/scoped_file.h"
@@ -31,15 +32,10 @@ constexpr char kBloomFilterNumHashKey[] = "bloomfilter_num_hash";
 constexpr char kBloomFilterNumBitsKey[] = "bloomfilter_num_bits";
 constexpr char kExpiryDateKey[] = "expiry_date";
 
-// A callback that accepts an `absl::optional<base::Time>` expiry_date:
+// A callback for the result of loading and looking up the allowlist. If the
-// - If the allowlist loading fails, it will be called with a null value.
+// allowlist loading fails, it will be called with a null record.
-// - If the package name isn't in the allowlist, it will be called with an
-//   always expired date `base::Time::Min()`.
-// - If the package name is in the allowlist, it will be called with the
-//   expiry_date after which the app package name shouldn't be recorded in UMA
-//   metrics.
 using AllowListLookupCallback =
-    base::OnceCallback<void(absl::optional<base::Time>)>;
+    base::OnceCallback<void(absl::optional<AppPackageNameLoggingRule>)>;
 
 // Defines a loader responsible for receiving the allowlist for apps package
 // names that can be included in UMA records and lookup the embedding app's name
@@ -48,10 +44,13 @@ class AwAppsPackageNamesAllowlistComponentLoaderPolicy
     : public component_updater::ComponentLoaderPolicy {
  public:
   // `app_package_name` the embedding app package name.
-  // `lookup_callback` callback to report the result of looking up
+  // `cached_record`    the cached lookup result of a previous successfully
-  //                   `app_package_name` in the packages names allowlist.
+  //                    loaded allowlist, if any.
+  // `lookup_callback`  callback to report the result of looking up
+  //                    `app_package_name` in the packages names allowlist.
   AwAppsPackageNamesAllowlistComponentLoaderPolicy(
       std::string app_package_name,
+      absl::optional<AppPackageNameLoggingRule> cached_record,
       AllowListLookupCallback lookup_callback);
   ~AwAppsPackageNamesAllowlistComponentLoaderPolicy() override;
 
@@ -70,6 +69,8 @@ class AwAppsPackageNamesAllowlistComponentLoaderPolicy
 
  private:
   std::string app_package_name_;
+  absl::optional<AppPackageNameLoggingRule> cached_record_;
+
   AllowListLookupCallback lookup_callback_;
 };
 

android_webview/browser/component_updater/loader_policies/aw_apps_package_names_allowlist_component_loader_policy_unittest.cc

@@ -10,6 +10,7 @@
 
 #include <utility>
 
+#include "android_webview/common/metrics/app_package_name_logging_rule.h"
 #include "base/containers/flat_map.h"
 #include "base/cxx17_backports.h"
 #include "base/files/file_path.h"
@@ -31,6 +32,7 @@ namespace {
 
 constexpr int kNumHash = 11;
 constexpr int kNumBitsPerEntry = 16;
+constexpr char kTestAllowlistVersion[] = "123.456.789.10";
 const std::string kTestAllowlist[] = {"com.example.test", "my.fake.app",
                                       "yet.another.app"};
 double MillisFromUnixEpoch(const base::Time& time) {
@@ -80,9 +82,10 @@ class AwAppsPackageNamesAllowlistComponentLoaderPolicyTest
     return base::ScopedFD(allowlist_fd);
   }
 
-  void LookupConfirmationCallback(absl::optional<base::Time> expiry_date) {
+  void LookupConfirmationCallback(
+      absl::optional<AppPackageNameLoggingRule> record) {
     EXPECT_TRUE(checker_.CalledOnValidSequence());
-    allowlist_expiry_date_ = expiry_date;
+    allowlist_lookup_result_ = record;
     lookup_run_loop_.Quit();
   }
 
@@ -92,7 +95,7 @@ class AwAppsPackageNamesAllowlistComponentLoaderPolicyTest
   base::SequenceCheckerImpl checker_;
   base::RunLoop lookup_run_loop_;
 
-  absl::optional<base::Time> allowlist_expiry_date_;
+  absl::optional<AppPackageNameLoggingRule> allowlist_lookup_result_;
 
  private:
   base::FilePath allowlist_path_;
@@ -107,20 +110,50 @@ TEST_F(AwAppsPackageNamesAllowlistComponentLoaderPolicyTest,
   base::Time one_day_from_now =
       base::Time::Now() + base::TimeDelta::FromDays(1);
   manifest->SetDoubleKey(kExpiryDateKey, MillisFromUnixEpoch(one_day_from_now));
+  base::Version new_version(kTestAllowlistVersion);
 
   auto policy =
       std::make_unique<AwAppsPackageNamesAllowlistComponentLoaderPolicy>(
           kTestAllowlist[1],
+          AppPackageNameLoggingRule(base::Version("123.456.789.0"),
+                                    base::Time::Min()),
           base::BindOnce(&AwAppsPackageNamesAllowlistComponentLoaderPolicyTest::
                              LookupConfirmationCallback,
                          base::Unretained(this)));
 
-  policy->ComponentLoaded(base::Version(), fd_map,
+  policy->ComponentLoaded(new_version, fd_map,
                           base::DictionaryValue::From(std::move(manifest)));
 
   lookup_run_loop_.Run();
-  EXPECT_TRUE(allowlist_expiry_date_.has_value());
+  ASSERT_TRUE(allowlist_lookup_result_.has_value());
-  EXPECT_EQ(allowlist_expiry_date_.value(), one_day_from_now);
+  EXPECT_TRUE(allowlist_lookup_result_.value().IsAppPackageNameAllowed());
+  EXPECT_EQ(allowlist_lookup_result_.value().GetVersion(), new_version);
+  EXPECT_EQ(allowlist_lookup_result_.value().GetExpiryDate(), one_day_from_now);
+}
+
+TEST_F(AwAppsPackageNamesAllowlistComponentLoaderPolicyTest,
+       TestSameVersionAsCache) {
+  base::flat_map<std::string, base::ScopedFD> fd_map;
+  std::unique_ptr<base::Value> manifest = BuildTestManifest();
+  base::Time one_day_from_now =
+      base::Time::Now() + base::TimeDelta::FromDays(1);
+  base::Version version(kTestAllowlistVersion);
+
+  AppPackageNameLoggingRule expected_record(version, one_day_from_now);
+  auto policy =
+      std::make_unique<AwAppsPackageNamesAllowlistComponentLoaderPolicy>(
+          "test.some.app", expected_record,
+          base::BindOnce(&AwAppsPackageNamesAllowlistComponentLoaderPolicyTest::
+                             LookupConfirmationCallback,
+                         base::Unretained(this)));
+
+  policy->ComponentLoaded(version, fd_map,
+                          base::DictionaryValue::From(std::move(manifest)));
+
+  lookup_run_loop_.Run();
+  ASSERT_TRUE(allowlist_lookup_result_.has_value());
+  EXPECT_TRUE(allowlist_lookup_result_.value().IsAppPackageNameAllowed());
+  EXPECT_TRUE(expected_record.IsSameAs(allowlist_lookup_result_.value()));
 }
 
 TEST_F(AwAppsPackageNamesAllowlistComponentLoaderPolicyTest,
@@ -128,20 +161,22 @@ TEST_F(AwAppsPackageNamesAllowlistComponentLoaderPolicyTest,
   WritePackageNamesAllowListToFile();
   base::flat_map<std::string, base::ScopedFD> fd_map;
   fd_map[kAllowlistBloomFilterFileName] = OpenAndGetAllowlistFd();
+  base::Version new_version(kTestAllowlistVersion);
 
   auto policy =
       std::make_unique<AwAppsPackageNamesAllowlistComponentLoaderPolicy>(
-          "non.existent.app",
+          "non.existent.app", absl::optional<AppPackageNameLoggingRule>(),
           base::BindOnce(&AwAppsPackageNamesAllowlistComponentLoaderPolicyTest::
                              LookupConfirmationCallback,
                          base::Unretained(this)));
 
-  policy->ComponentLoaded(base::Version(), fd_map,
+  policy->ComponentLoaded(new_version, fd_map,
                           base::DictionaryValue::From(BuildTestManifest()));
 
   lookup_run_loop_.Run();
-  EXPECT_TRUE(allowlist_expiry_date_.has_value());
+  ASSERT_TRUE(allowlist_lookup_result_.has_value());
-  EXPECT_TRUE(allowlist_expiry_date_.value().is_min());
+  EXPECT_EQ(allowlist_lookup_result_.value().GetVersion(), new_version);
+  EXPECT_FALSE(allowlist_lookup_result_.value().IsAppPackageNameAllowed());
 }
 
 TEST_F(AwAppsPackageNamesAllowlistComponentLoaderPolicyTest,
@@ -151,16 +186,16 @@ TEST_F(AwAppsPackageNamesAllowlistComponentLoaderPolicyTest,
 
   auto policy =
       std::make_unique<AwAppsPackageNamesAllowlistComponentLoaderPolicy>(
-          kTestAllowlist[1],
+          kTestAllowlist[1], absl::optional<AppPackageNameLoggingRule>(),
           base::BindOnce(&AwAppsPackageNamesAllowlistComponentLoaderPolicyTest::
                              LookupConfirmationCallback,
                          base::Unretained(this)));
 
-  policy->ComponentLoaded(base::Version(), fd_map,
+  policy->ComponentLoaded(base::Version(kTestAllowlistVersion), fd_map,
                           base::DictionaryValue::From(BuildTestManifest()));
 
   lookup_run_loop_.Run();
-  EXPECT_FALSE(allowlist_expiry_date_.has_value());
+  EXPECT_FALSE(allowlist_lookup_result_.has_value());
 }
 
 TEST_F(AwAppsPackageNamesAllowlistComponentLoaderPolicyTest,
@@ -171,16 +206,16 @@ TEST_F(AwAppsPackageNamesAllowlistComponentLoaderPolicyTest,
 
   auto policy =
       std::make_unique<AwAppsPackageNamesAllowlistComponentLoaderPolicy>(
-          kTestAllowlist[1],
+          kTestAllowlist[1], absl::optional<AppPackageNameLoggingRule>(),
           base::BindOnce(&AwAppsPackageNamesAllowlistComponentLoaderPolicyTest::
                              LookupConfirmationCallback,
                          base::Unretained(this)));
 
-  policy->ComponentLoaded(base::Version(), fd_map,
+  policy->ComponentLoaded(base::Version(kTestAllowlistVersion), fd_map,
                           std::make_unique<base::DictionaryValue>());
 
   lookup_run_loop_.Run();
-  EXPECT_FALSE(allowlist_expiry_date_.has_value());
+  EXPECT_FALSE(allowlist_lookup_result_.has_value());
 }
 
 TEST_F(AwAppsPackageNamesAllowlistComponentLoaderPolicyTest,
@@ -191,16 +226,16 @@ TEST_F(AwAppsPackageNamesAllowlistComponentLoaderPolicyTest,
 
   auto policy =
       std::make_unique<AwAppsPackageNamesAllowlistComponentLoaderPolicy>(
-          kTestAllowlist[1],
+          kTestAllowlist[1], absl::optional<AppPackageNameLoggingRule>(),
           base::BindOnce(&AwAppsPackageNamesAllowlistComponentLoaderPolicyTest::
                              LookupConfirmationCallback,
                          base::Unretained(this)));
 
-  policy->ComponentLoaded(base::Version(), fd_map,
+  policy->ComponentLoaded(base::Version(kTestAllowlistVersion), fd_map,
                           base::DictionaryValue::From(BuildTestManifest()));
 
   lookup_run_loop_.Run();
-  EXPECT_FALSE(allowlist_expiry_date_.has_value());
+  EXPECT_FALSE(allowlist_lookup_result_.has_value());
 }
 
 TEST_F(AwAppsPackageNamesAllowlistComponentLoaderPolicyTest,
@@ -211,16 +246,16 @@ TEST_F(AwAppsPackageNamesAllowlistComponentLoaderPolicyTest,
 
   auto policy =
       std::make_unique<AwAppsPackageNamesAllowlistComponentLoaderPolicy>(
-          kTestAllowlist[1],
+          kTestAllowlist[1], absl::optional<AppPackageNameLoggingRule>(),
           base::BindOnce(&AwAppsPackageNamesAllowlistComponentLoaderPolicyTest::
                              LookupConfirmationCallback,
                          base::Unretained(this)));
 
-  policy->ComponentLoaded(base::Version(), fd_map,
+  policy->ComponentLoaded(base::Version(kTestAllowlistVersion), fd_map,
                           base::DictionaryValue::From(BuildTestManifest()));
 
   lookup_run_loop_.Run();
-  EXPECT_FALSE(allowlist_expiry_date_.has_value());
+  EXPECT_FALSE(allowlist_lookup_result_.has_value());
 }
 
 TEST_F(AwAppsPackageNamesAllowlistComponentLoaderPolicyTest,
@@ -235,16 +270,16 @@ TEST_F(AwAppsPackageNamesAllowlistComponentLoaderPolicyTest,
 
   auto policy =
       std::make_unique<AwAppsPackageNamesAllowlistComponentLoaderPolicy>(
-          kTestAllowlist[1],
+          kTestAllowlist[1], absl::optional<AppPackageNameLoggingRule>(),
           base::BindOnce(&AwAppsPackageNamesAllowlistComponentLoaderPolicyTest::
                              LookupConfirmationCallback,
                          base::Unretained(this)));
 
-  policy->ComponentLoaded(base::Version(), fd_map,
+  policy->ComponentLoaded(base::Version(kTestAllowlistVersion), fd_map,
                           base::DictionaryValue::From(std::move(manifest)));
 
   lookup_run_loop_.Run();
-  EXPECT_FALSE(allowlist_expiry_date_.has_value());
+  EXPECT_FALSE(allowlist_lookup_result_.has_value());
 }
 
 }  // namespace android_webview

android_webview/browser/metrics/aw_metrics_service_client.cc

@@ -10,6 +10,7 @@
 #include "android_webview/browser/metrics/aw_stability_metrics_provider.h"
 #include "android_webview/browser_jni_headers/AwMetricsServiceClient_jni.h"
 #include "android_webview/common/aw_features.h"
+#include "android_webview/common/metrics/app_package_name_logging_rule.h"
 #include "base/android/callback_android.h"
 #include "base/android/jni_android.h"
 #include "base/feature_list.h"
@@ -25,8 +26,8 @@
 namespace android_webview {
 
 namespace prefs {
-const char kMetricsShouldRecordAppPackageNameExpiryDate[] =
+const char kMetricsAppPackageNameLoggingRule[] =
-    "aw_metrics_app_package_name_allowlist.expiry_date";
+    "aw_metrics_app_package_name_logging_rule";
 }  // namespace prefs
 
 namespace {
@@ -103,22 +104,41 @@ bool AwMetricsServiceClient::ShouldRecordPackageName() {
     return ::metrics::AndroidMetricsServiceClient::ShouldRecordPackageName();
   }
 
-  PrefService* local_state = pref_service();
+  return cached_package_name_record_.has_value() &&
-  DCHECK(local_state);
+         cached_package_name_record_.value().IsAppPackageNameAllowed();
-  return local_state->GetTime(
-             prefs::kMetricsShouldRecordAppPackageNameExpiryDate) >=
-         base::Time::Now();
 }
 
-void AwMetricsServiceClient::SetShouldRecordPackageName(
+void AwMetricsServiceClient::SetAppPackageNameLoggingRule(
-    absl::optional<base::Time> expiry_date) {
+    absl::optional<AppPackageNameLoggingRule> record) {
-  if (!expiry_date.has_value())
+  if (!record.has_value())
     return;
 
+  absl::optional<AppPackageNameLoggingRule> cached_record =
+      GetCachedAppPackageNameLoggingRule();
+
+  if (cached_record.has_value() &&
+      record.value().IsSameAs(cached_package_name_record_.value())) {
+    return;
+  }
+
   PrefService* local_state = pref_service();
   DCHECK(local_state);
-  local_state->SetTime(prefs::kMetricsShouldRecordAppPackageNameExpiryDate,
+  local_state->Set(prefs::kMetricsAppPackageNameLoggingRule,
-                       expiry_date.value());
+                   record.value().ToDictionary());
+  cached_package_name_record_ = record;
+}
+
+absl::optional<AppPackageNameLoggingRule>
+AwMetricsServiceClient::GetCachedAppPackageNameLoggingRule() {
+  if (cached_package_name_record_.has_value()) {
+    return cached_package_name_record_;
+  }
+
+  PrefService* local_state = pref_service();
+  DCHECK(local_state);
+  cached_package_name_record_ = AppPackageNameLoggingRule::FromDictionary(
+      *(local_state->Get(prefs::kMetricsAppPackageNameLoggingRule)));
+  return cached_package_name_record_;
 }
 
 void AwMetricsServiceClient::OnMetricsStart() {
@@ -172,8 +192,8 @@ void AwMetricsServiceClient::RegisterAdditionalMetricsProviders(
 void AwMetricsServiceClient::RegisterMetricsPrefs(
     PrefRegistrySimple* registry) {
   RegisterPrefs(registry);
-  registry->RegisterTimePref(
+  registry->RegisterDictionaryPref(prefs::kMetricsAppPackageNameLoggingRule,
-      prefs::kMetricsShouldRecordAppPackageNameExpiryDate, base::Time::Min());
+                                   base::Value(base::Value::Type::DICTIONARY));
 }
 
 // static

android_webview/browser/metrics/aw_metrics_service_client.h

@@ -9,11 +9,11 @@
 #include <string>
 
 #include "android_webview/browser/lifecycle/webview_app_state_observer.h"
+#include "android_webview/common/metrics/app_package_name_logging_rule.h"
 #include "base/macros.h"
 #include "base/metrics/field_trial.h"
 #include "base/no_destructor.h"
 #include "base/sequence_checker.h"
-#include "base/time/time.h"
 #include "components/embedder_support/android/metrics/android_metrics_service_client.h"
 #include "components/metrics/enabled_state_provider.h"
 #include "components/metrics/metrics_log_uploader.h"
@@ -24,7 +24,7 @@
 namespace android_webview {
 
 namespace prefs {
-extern const char kMetricsShouldRecordAppPackageNameExpiryDate[];
+extern const char kMetricsAppPackageNameLoggingRule[];
 }  // namespace prefs
 
 // These values are persisted to logs. Entries should not be renumbered and
@@ -98,7 +98,7 @@ enum class BackfillInstallDate {
 // WebView will try to lookup the embedding app's package name in a list of apps
 // whose package names are allowed to be recorded. This operation takes place on
 // a background thread. The result of the lookup is then posted back on the UI
-// thread and SetShouldRecordPackageName() will be called. Unlike user's
+// thread and SetAppPackageNameLoggingRule() will be called. Unlike user's
 // consent, the metrics service doesn't currently block on the allowlist lookup
 // result. If the result isn't present at the moment of creating a metrics log,
 // it assumes that the app package name isn't allowed to be logged.
@@ -166,11 +166,15 @@ class AwMetricsServiceClient : public ::metrics::AndroidMetricsServiceClient,
   // dynamically downloaded allowlist of apps see
   // `AwAppsPackageNamesAllowlistComponentLoaderPolicy`.
   //
-  // `expiry_date` the date after which the app package name shouldn't be
+  // `record` If it has a null value, then it will be ignored and the cached
-  //               recoreded in UMA because the allowlist that contained this
+  //          record will be used if any.
-  //               app has expired. If it has a null value, then it will be
+  void SetAppPackageNameLoggingRule(
-  //               ignored and the cached date will be used if any.
+      absl::optional<AppPackageNameLoggingRule> record);
-  void SetShouldRecordPackageName(absl::optional<base::Time> expiry_date);
+
+  // Get the cached record of the app package names allowlist set by
+  // `SetAppPackageNameLoggingRule` if any.
+  absl::optional<AppPackageNameLoggingRule>
+  GetCachedAppPackageNameLoggingRule();
 
  protected:
   // Restrict usage of the inherited AndroidMetricsServiceClient::RegisterPrefs,
@@ -181,6 +185,8 @@ class AwMetricsServiceClient : public ::metrics::AndroidMetricsServiceClient,
   bool app_in_foreground_ = false;
   std::unique_ptr<Delegate> delegate_;
 
+  absl::optional<AppPackageNameLoggingRule> cached_package_name_record_;
+
   DISALLOW_COPY_AND_ASSIGN(AwMetricsServiceClient);
 };
 

android_webview/browser/metrics/aw_metrics_service_client_unittest.cc

@@ -7,10 +7,12 @@
 #include <memory>
 
 #include "android_webview/common/aw_features.h"
+#include "android_webview/common/metrics/app_package_name_logging_rule.h"
 #include "base/metrics/user_metrics.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/test/test_simple_task_runner.h"
 #include "base/time/time.h"
+#include "base/version.h"
 #include "components/prefs/testing_pref_service.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -18,6 +20,8 @@ namespace android_webview {
 
 namespace {
 
+constexpr char kTestAllowlistVersion[] = "123.456.789.10";
+
 class AwMetricsServiceClientTestDelegate
     : public AwMetricsServiceClient::Delegate {
   void RegisterAdditionalMetricsProviders(
@@ -45,6 +49,7 @@ class AwMetricsServiceClientTest : public testing::Test {
   }
 
   AwMetricsServiceClient* GetClient() { return client_.get(); }
+  TestingPrefServiceSimple* GetPrefs() { return prefs_.get(); }
 
  private:
   scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
@@ -59,45 +64,90 @@ TEST_F(AwMetricsServiceClientTest, TestShouldRecordPackageName_CacheNotSet) {
   scoped_list.InitAndEnableFeature(
       android_webview::features::kWebViewAppsPackageNamesAllowlist);
 
-  EXPECT_FALSE(GetClient()->ShouldRecordPackageName());
+  AwMetricsServiceClient* client = GetClient();
-}
-
-TEST_F(AwMetricsServiceClientTest,
-       TestShouldRecordPackageName_TestExpiredResult) {
-  base::test::ScopedFeatureList scoped_list;
-  scoped_list.InitAndEnableFeature(
-      android_webview::features::kWebViewAppsPackageNamesAllowlist);
-
-  auto* client = GetClient();
-  auto one_day_ago = base::Time::Now() - base::TimeDelta::FromDays(1);
-  client->SetShouldRecordPackageName(/* expiry_date= */ one_day_ago);
   EXPECT_FALSE(client->ShouldRecordPackageName());
+  EXPECT_FALSE(client->GetCachedAppPackageNameLoggingRule().has_value());
 }
 
-TEST_F(AwMetricsServiceClientTest,
+TEST_F(AwMetricsServiceClientTest, TestShouldRecordPackageName_WithCache) {
-       TestShouldRecordPackageName_TestValidResult) {
   base::test::ScopedFeatureList scoped_list;
   scoped_list.InitAndEnableFeature(
       android_webview::features::kWebViewAppsPackageNamesAllowlist);
 
-  auto* client = GetClient();
+  AwMetricsServiceClient* client = GetClient();
+  TestingPrefServiceSimple* prefs = GetPrefs();
+
   auto one_day_from_now = base::Time::Now() + base::TimeDelta::FromDays(1);
-  client->SetShouldRecordPackageName(/* expiry_date= */ one_day_from_now);
+  AppPackageNameLoggingRule expected_record(
+      base::Version(kTestAllowlistVersion), one_day_from_now);
+  prefs->Set(prefs::kMetricsAppPackageNameLoggingRule,
+             expected_record.ToDictionary());
+
+  absl::optional<AppPackageNameLoggingRule> cached_record =
+      client->GetCachedAppPackageNameLoggingRule();
   EXPECT_TRUE(client->ShouldRecordPackageName());
+  ASSERT_TRUE(cached_record.has_value());
+  EXPECT_TRUE(expected_record.IsSameAs(cached_record.value()));
 }
 
 TEST_F(AwMetricsServiceClientTest,
-       TestShouldRecordPackageName_TestInvalidResult) {
+       TestShouldRecordPackageName_TestShouldNotRecordPackageName) {
+  base::test::ScopedFeatureList scoped_list;
+  scoped_list.InitAndEnableFeature(
+      android_webview::features::kWebViewAppsPackageNamesAllowlist);
+
+  AwMetricsServiceClient* client = GetClient();
+  AppPackageNameLoggingRule expected_record(
+      base::Version(kTestAllowlistVersion), base::Time::Min());
+  client->SetAppPackageNameLoggingRule(expected_record);
+  absl::optional<AppPackageNameLoggingRule> cached_record =
+      client->GetCachedAppPackageNameLoggingRule();
+
+  EXPECT_FALSE(client->ShouldRecordPackageName());
+  ASSERT_TRUE(cached_record.has_value());
+  EXPECT_TRUE(expected_record.IsSameAs(cached_record.value()));
+}
+
+TEST_F(AwMetricsServiceClientTest,
+       TestShouldRecordPackageName_TestShouldRecordPackageName) {
   base::test::ScopedFeatureList scoped_list;
   scoped_list.InitAndEnableFeature(
       android_webview::features::kWebViewAppsPackageNamesAllowlist);
 
-  auto* client = GetClient();
   auto one_day_from_now = base::Time::Now() + base::TimeDelta::FromDays(1);
-  client->SetShouldRecordPackageName(/* expiry_date= */ one_day_from_now);
+
-  client->SetShouldRecordPackageName(
+  AwMetricsServiceClient* client = GetClient();
-      /* expiry_date= */ absl::optional<base::Time>());
+  AppPackageNameLoggingRule expected_record(
+      base::Version(kTestAllowlistVersion), one_day_from_now);
+  client->SetAppPackageNameLoggingRule(expected_record);
+  absl::optional<AppPackageNameLoggingRule> cached_record =
+      client->GetCachedAppPackageNameLoggingRule();
+
   EXPECT_TRUE(client->ShouldRecordPackageName());
+  ASSERT_TRUE(cached_record.has_value());
+  EXPECT_TRUE(expected_record.IsSameAs(cached_record.value()));
+}
+
+TEST_F(AwMetricsServiceClientTest,
+       TestShouldRecordPackageName_TestFailureAfterValidResult) {
+  base::test::ScopedFeatureList scoped_list;
+  scoped_list.InitAndEnableFeature(
+      android_webview::features::kWebViewAppsPackageNamesAllowlist);
+
+  auto one_day_from_now = base::Time::Now() + base::TimeDelta::FromDays(1);
+
+  AwMetricsServiceClient* client = GetClient();
+  AppPackageNameLoggingRule expected_record(
+      base::Version(kTestAllowlistVersion), one_day_from_now);
+  client->SetAppPackageNameLoggingRule(expected_record);
+  client->SetAppPackageNameLoggingRule(
+      absl::optional<AppPackageNameLoggingRule>());
+  absl::optional<AppPackageNameLoggingRule> cached_record =
+      client->GetCachedAppPackageNameLoggingRule();
+
+  EXPECT_TRUE(client->ShouldRecordPackageName());
+  ASSERT_TRUE(cached_record.has_value());
+  EXPECT_TRUE(expected_record.IsSameAs(cached_record.value()));
 }
 
 }  // namespace android_webview

android_webview/common/BUILD.gn

@@ -29,6 +29,8 @@ source_set("common") {
     "crash_reporter/crash_keys.cc",
     "crash_reporter/crash_keys.h",
     "devtools_instrumentation.h",
+    "metrics/app_package_name_logging_rule.cc",
+    "metrics/app_package_name_logging_rule.h",
     "url_constants.cc",
     "url_constants.h",
   ]
@@ -74,3 +76,14 @@ mojom("mojom") {
     "//url/mojom:url_mojom_gurl",
   ]
 }
+
+source_set("tests") {
+  testonly = true
+
+  sources = [ "metrics/app_package_name_logging_rule_unittests.cc" ]
+  deps = [
+    ":common",
+    "//base",
+    "//base/test:test_support",
+  ]
+}

android_webview/common/metrics/app_package_name_logging_rule.cc

@@ -0,0 +1,84 @@
+// Copyright 2021 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "android_webview/common/metrics/app_package_name_logging_rule.h"
+
+#include "base/time/time.h"
+#include "base/util/values/values_util.h"
+#include "base/values.h"
+#include "base/version.h"
+#include "third_party/abseil-cpp/absl/types/optional.h"
+
+namespace android_webview {
+
+namespace {
+constexpr char kExpiryDateKey[] = "expiry-date";
+constexpr char kVersionKey[] = "allowlist-version";
+}  // namespace
+
+AppPackageNameLoggingRule::AppPackageNameLoggingRule(
+    const base::Version& version,
+    const base::Time& expiry_date)
+    : version_(version), expiry_date_(expiry_date) {
+  DCHECK(version.IsValid());
+  DCHECK(!expiry_date.is_null());
+}
+
+base::Version AppPackageNameLoggingRule::GetVersion() const {
+  return version_;
+}
+
+base::Time AppPackageNameLoggingRule::GetExpiryDate() const {
+  return expiry_date_;
+}
+
+bool AppPackageNameLoggingRule::IsAppPackageNameAllowed() const {
+  return expiry_date_ >= base::Time::Now();
+}
+
+bool AppPackageNameLoggingRule::IsSameAs(
+    const AppPackageNameLoggingRule& record) const {
+  if (record.GetVersion() == GetVersion()) {
+    DCHECK(record.GetExpiryDate() == GetExpiryDate());
+    return true;
+  }
+  return false;
+}
+
+// static
+absl::optional<AppPackageNameLoggingRule>
+AppPackageNameLoggingRule::FromDictionary(const base::Value& dict) {
+  const std::string* version_string = dict.FindStringKey(kVersionKey);
+  if (!version_string) {
+    return absl::optional<AppPackageNameLoggingRule>();
+  }
+  base::Version version(*version_string);
+  if (!version.IsValid()) {
+    return absl::optional<AppPackageNameLoggingRule>();
+  }
+
+  const base::Value* expiry_date_value = dict.FindKey(kExpiryDateKey);
+  if (!expiry_date_value) {
+    return AppPackageNameLoggingRule(version, base::Time::Min());
+  }
+  absl::optional<base::Time> expiry_date =
+      util::ValueToTime(*expiry_date_value);
+  if (!expiry_date.has_value()) {
+    return AppPackageNameLoggingRule(version, base::Time::Min());
+  }
+
+  return AppPackageNameLoggingRule(version, expiry_date.value());
+}
+
+base::Value AppPackageNameLoggingRule::ToDictionary() {
+  base::Value dict(base::Value::Type::DICTIONARY);
+
+  dict.SetStringKey(kVersionKey, version_.GetString());
+  if (!expiry_date_.is_min()) {
+    dict.SetKey(kExpiryDateKey, util::TimeToValue(expiry_date_));
+  }
+  return dict;
+}
+
+}  // namespace android_webview

android_webview/common/metrics/app_package_name_logging_rule.h

@@ -0,0 +1,54 @@
+// Copyright 2021 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef ANDROID_WEBVIEW_COMMON_METRICS_APP_PACKAGE_NAME_LOGGING_RULE_H_
+#define ANDROID_WEBVIEW_COMMON_METRICS_APP_PACKAGE_NAME_LOGGING_RULE_H_
+
+#include "base/time/time.h"
+#include "base/values.h"
+#include "base/version.h"
+#include "third_party/abseil-cpp/absl/types/optional.h"
+
+namespace android_webview {
+
+// A class to hold the state of whether an app package name should be recorded
+// in UMA metrics log or not. This represent the result of looking the app
+// package name in a list of allowed apps.
+class AppPackageNameLoggingRule {
+ public:
+  AppPackageNameLoggingRule(const base::Version& version,
+                            const base::Time& expiry_date);
+  ~AppPackageNameLoggingRule() = default;
+
+  AppPackageNameLoggingRule(const AppPackageNameLoggingRule&) = default;
+  AppPackageNameLoggingRule& operator=(const AppPackageNameLoggingRule&) =
+      default;
+  AppPackageNameLoggingRule(AppPackageNameLoggingRule&&) = default;
+  AppPackageNameLoggingRule& operator=(AppPackageNameLoggingRule&&) = default;
+
+  base::Version GetVersion() const;
+  base::Time GetExpiryDate() const;
+
+  // Return `true` is the app is in the allowlist and the result hasn't expired,
+  // `false` otherwise.
+  bool IsAppPackageNameAllowed() const;
+
+  // If it has the same version and expiry_date as `record`.
+  bool IsSameAs(const AppPackageNameLoggingRule& record) const;
+
+  base::Value ToDictionary();
+
+  // Creates a valid AppPackageNameLoggingRule from a dictionary, or null if
+  // the dictionary have invalid values.
+  static absl::optional<AppPackageNameLoggingRule> FromDictionary(
+      const base::Value& dict);
+
+ private:
+  base::Version version_;
+  base::Time expiry_date_;
+};
+
+}  // namespace android_webview
+
+#endif  // ANDROID_WEBVIEW_COMMON_METRICS_APP_PACKAGE_NAME_LOGGING_RULE_H_

android_webview/common/metrics/app_package_name_logging_rule_unittests.cc

@@ -0,0 +1,60 @@
+// Copyright 2021 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "android_webview/common/metrics/app_package_name_logging_rule.h"
+
+#include "base/time/time.h"
+#include "base/version.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/abseil-cpp/absl/types/optional.h"
+
+namespace android_webview {
+
+namespace {
+
+constexpr char kTestAllowlistVersion[] = "123.456.789.10";
+}  // namespace
+
+class AppPackageNameLoggingRuleTest : public testing::Test {
+ public:
+  AppPackageNameLoggingRuleTest() = default;
+
+  AppPackageNameLoggingRuleTest& operator=(
+      const AppPackageNameLoggingRuleTest&) = delete;
+  AppPackageNameLoggingRuleTest(AppPackageNameLoggingRuleTest&&) = delete;
+  AppPackageNameLoggingRuleTest& operator=(AppPackageNameLoggingRuleTest&&) =
+      delete;
+};
+
+TEST_F(AppPackageNameLoggingRuleTest, TestFromDictionary) {
+  base::Version version(kTestAllowlistVersion);
+  base::Time one_day_from_now =
+      base::Time::Now() + base::TimeDelta::FromDays(1);
+  {
+    AppPackageNameLoggingRule expected_record(version, one_day_from_now);
+    absl::optional<AppPackageNameLoggingRule> record =
+        AppPackageNameLoggingRule::FromDictionary(
+            expected_record.ToDictionary());
+    ASSERT_TRUE(record.has_value());
+    EXPECT_TRUE(expected_record.IsSameAs(record.value()));
+  }
+
+  {
+    AppPackageNameLoggingRule expected_record(version, base::Time::Min());
+    absl::optional<AppPackageNameLoggingRule> record =
+        AppPackageNameLoggingRule::FromDictionary(
+            expected_record.ToDictionary());
+    ASSERT_TRUE(record.has_value());
+    EXPECT_TRUE(expected_record.IsSameAs(record.value()));
+  }
+
+  {
+    absl::optional<AppPackageNameLoggingRule> record =
+        AppPackageNameLoggingRule::FromDictionary(
+            base::Value(base::Value::Type::DICTIONARY));
+    EXPECT_FALSE(record.has_value());
+  }
+}
+
+}  // namespace android_webview

android_webview/test/BUILD.gn

@@ -485,6 +485,7 @@ test("android_webview_unittests") {
     "//android_webview/browser/lifecycle",
     "//android_webview/browser/metrics",
     "//android_webview/common",
+    "//android_webview/common:tests",
     "//android_webview/nonembedded",
     "//base/test:test_support",
     "//components/autofill/core/browser",