Skip to content

Commit

Permalink
[Topics] fix crashes in BrowsingTopicsCalculator
Browse files Browse the repository at this point in the history
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.

(cherry picked from commit 2a7e70e)

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-Original-Commit-Position: refs/heads/main@{#997765}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3621681
Auto-Submit: Yao Xiao <yaoxia@chromium.org>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/branch-heads/5005@{#361}
Cr-Branched-From: 5b4d945-refs/heads/main@{#992738}
  • Loading branch information
yaoxiachromium authored and Chromium LUCI CQ committed May 2, 2022
1 parent 6440ab3 commit 2b5cc2e
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 6 deletions.
17 changes: 11 additions & 6 deletions components/browsing_topics/browsing_topics_calculator.cc
Expand Up @@ -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());
Expand Down Expand Up @@ -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;
}
Expand Down
28 changes: 28 additions & 0 deletions components/browsing_topics/browsing_topics_calculator_unittest.cc
Expand Up @@ -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();
Expand Down

0 comments on commit 2b5cc2e

Please sign in to comment.