Keep relevant UKM sources alive, follow-up

Follow-up to https://chromium-review.googlesource.com/c/chromium/src/+/1687059

Change-Id: Ifbe03a84c72d4ae473bad07d61c7b89984c70521
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1715411
Commit-Queue: Yue Ru Sun <yrsun@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Cr-Commit-Position: refs/heads/master@{#681355}

components/ukm/ukm_recorder_impl.cc

@@ -64,8 +64,9 @@ size_t GetMaxSources() {
       kUkmFeature, "MaxSources", kDefaultMaxSources));
 }
 
-// Gets the maximum number of Sources we can kept in memory to defer to the next
-// reporting interval at the end of the current reporting cycle.
+// Gets the maximum number of Sources we can keep in memory at the end of the
+// current reporting cycle that will stay accessible in the next reporting
+// interval.
 size_t GetMaxKeptSources() {
   constexpr size_t kDefaultMaxKeptSources = 100;
   return static_cast<size_t>(base::GetFieldTrialParamByFeatureAsInt(
@@ -295,9 +296,11 @@ void UkmRecorderImpl::StoreRecordingsInReport(Report* report) {
   std::unordered_map<ukm::SourceIdType, int> serialized_source_type_counts;
 
   for (const auto& kv : recordings_.sources) {
-    // Sources of non-navigation types will not be kept after current report.
-    if (GetSourceIdType(kv.first) != base::UkmSourceId::Type::NAVIGATION_ID) {
-      recordings_.obsolete_source_ids.insert(kv.first);
+    // 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) {
+      MarkSourceForDeletion(kv.first);
     }
     // If the source id is not whitelisted, don't send it unless it has
     // associated entries and the URL matches that of a whitelisted source.
@@ -316,6 +319,10 @@ void UkmRecorderImpl::StoreRecordingsInReport(Report* report) {
       // Omit entryless sources from the report.
       if (!base::Contains(source_ids_seen, kv.first)) {
         continue;
+      } else {
+        // Source of base::UkmSourceId::Type::UKM type will not be kept after
+        // entries are logged.
+        MarkSourceForDeletion(kv.first);
       }
     }
     Source* proto_source = report->add_sources();

components/ukm/ukm_service_unittest.cc

@@ -711,8 +711,10 @@ TEST_F(UkmServiceTest, UnreferencedNonWhitelistedSources) {
       // The one whitelisted source is of navigation type.
       EXPECT_EQ(1, proto_report.source_counts().navigation_sources());
       EXPECT_EQ(0, proto_report.source_counts().unmatched_sources());
-      // Only the navigation type source is deferred.
-      EXPECT_EQ(1, proto_report.source_counts().deferred_sources());
+      // Source 0 of navigation type, and entryless sources 1, 3, 4, 5 of
+      // non-whitelisted type are eligible to be deferred, but MaxKeptSources
+      // restricts deferral to the 3 latest created ones.
+      EXPECT_EQ(3, proto_report.source_counts().deferred_sources());
       EXPECT_EQ(0, proto_report.source_counts().carryover_sources());
 
       ASSERT_EQ(3, proto_report.sources_size());
@@ -756,12 +758,14 @@ TEST_F(UkmServiceTest, UnreferencedNonWhitelistedSources) {
       // Only the navigation type source is deferred.
       EXPECT_EQ(1, proto_report.source_counts().deferred_sources());
       // Number of sources carried over from the previous report to this report.
-      EXPECT_EQ(1, proto_report.source_counts().carryover_sources());
-      // Only the navigation source is again included in current report and
-      // there is a new entry associated to it.
-      ASSERT_EQ(1, proto_report.sources_size());
-      EXPECT_EQ(whitelisted_id, proto_report.sources(0).id());
+      EXPECT_EQ(3, proto_report.source_counts().carryover_sources());
+      // Out of sources 3, 4, 5 that were retained from the previous cycle,
+      // sources 3 and 4 got new entries are thus included in this report.
+      ASSERT_EQ(2, proto_report.sources_size());
+      EXPECT_EQ(ids[3], proto_report.sources(0).id());
       EXPECT_EQ(kURL.spec(), proto_report.sources(0).url());
+      EXPECT_EQ(ids[4], proto_report.sources(1).id());
+      EXPECT_EQ(kURL.spec(), proto_report.sources(1).url());
     }
   }
 }