From 2a7e70efd690765464426a9f72e786db18fd752a Mon Sep 17 00:00:00 2001
From: Yao Xiao <yaoxia@chromium.org>
Date: Fri, 29 Apr 2022 17:11:30 +0000
Subject: [PATCH] [Topics] fix crashes in BrowsingTopicsCalculator

Fix 2 issues:
- When the classification succeeded in general, the specific topics for
host (BatchAnnotationResult::topics()) could still be nullopt. Adding
the check for nullopt.
- We should not use host_topics_map.at(host) as there could be no topics
for a host.

Bug: 1321140
Change-Id: I04bd26f79fa0d447109f8864e4ece07be16d6d5f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3615972
Commit-Queue: Yao Xiao <yaoxia@chromium.org>
Reviewed-by: Josh Karlin <jkarlin@chromium.org>
Cr-Commit-Position: refs/heads/main@{#997765}
---
 .../browsing_topics_calculator.cc             | 17 +++++++----
 .../browsing_topics_calculator_unittest.cc    | 28 +++++++++++++++++++
 2 files changed, 39 insertions(+), 6 deletions(-)

diff --git a/components/browsing_topics/browsing_topics_calculator.cc b/components/browsing_topics/browsing_topics_calculator.cc
index a31613a1c3cbcb..0add266b3d544f 100644
--- a/components/browsing_topics/browsing_topics_calculator.cc
+++ b/components/browsing_topics/browsing_topics_calculator.cc
@@ -77,15 +77,15 @@ void DeriveHostTopicsMapAndTopicHostsMap(
     const optimization_guide::BatchAnnotationResult& result = results[i];
     const std::string raw_host = raw_hosts[i];
 
-    // As long as the annotation didn't fail in general, the individual
-    // `result.topics()` should always be valid.
-    const std::vector<optimization_guide::WeightedIdentifier>&
-        annotation_result_topics = result.topics().value();
+    const absl::optional<std::vector<optimization_guide::WeightedIdentifier>>&
+        annotation_result_topics = result.topics();
+    if (!annotation_result_topics)
+      continue;
 
     HashedHost host = HashMainFrameHostForStorage(raw_host);
 
     for (const optimization_guide::WeightedIdentifier& annotation_result_topic :
-         annotation_result_topics) {
+         *annotation_result_topics) {
       // Note that `annotation_result_topic.weight()` is ignored. This is the
       // intended use of the model for the Topics API.
       Topic topic = Topic(annotation_result_topic.value());
@@ -187,7 +187,12 @@ void BrowsingTopicsCalculator::DeriveTopTopics(
   // topics (https://github.com/jkarlin/topics/issues/42).
   std::map<Topic, size_t> topics_count;
   for (auto const& [host, host_count] : history_hosts_count) {
-    const std::set<Topic>& topics = host_topics_map.at(host);
+    // A host wouldn't be found if there were no topics associated with it.
+    auto it = host_topics_map.find(host);
+    if (it == host_topics_map.end())
+      continue;
+
+    const std::set<Topic>& topics = it->second;
     for (const Topic& topic : topics) {
       topics_count[topic] += host_count;
     }
diff --git a/components/browsing_topics/browsing_topics_calculator_unittest.cc b/components/browsing_topics/browsing_topics_calculator_unittest.cc
index 434cdd00782991..7d9e38560e132d 100644
--- a/components/browsing_topics/browsing_topics_calculator_unittest.cc
+++ b/components/browsing_topics/browsing_topics_calculator_unittest.cc
@@ -343,6 +343,34 @@ TEST_F(BrowsingTopicsCalculatorTest, TopTopicsRankedByFrequency) {
   EXPECT_EQ(result.padded_top_topics_start_index(), 5u);
 }
 
+TEST_F(BrowsingTopicsCalculatorTest, ModelHasNoTopicsForHost) {
+  base::Time begin_time = base::Time::Now();
+
+  AddHistoryEntries({kHost1, kHost2, kHost3, kHost4, kHost5, kHost6},
+                    begin_time);
+
+  test_page_content_annotator_.UsePageTopics(
+      *optimization_guide::TestModelInfoBuilder().SetVersion(1).Build(),
+      {{kTokenizedHost1, {}},
+       {kTokenizedHost2, {}},
+       {kTokenizedHost3, {}},
+       {kTokenizedHost4, {}},
+       {kTokenizedHost5, {}},
+       {kTokenizedHost6, {}}});
+
+  task_environment_.AdvanceClock(base::Seconds(1));
+
+  EpochTopics result = CalculateTopics();
+  ExpectResultTopicsEqual(result.top_topics_and_observing_domains(),
+                          {{Topic(101), {}},
+                           {Topic(102), {}},
+                           {Topic(103), {}},
+                           {Topic(104), {}},
+                           {Topic(105), {}}});
+
+  EXPECT_EQ(result.padded_top_topics_start_index(), 0u);
+}
+
 TEST_F(BrowsingTopicsCalculatorTest,
        TopTopicsRankedByFrequency_AlsoAffectedByHostsCount) {
   base::Time begin_time = base::Time::Now();