topics: Fix Model Update Race Condition
When a model update comes in, it will trigger any pending calculator
requests to start. This will happen during the execution of
`AnnotatorImpl::OnModelUpdated` in between `is_valid_model_` being set
to false and later to true.
Note that if the annotations request finishes in the same stack
(e.g.: all use the override list) then this race condition will always
happen.
To resolve this, the added check of `is_valid_model_` is removed. This
check is redundant anyways since the calculator also checks whether the
incoming taxonomy version is supported, and whether `GetModelInfo()`
returns a non-null value has it's source of truth further down in the
OptGuide model execution code, which is doing the correct thing.
Furthermore, the AnnotatorImpl class takes over the model available
callbacks to check for the correct taxonomy version.
Adds a regression test
Bug: 1495959
Change-Id: I29f24a81c6af15ebc8d41d302cb1a1865371b415
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4977545
Commit-Queue: Robert Ogden <[email protected]>
Reviewed-by: Yao Xiao <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1215821}
diff --git a/components/browsing_topics/annotator_impl.cc b/components/browsing_topics/annotator_impl.cc
index b4ed590..28f7645 100644
--- a/components/browsing_topics/annotator_impl.cc
+++ b/components/browsing_topics/annotator_impl.cc
@@ -6,6 +6,7 @@
#include "base/barrier_closure.h"
#include "base/containers/contains.h"
+#include "base/dcheck_is_on.h"
#include "base/files/file_util.h"
#include "base/ranges/algorithm.h"
#include "base/strings/string_number_conversions.h"
@@ -162,14 +163,26 @@
AnnotatorImpl::~AnnotatorImpl() = default;
void AnnotatorImpl::NotifyWhenModelAvailable(base::OnceClosure callback) {
- AddOnModelUpdatedCallback(std::move(callback));
+ if (GetBrowsingTopicsModelInfo().has_value()) {
+ std::move(callback).Run();
+ return;
+ }
+ model_available_callbacks_.AddUnsafe(std::move(callback));
}
absl::optional<optimization_guide::ModelInfo>
AnnotatorImpl::GetBrowsingTopicsModelInfo() const {
- if (!is_valid_model_) {
- return absl::nullopt;
+#if DCHECK_IS_ON()
+ if (GetModelInfo()) {
+ DCHECK(GetModelInfo()->GetModelMetadata());
+ absl::optional<optimization_guide::proto::PageTopicsModelMetadata>
+ model_metadata = optimization_guide::ParsedAnyMetadata<
+ optimization_guide::proto::PageTopicsModelMetadata>(
+ *GetModelInfo()->GetModelMetadata());
+ DCHECK(model_metadata);
+ DCHECK(IsModelTaxonomyVersionSupported(model_metadata->taxonomy_version()));
}
+#endif // DCHECK_IS_ON()
return GetModelInfo();
}
@@ -451,7 +464,6 @@
// First invoke parent to update internal status.
optimization_guide::BertModelHandler::OnModelUpdated(optimization_target,
model_info);
- is_valid_model_ = false;
if (optimization_target !=
optimization_guide::proto::OPTIMIZATION_TARGET_PAGE_TOPICS_V2) {
@@ -466,21 +478,23 @@
model_metadata = optimization_guide::ParsedAnyMetadata<
optimization_guide::proto::PageTopicsModelMetadata>(
*model_info->GetModelMetadata());
-
- if (!model_metadata ||
- !IsModelTaxonomyVersionSupported(model_metadata->taxonomy_version())) {
+ if (!model_metadata) {
return;
}
+ if (!IsModelTaxonomyVersionSupported(model_metadata->taxonomy_version())) {
+ // Also clear the model in the underlying model executor code so that it
+ // cannot be accidentally called on the wrong taxonomy version.
+ optimization_guide::BertModelHandler::OnModelUpdated(
+ optimization_guide::proto::OPTIMIZATION_TARGET_PAGE_TOPICS_V2,
+ absl::nullopt);
+ return;
+ }
+ version_ = model_metadata->version();
+
// New model, new override list.
- is_valid_model_ = true;
override_list_file_path_ = absl::nullopt;
override_list_ = absl::nullopt;
-
- if (model_metadata) {
- version_ = model_metadata->version();
- }
-
for (const base::FilePath& path : model_info->GetAdditionalFiles()) {
DCHECK(path.IsAbsolute());
if (path.BaseName() == base::FilePath(kOverrideListBasePath)) {
@@ -488,6 +502,13 @@
break;
}
}
+
+ // Run any callbacks that were waiting for an updated model.
+ //
+ // This should always be the last statement in this method, after all internal
+ // state has been updated because these callbacks may trigger an immediate
+ // annotation request.
+ model_available_callbacks_.Notify();
}
} // namespace browsing_topics
diff --git a/components/browsing_topics/annotator_impl.h b/components/browsing_topics/annotator_impl.h
index 4cc8820..4689819 100644
--- a/components/browsing_topics/annotator_impl.h
+++ b/components/browsing_topics/annotator_impl.h
@@ -9,6 +9,7 @@
#include <unordered_map>
#include <vector>
+#include "base/callback_list.h"
#include "base/files/file_path.h"
#include "base/functional/callback.h"
#include "base/memory/weak_ptr.h"
@@ -141,9 +142,9 @@
// memory.
size_t in_progess_batches_ = 0;
- // Indicates whether the model received was valid. Model will be invalid when
- // metadata versions are unsupported.
- bool is_valid_model_ = false;
+ // Callbacks that are run when the model is updated with the correct taxonomy
+ // version.
+ base::OnceClosureList model_available_callbacks_;
SEQUENCE_CHECKER(sequence_checker_);
diff --git a/components/browsing_topics/annotator_impl_unittest.cc b/components/browsing_topics/annotator_impl_unittest.cc
index fd7998c..47c4979 100644
--- a/components/browsing_topics/annotator_impl_unittest.cc
+++ b/components/browsing_topics/annotator_impl_unittest.cc
@@ -460,56 +460,6 @@
}
TEST_F(BrowsingTopicsAnnotatorImplTest,
- DifferentTaxonomyVersions_ModelUpdateSkipped) {
- scoped_feature_list_.Reset();
- scoped_feature_list_.InitWithFeaturesAndParameters(
- /*enabled_features=*/
- {{blink::features::kBrowsingTopicsParameters,
- {{"taxonomy_version", "2"}}}},
- /*disabled_features=*/{
- optimization_guide::features::kPreventLongRunningPredictionModels});
-
- optimization_guide::proto::PageTopicsModelMetadata model_metadata;
- model_metadata.set_taxonomy_version(1);
-
- optimization_guide::proto::Any any_metadata;
- any_metadata.set_type_url(
- "type.googleapis.com/com.foo.PageTopicsModelMetadata");
- model_metadata.SerializeToString(any_metadata.mutable_value());
-
- SendModelToAnnotatorSkipWaiting(any_metadata);
-
- absl::optional<optimization_guide::ModelInfo> model_info =
- annotator()->GetBrowsingTopicsModelInfo();
- EXPECT_FALSE(model_info);
-}
-
-TEST_F(BrowsingTopicsAnnotatorImplTest,
- TaxonomyConfiguredVersion1ServerVersion1_ModelUpdateSkipped) {
- scoped_feature_list_.Reset();
- scoped_feature_list_.InitWithFeaturesAndParameters(
- /*enabled_features=*/
- {{blink::features::kBrowsingTopicsParameters,
- {{"taxonomy_version", "1"}}}},
- /*disabled_features=*/{
- optimization_guide::features::kPreventLongRunningPredictionModels});
-
- optimization_guide::proto::PageTopicsModelMetadata model_metadata;
- model_metadata.set_taxonomy_version(1);
-
- optimization_guide::proto::Any any_metadata;
- any_metadata.set_type_url(
- "type.googleapis.com/com.foo.PageTopicsModelMetadata");
- model_metadata.SerializeToString(any_metadata.mutable_value());
-
- SendModelToAnnotatorSkipWaiting(any_metadata);
-
- absl::optional<optimization_guide::ModelInfo> model_info =
- annotator()->GetBrowsingTopicsModelInfo();
- EXPECT_FALSE(model_info);
-}
-
-TEST_F(BrowsingTopicsAnnotatorImplTest,
TaxonomyConfiguredVersion1ServerVersionEmpty_ModelUpdateSuccess) {
scoped_feature_list_.Reset();
scoped_feature_list_.InitWithFeaturesAndParameters(
@@ -533,30 +483,6 @@
EXPECT_TRUE(model_info);
}
-TEST_F(BrowsingTopicsAnnotatorImplTest,
- TaxonomyConfiguredVersion2ServerVersionEmpty_ModelUpdateSkipped) {
- scoped_feature_list_.Reset();
- scoped_feature_list_.InitWithFeaturesAndParameters(
- /*enabled_features=*/
- {{blink::features::kBrowsingTopicsParameters,
- {{"taxonomy_version", "2"}}}},
- /*disabled_features=*/{
- optimization_guide::features::kPreventLongRunningPredictionModels});
-
- optimization_guide::proto::PageTopicsModelMetadata model_metadata;
-
- optimization_guide::proto::Any any_metadata;
- any_metadata.set_type_url(
- "type.googleapis.com/com.foo.PageTopicsModelMetadata");
- model_metadata.SerializeToString(any_metadata.mutable_value());
-
- SendModelToAnnotatorSkipWaiting(any_metadata);
-
- absl::optional<optimization_guide::ModelInfo> model_info =
- annotator()->GetBrowsingTopicsModelInfo();
- EXPECT_FALSE(model_info);
-}
-
class BrowsingTopicsAnnotatorOverrideListTest
: public BrowsingTopicsAnnotatorImplTest {
public:
diff --git a/components/browsing_topics/browsing_topics_calculator_unittest.cc b/components/browsing_topics/browsing_topics_calculator_unittest.cc
index 2383e0fe..5aa0dd7 100644
--- a/components/browsing_topics/browsing_topics_calculator_unittest.cc
+++ b/components/browsing_topics/browsing_topics_calculator_unittest.cc
@@ -290,6 +290,49 @@
/*expected_bucket_count=*/2);
}
+// Regression test for crbug/1495959.
+TEST_F(BrowsingTopicsCalculatorTest, ModelAvailableAfterDelay) {
+ test_annotator_.SetModelAvailable(false);
+
+ base::Time begin_time = base::Time::Now();
+
+ AddHistoryEntries({kHost1, kHost2, kHost3, kHost4, kHost5, kHost6},
+ begin_time);
+
+ task_environment_.AdvanceClock(base::Seconds(1));
+
+ // This PostTask will run when the |CalculateTopics| run loop starts and will
+ // signal to the calculator that the model is ready, triggering it to start.
+ task_environment_.GetMainThreadTaskRunner()->PostTask(
+ FROM_HERE,
+ base::BindOnce(
+ [](TestAnnotator* annotator) {
+ annotator->UseModelInfo(*optimization_guide::TestModelInfoBuilder()
+ .SetVersion(1)
+ .Build());
+ annotator->UseAnnotations({
+ {kHost1, {1, 2, 3, 4, 5, 6}},
+ {kHost2, {2, 3, 4, 5, 6}},
+ {kHost3, {3, 4, 5, 6}},
+ {kHost4, {4, 5, 6}},
+ {kHost5, {5, 6}},
+ {kHost6, {6}},
+ });
+ annotator->SetModelAvailable(true);
+ },
+ &test_annotator_));
+
+ EpochTopics result = CalculateTopics();
+ ExpectResultTopicsEqual(result.top_topics_and_observing_domains(),
+ {{Topic(6), {}},
+ {Topic(5), {}},
+ {Topic(4), {}},
+ {Topic(3), {}},
+ {Topic(2), {}}});
+
+ EXPECT_EQ(result.padded_top_topics_start_index(), 5u);
+}
+
TEST_F(BrowsingTopicsCalculatorTest, TopTopicsRankedByFrequency) {
base::Time begin_time = base::Time::Now();
diff --git a/components/browsing_topics/test_util.cc b/components/browsing_topics/test_util.cc
index 80e8216f..eaebccf 100644
--- a/components/browsing_topics/test_util.cc
+++ b/components/browsing_topics/test_util.cc
@@ -248,6 +248,13 @@
model_info_ = model_info;
}
+void TestAnnotator::SetModelAvailable(bool model_available) {
+ model_available_ = model_available;
+ if (model_available_) {
+ model_available_callbacks_.Notify();
+ }
+}
+
void TestAnnotator::BatchAnnotate(BatchAnnotationCallback callback,
const std::vector<std::string>& inputs) {
std::vector<Annotation> annotations;
@@ -265,7 +272,10 @@
}
void TestAnnotator::NotifyWhenModelAvailable(base::OnceClosure callback) {
- // Always run the callback so that tests do not hang.
+ if (!model_available_) {
+ model_available_callbacks_.AddUnsafe(std::move(callback));
+ return;
+ }
std::move(callback).Run();
}
diff --git a/components/browsing_topics/test_util.h b/components/browsing_topics/test_util.h
index 54cd303..300af26 100644
--- a/components/browsing_topics/test_util.h
+++ b/components/browsing_topics/test_util.h
@@ -7,6 +7,7 @@
#include "base/containers/queue.h"
+#include "base/callback_list.h"
#include "base/memory/weak_ptr.h"
#include "base/time/time.h"
#include "components/browsing_topics/annotator.h"
@@ -151,6 +152,10 @@
void UseModelInfo(
const absl::optional<optimization_guide::ModelInfo>& model_info);
+ // If setting to true when it had been false, all callbacks that have been
+ // passed to |NotifyWhenModelAvailable| will be ran.
+ void SetModelAvailable(bool is_available);
+
// Annotator:
void BatchAnnotate(BatchAnnotationCallback callback,
const std::vector<std::string>& inputs) override;
@@ -161,6 +166,8 @@
private:
std::map<std::string, std::set<int32_t>> annotations_;
absl::optional<optimization_guide::ModelInfo> model_info_;
+ bool model_available_ = true;
+ base::OnceClosureList model_available_callbacks_;
};
} // namespace browsing_topics
