[Topics] Block descendant topics when a topic is blocked

Anytime a list of blocked topics is retrieved, it still just lists the explicitly blocked topics, not the implicitly blocked topics (descendants of explicitly blocked topics). Allowing (unblocking) a topic only allows its descendants that are not explicitly blocked and are not implicitly blocked due to another ancestor.

In the Ad topics setting page, when a topic is blocked, its descendants remain in the list of top topics until the page is reloaded.

Change-Id: I38e055b70a0f9db34db2d5dc72610eb01bdb9481
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4357086
Reviewed-by: Yao Xiao <yaoxia@chromium.org>
Commit-Queue: Abigail Katcoff <abigailkatcoff@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1122453}

components/browsing_topics/browsing_topics_service_impl.cc

@@ -5,6 +5,7 @@
 #include "components/browsing_topics/browsing_topics_service_impl.h"
 
 #include <random>
+#include <vector>
 
 #include "base/metrics/histogram_functions.h"
 #include "base/notreached.h"
@@ -18,6 +19,7 @@
 #include "components/browsing_topics/mojom/browsing_topics_internals.mojom.h"
 #include "components/browsing_topics/util.h"
 #include "components/optimization_guide/content/browser/page_content_annotations_service.h"
+#include "components/privacy_sandbox/canonical_topic.h"
 #include "content/public/browser/browsing_topics_site_data_manager.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "services/metrics/public/cpp/ukm_builders.h"
@@ -32,17 +34,10 @@ namespace {
 // Returns whether the topics should all be cleared given
 // `browsing_topics_data_accessible_since` and `is_topic_allowed_by_settings`.
 // Returns true if `browsing_topics_data_accessible_since` is greater than the
-// last calculation time, or if any top topic is disallowed from the settings.
-// The latter could happen if the topic became disallowed when
-// `browsing_topics_state` was still loading (and we didn't get a chance to
-// clear it). This is an unlikely edge case, so it's fine to over-delete.
+// last calculation time.
 bool ShouldClearTopicsOnStartup(
     const BrowsingTopicsState& browsing_topics_state,
-    base::Time browsing_topics_data_accessible_since,
-    base::RepeatingCallback<bool(const privacy_sandbox::CanonicalTopic&)>
-        is_topic_allowed_by_settings) {
-  DCHECK(!is_topic_allowed_by_settings.is_null());
-
+    base::Time browsing_topics_data_accessible_since) {
   if (browsing_topics_state.epochs().empty())
     return false;
 
@@ -56,25 +51,39 @@ bool ShouldClearTopicsOnStartup(
     return true;
   }
 
+  return false;
+}
+
+// Returns a vector of top topics which are disallowed and thus should be
+// cleared. This could happen if the topic became disallowed when
+// `browsing_topics_state` was still loading (and we didn't get a chance to
+// clear it).
+std::vector<privacy_sandbox::CanonicalTopic> TopTopicsToClearOnStartup(
+    const BrowsingTopicsState& browsing_topics_state,
+    base::RepeatingCallback<bool(const privacy_sandbox::CanonicalTopic&)>
+        is_topic_allowed_by_settings) {
+  DCHECK(!is_topic_allowed_by_settings.is_null());
+  std::vector<privacy_sandbox::CanonicalTopic> top_topics_to_clear;
   for (const EpochTopics& epoch : browsing_topics_state.epochs()) {
     for (const TopicAndDomains& topic_and_domains :
          epoch.top_topics_and_observing_domains()) {
       if (!topic_and_domains.IsValid())
         continue;
-
-      if (!is_topic_allowed_by_settings.Run(privacy_sandbox::CanonicalTopic(
-              topic_and_domains.topic(), epoch.taxonomy_version()))) {
-        return true;
+      privacy_sandbox::CanonicalTopic canonical_topic =
+          privacy_sandbox::CanonicalTopic(topic_and_domains.topic(),
+                                          epoch.taxonomy_version());
+      if (!is_topic_allowed_by_settings.Run(canonical_topic)) {
+        top_topics_to_clear.emplace_back(canonical_topic);
       }
     }
   }
-
-  return false;
+  return top_topics_to_clear;
 }
 
 struct StartupCalculateDecision {
-  bool clear_topics_data = true;
+  bool clear_all_topics_data = true;
   base::TimeDelta next_calculation_delay;
+  std::vector<privacy_sandbox::CanonicalTopic> topics_to_clear;
 };
 
 StartupCalculateDecision GetStartupCalculationDecision(
@@ -87,17 +96,21 @@ StartupCalculateDecision GetStartupCalculationDecision(
   // topics should have already been cleared when initializing the
   // `BrowsingTopicsState`.
   if (browsing_topics_state.next_scheduled_calculation_time().is_null()) {
-    return StartupCalculateDecision{
-        .clear_topics_data = false,
-        .next_calculation_delay = base::TimeDelta()};
+    return StartupCalculateDecision{.clear_all_topics_data = false,
+                                    .next_calculation_delay = base::TimeDelta(),
+                                    .topics_to_clear = {}};
   }
 
   // This could happen when clear-on-exit is turned on and has caused the
-  // cookies to be deleted on startup, of if a topic became disallowed when
-  // `browsing_topics_state` was still loading.
-  bool should_clear_topics_data = ShouldClearTopicsOnStartup(
-      browsing_topics_state, browsing_topics_data_accessible_since,
-      is_topic_allowed_by_settings);
+  // cookies to be deleted on startup
+  bool should_clear_all_topics_data = ShouldClearTopicsOnStartup(
+      browsing_topics_state, browsing_topics_data_accessible_since);
+
+  std::vector<privacy_sandbox::CanonicalTopic> topics_to_clear;
+  if (!should_clear_all_topics_data) {
+    topics_to_clear = TopTopicsToClearOnStartup(browsing_topics_state,
+                                                is_topic_allowed_by_settings);
+  }
 
   base::TimeDelta presumed_next_calculation_delay =
       browsing_topics_state.next_scheduled_calculation_time() -
@@ -106,8 +119,9 @@ StartupCalculateDecision GetStartupCalculationDecision(
   // The scheduled calculation time was reached before the startup.
   if (presumed_next_calculation_delay <= base::TimeDelta()) {
     return StartupCalculateDecision{
-        .clear_topics_data = should_clear_topics_data,
-        .next_calculation_delay = base::TimeDelta()};
+        .clear_all_topics_data = should_clear_all_topics_data,
+        .next_calculation_delay = base::TimeDelta(),
+        .topics_to_clear = topics_to_clear};
   }
 
   // This could happen if the machine time has changed since the last
@@ -116,13 +130,15 @@ StartupCalculateDecision GetStartupCalculationDecision(
   if (presumed_next_calculation_delay >=
       2 * blink::features::kBrowsingTopicsTimePeriodPerEpoch.Get()) {
     return StartupCalculateDecision{
-        .clear_topics_data = should_clear_topics_data,
-        .next_calculation_delay = base::TimeDelta()};
+        .clear_all_topics_data = should_clear_all_topics_data,
+        .next_calculation_delay = base::TimeDelta(),
+        .topics_to_clear = topics_to_clear};
   }
 
   return StartupCalculateDecision{
-      .clear_topics_data = should_clear_topics_data,
-      .next_calculation_delay = presumed_next_calculation_delay};
+      .clear_all_topics_data = should_clear_all_topics_data,
+      .next_calculation_delay = presumed_next_calculation_delay,
+      .topics_to_clear = topics_to_clear};
 }
 
 void RecordBrowsingTopicsApiResultMetrics(ApiAccessResult result,
@@ -601,8 +617,15 @@ void BrowsingTopicsServiceImpl::OnBrowsingTopicsStateLoaded() {
           &privacy_sandbox::PrivacySandboxSettings::IsTopicAllowed,
           base::Unretained(privacy_sandbox_settings_)));
 
-  if (decision.clear_topics_data)
+  if (decision.clear_all_topics_data) {
     browsing_topics_state_.ClearAllTopics();
+  } else if (!decision.topics_to_clear.empty()) {
+    for (const privacy_sandbox::CanonicalTopic& canonical_topic :
+         decision.topics_to_clear) {
+      browsing_topics_state_.ClearTopic(canonical_topic.topic_id(),
+                                        canonical_topic.taxonomy_version());
+    }
+  }
 
   site_data_manager_->ExpireDataBefore(browsing_topics_data_sccessible_since);
 

components/browsing_topics/browsing_topics_service_impl_unittest.cc

@@ -1863,22 +1863,22 @@ TEST_F(BrowsingTopicsServiceImplTest, ClearTopic) {
   // Finish file loading and two calculations.
   task_environment()->FastForwardBy(2 * kCalculatorDelay + kEpoch);
 
+  // Clearing topic 7 should clear child topic 8 as well.
   browsing_topics_service_->ClearTopic(
-      privacy_sandbox::CanonicalTopic(Topic(3), /*taxonomy_version=*/1));
+      privacy_sandbox::CanonicalTopic(Topic(7), /*taxonomy_version=*/1));
 
   std::vector<privacy_sandbox::CanonicalTopic> result =
       browsing_topics_service_->GetTopTopicsForDisplay();
 
-  EXPECT_EQ(result.size(), 9u);
+  EXPECT_EQ(result.size(), 8u);
   EXPECT_EQ(result[0].topic_id(), Topic(1));
   EXPECT_EQ(result[1].topic_id(), Topic(2));
-  EXPECT_EQ(result[2].topic_id(), Topic(4));
-  EXPECT_EQ(result[3].topic_id(), Topic(5));
-  EXPECT_EQ(result[4].topic_id(), Topic(6));
-  EXPECT_EQ(result[5].topic_id(), Topic(7));
-  EXPECT_EQ(result[6].topic_id(), Topic(8));
-  EXPECT_EQ(result[7].topic_id(), Topic(9));
-  EXPECT_EQ(result[8].topic_id(), Topic(10));
+  EXPECT_EQ(result[2].topic_id(), Topic(3));
+  EXPECT_EQ(result[3].topic_id(), Topic(4));
+  EXPECT_EQ(result[4].topic_id(), Topic(5));
+  EXPECT_EQ(result[5].topic_id(), Topic(6));
+  EXPECT_EQ(result[6].topic_id(), Topic(9));
+  EXPECT_EQ(result[7].topic_id(), Topic(10));
 }
 
 TEST_F(BrowsingTopicsServiceImplTest, ClearTopicBeforeLoadFinish) {
@@ -1911,11 +1911,15 @@ TEST_F(BrowsingTopicsServiceImplTest, ClearTopicBeforeLoadFinish) {
   // Finish file loading.
   task_environment()->RunUntilIdle();
 
-  // If a topic in the settings is cleared before load finish, all pre-existing
-  // topics data will be cleared in the `BrowsingTopicsState` after load finish.
+  // If a topic in the settings is cleared before load finish,
+  // that topic and its descendants will be cleared after load finish.
   std::vector<privacy_sandbox::CanonicalTopic> result =
       browsing_topics_service_->GetTopTopicsForDisplay();
-  EXPECT_EQ(result.size(), 0u);
+  EXPECT_EQ(result.size(), 4u);
+  EXPECT_EQ(result[0].topic_id(), Topic(1));
+  EXPECT_EQ(result[1].topic_id(), Topic(2));
+  EXPECT_EQ(result[2].topic_id(), Topic(4));
+  EXPECT_EQ(result[3].topic_id(), Topic(5));
 }
 
 TEST_F(BrowsingTopicsServiceImplTest, ClearAllTopicsData) {

components/browsing_topics/common/semantic_tree.cc

@@ -4,6 +4,8 @@
 
 #include "components/browsing_topics/common/semantic_tree.h"
 
+#include <vector>
+
 #include "base/containers/fixed_flat_map.h"
 #include "base/no_destructor.h"
 #include "components/strings/grit/components_strings.h"
@@ -1845,6 +1847,23 @@ std::set<Topic> SemanticTree::GetDescendantTopics(const Topic& topic) {
   return descendant_topics;
 }
 
+std::vector<Topic> SemanticTree::GetAncestorTopics(const Topic& topic) {
+  std::vector<Topic> ancestor_topics;
+  base::fixed_flat_map<Topic, SemanticTreeNodeInformation,
+                       kSemanticTreeSize>::const_iterator tree_node_it =
+      GetSemanticTreeInternal().find(topic);
+  while (tree_node_it != GetSemanticTreeInternal().end()) {
+    const SemanticTreeNodeInformation& node_info = tree_node_it->second;
+    if (!node_info.parent_topic.has_value()) {
+      break;
+    }
+    ancestor_topics.emplace_back(node_info.parent_topic.value());
+    tree_node_it =
+        GetSemanticTreeInternal().find(node_info.parent_topic.value());
+  }
+  return ancestor_topics;
+}
+
 absl::optional<int> SemanticTree::GetLocalizedNameMessageId(
     const Topic& topic,
     int taxonomy_version) {

components/browsing_topics/common/semantic_tree.h

@@ -24,6 +24,7 @@ class COMPONENT_EXPORT(BROWSING_TOPICS_COMMON) SemanticTree {
   SemanticTree(const SemanticTree& other) = delete;
   ~SemanticTree();
   std::set<Topic> GetDescendantTopics(const Topic& topic);
+  std::vector<Topic> GetAncestorTopics(const Topic& topic);
   absl::optional<int> GetLocalizedNameMessageId(const Topic& topic,
                                                 int taxonomy_version);
 

components/browsing_topics/common/semantic_tree_unittest.cc

@@ -48,6 +48,30 @@ TEST_F(SemanticTreeUnittest, GetDescendantTopicsMultipleLevelsOfDescendants) {
   EXPECT_EQ(topics, expected_descendants);
 }
 
+TEST_F(SemanticTreeUnittest, GetAncestorTopicsTopicNotInMap) {
+  std::vector<Topic> topics = semantic_tree_.GetAncestorTopics(Topic(10000));
+  EXPECT_TRUE(topics.empty());
+}
+
+TEST_F(SemanticTreeUnittest, GetAncestorTopicsNoAncestors) {
+  std::vector<Topic> topics = semantic_tree_.GetAncestorTopics(Topic(1));
+  EXPECT_TRUE(topics.empty());
+}
+
+TEST_F(SemanticTreeUnittest, GetAncestorTopicsOneAncestor) {
+  std::vector<Topic> topics = semantic_tree_.GetAncestorTopics(Topic(2));
+  EXPECT_FALSE(topics.empty());
+  std::vector<Topic> expected_ancestors = {Topic(1)};
+  EXPECT_EQ(topics, expected_ancestors);
+}
+
+TEST_F(SemanticTreeUnittest, GetAncestorTopicsMultipleAncestors) {
+  std::vector<Topic> topics = semantic_tree_.GetAncestorTopics(Topic(36));
+  EXPECT_FALSE(topics.empty());
+  std::vector<Topic> expected_ancestors = {Topic(33), Topic(23), Topic(1)};
+  EXPECT_EQ(topics, expected_ancestors);
+}
+
 TEST_F(SemanticTreeUnittest, GetLocalizedNameMessageIdValidTopicAndTaxonomy) {
   absl::optional<int> message_id =
       semantic_tree_.GetLocalizedNameMessageId(Topic(100), 1);

components/browsing_topics/epoch_topics.cc

@@ -4,10 +4,12 @@
 
 #include "components/browsing_topics/epoch_topics.h"
 
+#include "base/containers/contains.h"
 #include "base/hash/legacy_hash.h"
 #include "base/json/values_util.h"
 #include "base/logging.h"
 #include "base/numerics/checked_math.h"
+#include "components/browsing_topics/common/semantic_tree.h"
 #include "components/browsing_topics/util.h"
 #include "third_party/blink/public/common/features.h"
 #include "url/gurl.h"
@@ -209,15 +211,22 @@ void EpochTopics::ClearTopics() {
 }
 
 void EpochTopics::ClearTopic(Topic topic) {
-  for (TopicAndDomains& topic_and_domains : top_topics_and_observing_domains_) {
-    if (topic_and_domains.topic() != topic)
-      continue;
-
+  for (TopicAndDomains& top_topic_and_domains :
+       top_topics_and_observing_domains_) {
     // Invalidate `topic_and_domains`. We cannot delete the entry from
     // `top_topics_and_observing_domains_` because it would modify the list of
     // topics, and would break the ability to return the same topic for the same
     // site for the epoch .
-    topic_and_domains = TopicAndDomains();
+    if (top_topic_and_domains.topic() == topic) {
+      top_topic_and_domains = TopicAndDomains();
+      continue;
+    }
+    SemanticTree semantic_tree;
+    std::vector<Topic> top_topic_ancestors =
+        semantic_tree.GetAncestorTopics(top_topic_and_domains.topic());
+    if (base::Contains(top_topic_ancestors, topic)) {
+      top_topic_and_domains = TopicAndDomains();
+    }
   }
 }
 

components/browsing_topics/epoch_topics.h

@@ -60,7 +60,9 @@ class EpochTopics {
   // reset `padded_top_topics_start_index_` to 0.
   void ClearTopics();
 
-  // Clear an entry in `top_topics_and_observing_domains_` that matches `topic`.
+  // Clear an entry in `top_topics_and_observing_domains_` that matches `topic`
+  // and any entry in `top_topics_and_observing_domains_` that is a topic
+  // descended from `topic`.
   void ClearTopic(Topic topic);
 
   // Clear the domains in `top_topics_and_observing_domains_`  that match

components/browsing_topics/epoch_topics_unittest.cc

@@ -31,7 +31,7 @@ EpochTopics CreateTestEpochTopics() {
   top_topics_and_observing_domains.emplace_back(
       TopicAndDomains(Topic(4), {HashedDomain(2), HashedDomain(3)}));
   top_topics_and_observing_domains.emplace_back(
-      TopicAndDomains(Topic(5), {HashedDomain(1)}));
+      TopicAndDomains(Topic(100), {HashedDomain(1)}));
 
   EpochTopics epoch_topics(std::move(top_topics_and_observing_domains),
                            kPaddedTopTopicsStartIndex, kTaxonomySize,
@@ -222,7 +222,7 @@ TEST_F(EpochTopicsTest, ClearTopics) {
   EXPECT_FALSE(candidate_topic.IsValid());
 }
 
-TEST_F(EpochTopicsTest, ClearTopic) {
+TEST_F(EpochTopicsTest, ClearTopic_NoDescendants) {
   EpochTopics epoch_topics = CreateTestEpochTopics();
 
   EXPECT_FALSE(epoch_topics.empty());
@@ -238,6 +238,22 @@ TEST_F(EpochTopicsTest, ClearTopic) {
   EXPECT_TRUE(epoch_topics.top_topics_and_observing_domains()[4].IsValid());
 }
 
+TEST_F(EpochTopicsTest, ClearTopic_WithDescendants) {
+  EpochTopics epoch_topics = CreateTestEpochTopics();
+
+  EXPECT_FALSE(epoch_topics.empty());
+
+  epoch_topics.ClearTopic(Topic(1));
+
+  EXPECT_FALSE(epoch_topics.empty());
+
+  EXPECT_FALSE(epoch_topics.top_topics_and_observing_domains()[0].IsValid());
+  EXPECT_FALSE(epoch_topics.top_topics_and_observing_domains()[1].IsValid());
+  EXPECT_FALSE(epoch_topics.top_topics_and_observing_domains()[2].IsValid());
+  EXPECT_FALSE(epoch_topics.top_topics_and_observing_domains()[3].IsValid());
+  EXPECT_TRUE(epoch_topics.top_topics_and_observing_domains()[4].IsValid());
+}
+
 TEST_F(EpochTopicsTest, ClearContextDomain) {
   EpochTopics epoch_topics = CreateTestEpochTopics();