Add SourceIdType::WEBAPK_ID for recording webapk ukm

Whe recording WebAPK UKM, we use manifest url as the source_url. But UKM
has a mechanism that filter out the url that are not whitelisted.
Manifest url does not matches the navigation url or other whitelisted
url, so the record is drop with DropDataReason::NOT_MATCHED.

This CL adds another SourceIdType WEBAPK_ID for the WebAPK manifests, so
that these url are whitelisted.

Bug: 1044706
Change-Id: I988473f52afcf5b25cc5fafd43934e866f628f76
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2018086
Commit-Queue: Ella Ge <eirage@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Peter Kotwicz <pkotwicz@chromium.org>
Reviewed-by: Glenn Hartmann <hartmanng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#736903}

base/metrics/ukm_source_id.cc

@@ -13,7 +13,7 @@ namespace base {
 namespace {
 
 const int64_t kLowBitsMask = (INT64_C(1) << 32) - 1;
-const int64_t kNumTypeBits = 2;
+const int64_t kNumTypeBits = 3;
 const int64_t kTypeMask = (INT64_C(1) << kNumTypeBits) - 1;
 
 }  // namespace

base/metrics/ukm_source_id.h

@@ -36,6 +36,10 @@ class BASE_EXPORT UkmSourceId {
     // same report interval; it will not be kept in memory between different
     // reports.
     HISTORY_ID = 3,
+    // Source ID used by WebApkUkmRecorder. A new source of this type and
+    // associated events are expected to be recorded within the same report
+    // interval; it will not be kept in memory between different reports.
+    WEBAPK_ID = 4,
   };
 
   // Default constructor has the invalid value.

chrome/browser/android/webapk/webapk_ukm_recorder.cc

@@ -34,9 +34,8 @@ void WebApkUkmRecorder::RecordInstall(const GURL& manifest_url,
   if (!manifest_url.is_valid())
     return;
 
-  ukm::SourceId source_id = ukm::UkmRecorder::GetNewSourceID();
-  ukm::UkmRecorder* ukm_recorder = ukm::UkmRecorder::Get();
-  ukm_recorder->UpdateSourceURL(source_id, manifest_url);
+  ukm::SourceId source_id =
+      ukm::UkmRecorder::GetSourceIdForWebApkManifestUrl(manifest_url);
 
   // All installs through this method are browser-installs (ie, they should all
   // use the "browser" distributor).
@@ -44,7 +43,7 @@ void WebApkUkmRecorder::RecordInstall(const GURL& manifest_url,
       .SetDistributor(static_cast<int64_t>(WebApkDistributor::BROWSER))
       .SetAppVersion(version_code)
       .SetInstall(1)
-      .Record(ukm_recorder);
+      .Record(ukm::UkmRecorder::Get());
 }
 
 // static
@@ -55,14 +54,13 @@ void WebApkUkmRecorder::RecordSessionDuration(const GURL& manifest_url,
   if (!manifest_url.is_valid())
     return;
 
-  ukm::SourceId source_id = ukm::UkmRecorder::GetNewSourceID();
-  ukm::UkmRecorder* ukm_recorder = ukm::UkmRecorder::Get();
-  ukm_recorder->UpdateSourceURL(source_id, manifest_url);
+  ukm::SourceId source_id =
+      ukm::UkmRecorder::GetSourceIdForWebApkManifestUrl(manifest_url);
   ukm::builders::WebAPK_SessionEnd(source_id)
       .SetDistributor(distributor)
       .SetAppVersion(version_code)
       .SetSessionDuration(ukm::GetExponentialBucketMinForUserTiming(duration))
-      .Record(ukm_recorder);
+      .Record(ukm::UkmRecorder::Get());
 }
 
 // static
@@ -73,15 +71,14 @@ void WebApkUkmRecorder::RecordVisit(const GURL& manifest_url,
   if (!manifest_url.is_valid())
     return;
 
-  ukm::SourceId source_id = ukm::UkmRecorder::GetNewSourceID();
-  ukm::UkmRecorder* ukm_recorder = ukm::UkmRecorder::Get();
-  ukm_recorder->UpdateSourceURL(source_id, manifest_url);
+  ukm::SourceId source_id =
+      ukm::UkmRecorder::GetSourceIdForWebApkManifestUrl(manifest_url);
   ukm::builders::WebAPK_Visit(source_id)
       .SetDistributor(distributor)
       .SetAppVersion(version_code)
       .SetLaunchSource(source)
       .SetLaunch(1)
-      .Record(ukm_recorder);
+      .Record(ukm::UkmRecorder::Get());
 }
 
 // static
@@ -92,9 +89,8 @@ void WebApkUkmRecorder::RecordUninstall(const GURL& manifest_url,
                                         int64_t installed_duration_ms) {
   // UKM metric |launch_count| parameter is enum. '2' indicates >= 2 launches.
   launch_count = base::ClampToRange<int64_t>(launch_count, 0, 2);
-  ukm::SourceId source_id = ukm::UkmRecorder::GetNewSourceID();
-  ukm::UkmRecorder* ukm_recorder = ukm::UkmRecorder::Get();
-  ukm_recorder->UpdateSourceURL(source_id, manifest_url);
+  ukm::SourceId source_id =
+      ukm::UkmRecorder::GetSourceIdForWebApkManifestUrl(manifest_url);
   ukm::builders::WebAPK_Uninstall(source_id)
       .SetDistributor(distributor)
       .SetAppVersion(version_code)
@@ -102,7 +98,7 @@ void WebApkUkmRecorder::RecordUninstall(const GURL& manifest_url,
       .SetInstalledDuration(
           ukm::GetExponentialBucketMinForUserTiming(installed_duration_ms))
       .SetUninstall(1)
-      .Record(ukm_recorder);
+      .Record(ukm::UkmRecorder::Get());
 }
 
 // Called by the Java counterpart to record the Session Duration UKM metric.

components/ukm/ukm_recorder_impl.cc

@@ -47,7 +47,8 @@ std::string GetWhitelistEntries() {
 bool IsWhitelistedSourceId(SourceId source_id) {
   return GetSourceIdType(source_id) == SourceIdType::NAVIGATION_ID ||
          GetSourceIdType(source_id) == SourceIdType::APP_ID ||
-         GetSourceIdType(source_id) == SourceIdType::HISTORY_ID;
+         GetSourceIdType(source_id) == SourceIdType::HISTORY_ID ||
+         GetSourceIdType(source_id) == SourceIdType::WEBAPK_ID;
 }
 
 // Gets the maximum number of Sources we'll keep in memory before discarding any
@@ -320,7 +321,8 @@ void UkmRecorderImpl::StoreRecordingsInReport(Report* report) {
     // Don't keep sources of these types after current report because their
     // entries are logged only at source creation time.
     if (GetSourceIdType(kv.first) == base::UkmSourceId::Type::APP_ID ||
-        GetSourceIdType(kv.first) == base::UkmSourceId::Type::HISTORY_ID) {
+        GetSourceIdType(kv.first) == base::UkmSourceId::Type::HISTORY_ID ||
+        GetSourceIdType(kv.first) == base::UkmSourceId::Type::WEBAPK_ID) {
       MarkSourceForDeletion(kv.first);
     }
     // If the source id is not whitelisted, don't send it unless it has

components/ukm/ukm_recorder_impl.h

@@ -135,6 +135,7 @@ class UkmRecorderImpl : public UkmRecorder {
   friend ::ukm::UkmUtilsForTest;
   FRIEND_TEST_ALL_PREFIXES(UkmRecorderImplTest, IsSampledIn);
   FRIEND_TEST_ALL_PREFIXES(UkmRecorderImplTest, PurgeExtensionRecordings);
+  FRIEND_TEST_ALL_PREFIXES(UkmRecorderImplTest, WebApkSourceUrl);
 
   struct MetricAggregate {
     uint64_t total_count = 0;

components/ukm/ukm_recorder_impl_unittest.cc

@@ -7,6 +7,7 @@
 #include "base/bind.h"
 #include "base/metrics/ukm_source_id.h"
 #include "base/test/scoped_feature_list.h"
+#include "base/test/task_environment.h"
 #include "components/ukm/test_ukm_recorder.h"
 #include "services/metrics/public/cpp/ukm_builders.h"
 #include "services/metrics/public/cpp/ukm_entry_builder.h"
@@ -106,4 +107,22 @@ TEST(UkmRecorderImplTest, PurgeExtensionRecordings) {
   EXPECT_EQ(2U, recorder.sources().size());
 }
 
+TEST(UkmRecorderImplTest, WebApkSourceUrl) {
+  base::test::TaskEnvironment env;
+  ukm::TestAutoSetUkmRecorder test_ukm_recorder;
+
+  GURL url("https://example_url.com/manifest.json");
+  SourceId id = UkmRecorderImpl::GetSourceIdForWebApkManifestUrl(url);
+
+  ASSERT_NE(kInvalidSourceId, id);
+
+  const auto& sources = test_ukm_recorder.GetSources();
+  ASSERT_EQ(1ul, sources.size());
+  auto it = sources.find(id);
+  ASSERT_NE(sources.end(), it);
+  EXPECT_EQ(url, it->second->url());
+  EXPECT_EQ(1u, it->second->urls().size());
+  EXPECT_EQ(SourceIdType::WEBAPK_ID, GetSourceIdType(id));
+}
+
 }  // namespace ukm

components/ukm/ukm_service_unittest.cc

@@ -55,6 +55,10 @@ std::string Entry1And2Whitelist() {
   return std::string(TestEvent1::kEntryName) + ',' + TestEvent2::kEntryName;
 }
 
+SourceId ConvertSourceIdToWhitelistedType(SourceId id, SourceIdType type) {
+  return base::UkmSourceId::FromOtherId(id, type).ToInt64();
+}
+
 // A small shim exposing UkmRecorder methods to tests.
 class TestRecordingHelper {
  public:
@@ -1087,6 +1091,58 @@ TEST_F(UkmServiceTest, NonWhitelistedUrls) {
   }
 }
 
+TEST_F(UkmServiceTest, WhitelistIdType) {
+  ScopedUkmFeatureParams params({{"WhitelistEntries", Entry1And2Whitelist()}});
+
+  std::map<SourceIdType, bool> source_id_type_whitelisted = {
+      {SourceIdType::UKM, false},      {SourceIdType::NAVIGATION_ID, true},
+      {SourceIdType::APP_ID, true},    {SourceIdType::HISTORY_ID, true},
+      {SourceIdType::WEBAPK_ID, true},
+  };
+
+  for (std::pair<SourceIdType, bool> type : source_id_type_whitelisted) {
+    ClearPrefs();
+    UkmService service(&prefs_, &client_,
+                       true /* restrict_to_whitelisted_entries */,
+                       std::make_unique<MockDemographicMetricsProvider>());
+    TestRecordingHelper recorder(&service);
+    EXPECT_EQ(0, GetPersistedLogCount());
+    service.Initialize();
+    task_runner_->RunUntilIdle();
+    service.EnableRecording(/*extensions=*/false);
+    service.EnableReporting();
+
+    SourceId id = ConvertSourceIdToWhitelistedType(
+        1, static_cast<SourceIdType>(type.first));
+    ASSERT_EQ(GetSourceIdType(id), type.first);
+
+    recorder.UpdateSourceURL(id, GURL("https://google.com/foobar1"));
+
+    TestEvent1(id).Record(&service);
+
+    service.Flush();
+    EXPECT_EQ(1, GetPersistedLogCount());
+    Report proto_report = GetPersistedReport();
+
+    if (type.second) {
+      // Verify we've added one source.
+      EXPECT_EQ(1, proto_report.sources_size());
+      EXPECT_EQ(GURL("https://google.com/foobar1").spec(),
+                proto_report.sources(0).urls(0).url());
+    } else {
+      // No source added when id is not whitelisted type.
+      EXPECT_EQ(0, proto_report.sources_size());
+    }
+
+    // We've added the entry whether source is added or not.
+    ASSERT_EQ(1, proto_report.entries_size());
+    const Entry& proto_entry_a = proto_report.entries(0);
+    EXPECT_EQ(id, proto_entry_a.source_id());
+    EXPECT_EQ(base::HashMetricName(TestEvent1::kEntryName),
+              proto_entry_a.event_hash());
+  }
+}
+
 TEST_F(UkmServiceTest, SupportedSchemes) {
   struct {
     const char* url;
@@ -1334,12 +1390,15 @@ TEST_F(UkmServiceTest, PurgeNonNavigationSources) {
   // Seed some dummy sources.
   SourceId id0 = ConvertToSourceId(0, SourceIdType::UKM);
   recorder.UpdateSourceURL(id0, GURL("https://www.example0.com/"));
-  SourceId id1 = ConvertToSourceId(1, SourceIdType::NAVIGATION_ID);
+  SourceId id1 =
+      ConvertSourceIdToWhitelistedType(1, SourceIdType::NAVIGATION_ID);
   recorder.UpdateSourceURL(id1, GURL("https://www.example1.com/"));
-  SourceId id2 = ConvertToSourceId(2, SourceIdType::APP_ID);
+  SourceId id2 = ConvertSourceIdToWhitelistedType(2, SourceIdType::APP_ID);
   recorder.UpdateSourceURL(id2, GURL("https://www.example2.com/"));
-  SourceId id3 = ConvertToSourceId(3, SourceIdType::HISTORY_ID);
+  SourceId id3 = ConvertSourceIdToWhitelistedType(3, SourceIdType::HISTORY_ID);
   recorder.UpdateSourceURL(id3, GURL("https://www.example3.com/"));
+  SourceId id4 = ConvertSourceIdToWhitelistedType(4, SourceIdType::WEBAPK_ID);
+  recorder.UpdateSourceURL(id4, GURL("https://www.example3.com/"));
 
   service.Flush();
   int logs_count = 0;
@@ -1347,16 +1406,17 @@ TEST_F(UkmServiceTest, PurgeNonNavigationSources) {
 
   // All sources are present except id0 of non-whitelisted UKM type.
   Report proto_report = GetPersistedReport();
-  ASSERT_EQ(3, proto_report.sources_size());
+  ASSERT_EQ(4, proto_report.sources_size());
   EXPECT_EQ(id1, proto_report.sources(0).id());
   EXPECT_EQ(id2, proto_report.sources(1).id());
   EXPECT_EQ(id3, proto_report.sources(2).id());
+  EXPECT_EQ(id4, proto_report.sources(3).id());
 
   service.Flush();
   EXPECT_EQ(++logs_count, GetPersistedLogCount());
 
-  // Sources of APP_ID and HISTORY_ID types are not kept between reporting
-  // cycles, thus only 1 navigation type source remains.
+  // Sources of APP_ID, HISTORY_ID and WEBAPK_ID types are not kept between
+  // reporting cycles, thus only 1 navigation type source remains.
   proto_report = GetPersistedReport();
   ASSERT_EQ(1, proto_report.sources_size());
   EXPECT_EQ(id1, proto_report.sources(0).id());

services/metrics/public/cpp/ukm_recorder.cc

@@ -31,6 +31,17 @@ ukm::SourceId UkmRecorder::GetNewSourceID() {
   return AssignNewSourceId();
 }
 
+// static
+ukm::SourceId UkmRecorder::GetSourceIdForWebApkManifestUrl(
+    const GURL& manifest_url) {
+  ukm::SourceId source_id =
+      base::UkmSourceId::FromOtherId(GetNewSourceID(), SourceIdType::WEBAPK_ID)
+          .ToInt64();
+  ukm::UkmRecorder* ukm_recorder = ukm::UkmRecorder::Get();
+  ukm_recorder->UpdateSourceURL(source_id, manifest_url);
+  return source_id;
+}
+
 void UkmRecorder::RecordOtherURL(base::UkmSourceId source_id, const GURL& url) {
   UpdateSourceURL(source_id.ToInt64(), url);
 }

services/metrics/public/cpp/ukm_recorder.h

@@ -68,6 +68,10 @@ class METRICS_EXPORT UkmRecorder {
   void RecordOtherURL(base::UkmSourceId source_id, const GURL& url);
   void RecordAppURL(base::UkmSourceId source_id, const GURL& url);
 
+  // Gets new source Id for WEBAPK_ID type and updates the manifest url. This
+  // method should only be called by WebApkUkmRecorder class.
+  static SourceId GetSourceIdForWebApkManifestUrl(const GURL& manifest_url);
+
  private:
   friend DelegatingUkmRecorder;
   friend TestRecordingHelper;
@@ -79,8 +83,8 @@ class METRICS_EXPORT UkmRecorder {
   // WebApkUkmRecorder records metrics about installed Webapps. Instead of using
   // the current main frame URL, we want to record the URL of the Webapp
   // manifest which identifies the current app. Therefore, WebApkUkmRecorder
-  // needs to be a friend so that it can access the private UpdateSourceURL()
-  // method.
+  // needs to be a friend so that it can access the private
+  // GetSourceIdForWebApkManifestUrl() method.
   friend WebApkUkmRecorder;
 
   // Associates the SourceId with a URL. Most UKM recording code should prefer

services/metrics/public/cpp/ukm_source_id.cc

@@ -15,6 +15,11 @@ SourceId AssignNewSourceId() {
 }
 
 SourceId ConvertToSourceId(int64_t other_id, SourceIdType id_type) {
+  // DCHECK is to restrict the usage of WEBAPK_ID, WebApk should use
+  // |UkmRecorder::GetSourceIdForWebApkManifestUrl()| instead.
+  // TODO(crbug.com/1046964): Ideally we should restrict
+  // UkmSourceId::FromOtherId() as well.
+  DCHECK(id_type != SourceIdType::WEBAPK_ID);
   return base::UkmSourceId::FromOtherId(other_id, id_type).ToInt64();
 }