Reland "[Cros SR] Reflect user history deletion actions in the model."

The original CL had a potential thread safety issue, which is fixed
in this CL. This also adds a sequence checker to RecurrenceRanker to
ensure this is fixed.

PS1 is the original CL. PS2 is the fix and sequence checker.

Original CL: crrev.com/c/1725032

Bug: 931149
Change-Id: Iaa5d87d38fa6a9bd5a5605b9feb9a5fd9154d3f0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1732269Reviewed-by: Jia Meng <jiameng@chromium.org>
Commit-Queue: Tony Yeoman <tby@chromium.org>
Cr-Commit-Position: refs/heads/master@{#683903}

chrome/browser/ui/app_list/search/search_controller.cc

@@ -18,6 +18,7 @@
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
+#include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/app_list/app_list_controller_delegate.h"
 #include "chrome/browser/ui/app_list/app_list_model_updater.h"
@@ -72,8 +73,11 @@ SearchController::SearchController(AppListModelUpdater* model_updater,
     : mixer_(std::make_unique<Mixer>(model_updater)),
       list_controller_(list_controller) {
   std::unique_ptr<SearchResultRanker> ranker =
-      std::make_unique<SearchResultRanker>(profile,
-                                           content::GetSystemConnector());
+      std::make_unique<SearchResultRanker>(
+          profile,
+          HistoryServiceFactory::GetForProfile(
+              profile, ServiceAccessType::EXPLICIT_ACCESS),
+          content::GetSystemConnector());
   ranker->InitializeRankers();
   mixer_->SetNonAppSearchResultRanker(std::move(ranker));
 }

chrome/browser/ui/app_list/search/search_result_ranker/recurrence_ranker.cc

@@ -166,6 +166,7 @@ RecurrenceRanker::RecurrenceRanker(const std::string& model_identifier,
           TimeDelta::FromSeconds(config.min_seconds_between_saves())),
       time_of_last_save_(Time::Now()),
       weak_factory_(this) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   task_runner_ = base::CreateSequencedTaskRunnerWithTraits(
       {base::TaskPriority::BEST_EFFORT, base::MayBlock(),
        base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN});
@@ -201,6 +202,7 @@ RecurrenceRanker::~RecurrenceRanker() = default;
 
 void RecurrenceRanker::OnLoadProtoFromDiskComplete(
     std::unique_ptr<RecurrenceRankerProto> proto) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   load_from_disk_completed_ = true;
   LogInitializationStatus(model_identifier_,
                           InitializationStatus::kInitialized);
@@ -242,6 +244,7 @@ void RecurrenceRanker::OnLoadProtoFromDiskComplete(
 
 void RecurrenceRanker::Record(const std::string& target,
                               const std::string& condition) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (!load_from_disk_completed_)
     return;
   LogUsage(model_identifier_, Usage::kRecord);
@@ -252,6 +255,7 @@ void RecurrenceRanker::Record(const std::string& target,
 
 void RecurrenceRanker::RenameTarget(const std::string& target,
                                     const std::string& new_target) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (!load_from_disk_completed_)
     return;
   LogUsage(model_identifier_, Usage::kRenameTarget);
@@ -261,6 +265,7 @@ void RecurrenceRanker::RenameTarget(const std::string& target,
 }
 
 void RecurrenceRanker::RemoveTarget(const std::string& target) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   // TODO(tby): Find a solution to the edge case of a removal before disk
   // loading is complete, resulting in the remove getting dropped.
   if (!load_from_disk_completed_)
@@ -273,6 +278,7 @@ void RecurrenceRanker::RemoveTarget(const std::string& target) {
 
 void RecurrenceRanker::RenameCondition(const std::string& condition,
                                        const std::string& new_condition) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (!load_from_disk_completed_)
     return;
   LogUsage(model_identifier_, Usage::kRenameCondition);
@@ -282,6 +288,7 @@ void RecurrenceRanker::RenameCondition(const std::string& condition,
 }
 
 void RecurrenceRanker::RemoveCondition(const std::string& condition) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (!load_from_disk_completed_)
     return;
   LogUsage(model_identifier_, Usage::kRemoveCondition);
@@ -292,6 +299,7 @@ void RecurrenceRanker::RemoveCondition(const std::string& condition) {
 
 std::map<std::string, float> RecurrenceRanker::Rank(
     const std::string& condition) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (!load_from_disk_completed_)
     return {};
   LogUsage(model_identifier_, Usage::kRank);
@@ -313,6 +321,7 @@ std::map<std::string, float> RecurrenceRanker::Rank(
 
 void RecurrenceRanker::MaybeCleanup(float proportion_valid,
                                     const FrecencyStore::ScoreTable& targets) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (proportion_valid > kMinValidTargetProportionBeforeCleanup)
     return;
 
@@ -325,6 +334,7 @@ void RecurrenceRanker::MaybeCleanup(float proportion_valid,
 std::vector<std::pair<std::string, float>> RecurrenceRanker::RankTopN(
     int n,
     const std::string& condition) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (!load_from_disk_completed_)
     return {};
 
@@ -333,15 +343,18 @@ std::vector<std::pair<std::string, float>> RecurrenceRanker::RankTopN(
 
 std::map<std::string, FrecencyStore::ValueData>*
 RecurrenceRanker::GetTargetData() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   return targets_->get_mutable_values();
 }
 
 std::map<std::string, FrecencyStore::ValueData>*
 RecurrenceRanker::GetConditionData() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   return conditions_->get_mutable_values();
 }
 
 void RecurrenceRanker::SaveToDisk() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (is_ephemeral_user_)
     return;
 
@@ -354,12 +367,14 @@ void RecurrenceRanker::SaveToDisk() {
 }
 
 void RecurrenceRanker::MaybeSave() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (Time::Now() - time_of_last_save_ > min_seconds_between_saves_) {
     SaveToDisk();
   }
 }
 
 void RecurrenceRanker::ToProto(RecurrenceRankerProto* proto) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   proto->set_config_hash(config_hash_);
   predictor_->ToProto(proto->mutable_predictor());
   targets_->ToProto(proto->mutable_targets());

chrome/browser/ui/app_list/search/search_result_ranker/recurrence_ranker.h

@@ -142,6 +142,8 @@ class RecurrenceRanker {
   const base::TimeDelta min_seconds_between_saves_;
   base::Time time_of_last_save_;
 
+  SEQUENCE_CHECKER(sequence_checker_);
+
   scoped_refptr<base::SequencedTaskRunner> task_runner_;
   base::WeakPtrFactory<RecurrenceRanker> weak_factory_;
 

chrome/browser/ui/app_list/search/search_result_ranker/search_result_ranker.cc

@@ -16,10 +16,14 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/task/post_task.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 #include "chrome/browser/chromeos/file_manager/file_tasks_notifier.h"
 #include "chrome/browser/chromeos/file_manager/file_tasks_notifier_factory.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
+#include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ui/app_list/search/chrome_search_result.h"
 #include "chrome/browser/ui/app_list/search/search_result_ranker/app_search_result_ranker.h"
 #include "chrome/browser/ui/app_list/search/search_result_ranker/ranking_item_util.h"
@@ -126,9 +130,15 @@ float ReRange(const float score, const float min, const float max) {
 }  // namespace
 
 SearchResultRanker::SearchResultRanker(Profile* profile,
+                                       history::HistoryService* history_service,
                                        service_manager::Connector* connector)
-    : config_converter_(connector), profile_(profile) {
+    : config_converter_(connector),
+      history_service_observer_(this),
+      profile_(profile),
+      weak_factory_(this) {
   DCHECK(profile);
+  DCHECK(history_service);
+  history_service_observer_.Add(history_service);
   if (auto* notifier =
           file_manager::file_tasks::FileTasksNotifier::GetForProfile(
               profile_)) {
@@ -328,4 +338,44 @@ void SearchResultRanker::OnFilesOpened(
                               GetTypeFromFileTaskNotifier(file_open.open_type));
 }
 
+void SearchResultRanker::OnURLsDeleted(
+    history::HistoryService* history_service,
+    const history::DeletionInfo& deletion_info) {
+  if (!query_based_mixed_types_ranker_)
+    return;
+
+  if (deletion_info.IsAllHistory()) {
+    // TODO(931149): We clear the whole model because we expect most targets to
+    // be URLs. In future, consider parsing the targets and only deleting URLs.
+    query_based_mixed_types_ranker_->GetTargetData()->clear();
+  } else {
+    for (const auto& row : deletion_info.deleted_rows()) {
+      // In order to perform URL normalization, NormalizeId requires any omnibox
+      // item type as argument. Pass kOmniboxGeneric here as we don't know the
+      // specific type.
+      query_based_mixed_types_ranker_->RemoveTarget(
+          NormalizeId(row.url().spec(), RankingItemType::kOmniboxGeneric));
+    }
+  }
+
+  // Force a save to disk. It is possible to get many calls to OnURLsDeleted in
+  // quick succession, eg. when all history is cleared from a different device.
+  // So delay the save slightly and only perform one save for all updates during
+  // that delay.
+  if (!query_mixed_ranker_save_queued_) {
+    query_mixed_ranker_save_queued_ = true;
+    DCHECK(base::SequencedTaskRunnerHandle::IsSet());
+    base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
+        FROM_HERE,
+        base::BindOnce(&SearchResultRanker::SaveQueryMixedRankerAfterDelete,
+                       weak_factory_.GetWeakPtr()),
+        TimeDelta::FromSeconds(3));
+  }
+}
+
+void SearchResultRanker::SaveQueryMixedRankerAfterDelete() {
+  query_based_mixed_types_ranker_->SaveToDisk();
+  query_mixed_ranker_save_queued_ = false;
+}
+
 }  // namespace app_list

chrome/browser/ui/app_list/search/search_result_ranker/search_result_ranker.h

@@ -13,6 +13,7 @@
 
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
+#include "base/scoped_observer.h"
 #include "base/strings/string16.h"
 #include "base/time/time.h"
 #include "chrome/browser/chromeos/file_manager/file_tasks_notifier.h"
@@ -23,6 +24,8 @@
 #include "chrome/browser/ui/app_list/search/search_result_ranker/app_launch_event_logger.h"
 #include "chrome/browser/ui/app_list/search/search_result_ranker/app_search_result_ranker.h"
 #include "chrome/browser/ui/app_list/search/search_result_ranker/recurrence_ranker_util.h"
+#include "components/history/core/browser/history_service.h"
+#include "components/history/core/browser/history_service_observer.h"
 
 namespace app_list {
 
@@ -35,9 +38,12 @@ enum class RankingItemType;
 // FetchRankings queries each model for ranking results. Rank modifies the
 // scores of provided search results, which are intended to be the output of a
 // search provider.
-class SearchResultRanker : file_manager::file_tasks::FileTasksObserver {
+class SearchResultRanker : file_manager::file_tasks::FileTasksObserver,
+                           history::HistoryServiceObserver {
  public:
-  SearchResultRanker(Profile* profile, service_manager::Connector* connector);
+  SearchResultRanker(Profile* profile,
+                     history::HistoryService* history_service,
+                     service_manager::Connector* connector);
   ~SearchResultRanker() override;
 
   // Performs all setup of rankers. This is separated from the constructor for
@@ -64,6 +70,10 @@ class SearchResultRanker : file_manager::file_tasks::FileTasksObserver {
   // file_manager::file_tasks::FileTaskObserver:
   void OnFilesOpened(const std::vector<FileOpenEvent>& file_opens) override;
 
+  // history::HistoryServiceObserver:
+  void OnURLsDeleted(history::HistoryService* history_service,
+                     const history::DeletionInfo& deletion_info) override;
+
   RecurrenceRanker* get_zero_state_mixed_types_ranker() {
     return zero_state_mixed_types_ranker_.get();
   }
@@ -77,6 +87,12 @@ class SearchResultRanker : file_manager::file_tasks::FileTasksObserver {
  private:
   FRIEND_TEST_ALL_PREFIXES(SearchResultRankerTest,
                            QueryMixedModelConfigDeployment);
+  FRIEND_TEST_ALL_PREFIXES(SearchResultRankerTest,
+                           QueryMixedModelDeletesURLCorrectly);
+
+  // Saves |query_based_mixed_types_ranker_| to disk. Called after a delay when
+  // URLs get deleted.
+  void SaveQueryMixedRankerAfterDelete();
 
   // Records the time of the last call to FetchRankings() and is used to
   // limit the number of queries to the models within a short timespan.
@@ -100,6 +116,9 @@ class SearchResultRanker : file_manager::file_tasks::FileTasksObserver {
   // these are local files and omnibox results.
   std::unique_ptr<RecurrenceRanker> query_based_mixed_types_ranker_;
   std::map<std::string, float> query_mixed_ranks_;
+  // Flag set when a delayed task to save the model is created. This is used to
+  // prevent several delayed tasks from being created.
+  bool query_mixed_ranker_save_queued_ = false;
 
   // Ranks files and previous queries for launcher zero-state.
   std::unique_ptr<RecurrenceRanker> zero_state_mixed_types_ranker_;
@@ -113,6 +132,9 @@ class SearchResultRanker : file_manager::file_tasks::FileTasksObserver {
   // Logs launch events and stores feature data for aggregated model.
   app_list::AppLaunchEventLogger app_launch_event_logger_;
 
+  ScopedObserver<history::HistoryService, history::HistoryServiceObserver>
+      history_service_observer_;
+
   // TODO(931149): Move the AppSearchResultRanker instance and associated logic
   // to here.
 
@@ -121,6 +143,8 @@ class SearchResultRanker : file_manager::file_tasks::FileTasksObserver {
   base::flat_map<std::string, float> app_ranks_;
 
   Profile* profile_;
+
+  base::WeakPtrFactory<SearchResultRanker> weak_factory_;
 };
 
 }  // namespace app_list

chrome/browser/ui/app_list/search/search_result_ranker/search_result_ranker_unittest.cc

@@ -14,6 +14,7 @@
 
 #include "ash/public/cpp/app_list/app_list_features.h"
 #include "ash/public/cpp/app_list/app_list_types.h"
+#include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/stl_util.h"
 #include "base/strings/string16.h"
@@ -26,6 +27,11 @@
 #include "chrome/browser/ui/app_list/search/search_result_ranker/ranking_item_util.h"
 #include "chrome/browser/ui/app_list/search/search_result_ranker/recurrence_ranker.h"
 #include "chrome/test/base/testing_profile.h"
+#include "components/history/core/browser/history_database_params.h"
+#include "components/history/core/browser/history_service.h"
+#include "components/history/core/browser/history_types.h"
+#include "components/history/core/test/history_service_test_util.h"
+#include "components/history/core/test/test_history_database.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "services/data_decoder/public/cpp/test_data_decoder_service.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -39,6 +45,7 @@ using ResultType = ash::SearchResultType;
 using base::ScopedTempDir;
 using base::test::ScopedFeatureList;
 using testing::ElementsAre;
+using testing::UnorderedElementsAre;
 using testing::WhenSorted;
 
 class TestSearchResult : public ChromeSearchResult {
@@ -71,6 +78,10 @@ MATCHER_P(HasId, id, "") {
   return base::UTF16ToUTF8(arg.result->title()) == id;
 }
 
+MATCHER_P2(HasIdScore, id, score, "") {
+  return base::UTF16ToUTF8(arg.result->title()) == id && arg.score == score;
+}
+
 }  // namespace
 
 class SearchResultRankerTest : public testing::Test {
@@ -87,6 +98,11 @@ class SearchResultRankerTest : public testing::Test {
     profile_builder.SetProfileName("testuser@gmail.com");
     profile_builder.SetPath(temp_dir_.GetPath().AppendASCII("TestProfile"));
     profile_ = profile_builder.Build();
+
+    history_service_ = std::make_unique<history::HistoryService>();
+    history_service_->Init(
+        history::TestHistoryDatabaseParamsForPath(temp_dir_.GetPath()));
+    Wait();
   }
 
   std::unique_ptr<SearchResultRanker> MakeRanker(
@@ -100,8 +116,8 @@ class SearchResultRankerTest : public testing::Test {
           app_list_features::kEnableQueryBasedMixedTypesRanker);
     }
 
-    auto ranker = std::make_unique<SearchResultRanker>(profile_.get(),
-                                                       dd_service_.connector());
+    auto ranker = std::make_unique<SearchResultRanker>(
+        profile_.get(), history_service_.get(), dd_service_.connector());
     return ranker;
   }
 
@@ -116,9 +132,10 @@ class SearchResultRankerTest : public testing::Test {
     return results;
   }
 
+  history::HistoryService* history_service() { return history_service_.get(); }
+
   void Wait() { thread_bundle_.RunUntilIdle(); }
 
- private:
   content::TestBrowserThreadBundle thread_bundle_;
 
   // This is used only to make the ownership clear for the TestSearchResult
@@ -130,9 +147,11 @@ class SearchResultRankerTest : public testing::Test {
 
   ScopedFeatureList scoped_feature_list_;
   ScopedTempDir temp_dir_;
+  std::unique_ptr<history::HistoryService> history_service_;
 
   std::unique_ptr<Profile> profile_;
 
+ private:
   DISALLOW_COPY_AND_ASSIGN(SearchResultRankerTest);
 };
 
@@ -336,4 +355,108 @@ TEST_F(SearchResultRankerTest, QueryMixedModelConfigDeployment) {
             "ExponentialWeightsEnsemble");
 }
 
+// Tests that, when a URL is deleted from the history service, the query-based
+// mixed-types model deletes it in memory and from disk.
+TEST_F(SearchResultRankerTest, QueryMixedModelDeletesURLCorrectly) {
+  // Create ranker.
+  const std::string json = R"({
+      "min_seconds_between_saves": 1000,
+      "target_limit": 100,
+      "target_decay": 0.5,
+      "condition_limit": 10,
+      "condition_decay": 0.5,
+      "predictor": {
+        "predictor_type": "fake"
+      }
+    })";
+
+  base::RunLoop run_loop;
+  auto ranker =
+      MakeRanker(true, {{"boost_coefficient", "1.0"}, {"config", json}});
+  ranker->set_json_config_parsed_for_testing(run_loop.QuitClosure());
+  ranker->InitializeRankers();
+  run_loop.Run();
+  Wait();
+
+  const base::FilePath model_path =
+      profile_->GetPath().AppendASCII("query_based_mixed_types_ranker.pb");
+
+  // Train the model on two URLs.
+  const std::string url_1 = "http://www.google.com/testing";
+  AppLaunchData url_1_data;
+  url_1_data.id = url_1;
+  url_1_data.ranking_item_type = RankingItemType::kOmniboxHistory;
+  url_1_data.query = "query";
+  ranker->Train(url_1_data);
+  ranker->Train(url_1_data);
+
+  const std::string url_2 = "http://www.other.com";
+  AppLaunchData url_2_data;
+  url_2_data.id = url_2;
+  url_2_data.ranking_item_type = RankingItemType::kOmniboxHistory;
+  url_2_data.query = "query";
+  ranker->Train(url_2_data);
+
+  // Expect the scores of the urls to reflect their training.
+  {
+    ranker->FetchRankings(base::UTF8ToUTF16("query"));
+    auto results = MakeSearchResults(
+        {url_1, url_2, "untrained"},
+        {ResultType::kOmnibox, ResultType::kOmnibox, ResultType::kOmnibox},
+        {0.0f, 0.0f, 0.5f});
+    ranker->Rank(&results);
+    EXPECT_THAT(results, UnorderedElementsAre(HasIdScore(url_1, 2.0f),
+                                              HasIdScore(url_2, 1.0f),
+                                              HasIdScore("untrained", 0.5f)));
+  }
+
+  // Now delete |url_1| from the history service and ensure we save the model to
+  // disk.
+  EXPECT_FALSE(base::PathExists(model_path));
+  history_service()->AddPage(GURL(url_1), base::Time::Now(),
+                             history::VisitSource::SOURCE_BROWSED);
+  history_service()->DeleteURL(GURL(url_1));
+  history::BlockUntilHistoryProcessesPendingRequests(history_service());
+  Wait();
+  EXPECT_TRUE(base::PathExists(model_path));
+
+  // Force cache expiry.
+  ranker->time_of_last_fetch_ = base::Time();
+
+  // Expect the score of |url_1| to be 0.0, it should have been deleted from
+  // the model.
+  {
+    ranker->FetchRankings(base::UTF8ToUTF16("query"));
+    auto results = MakeSearchResults(
+        {url_1, url_2, "untrained"},
+        {ResultType::kOmnibox, ResultType::kOmnibox, ResultType::kOmnibox},
+        {0.0f, 0.0f, 0.5f});
+    ranker->Rank(&results);
+    EXPECT_THAT(results, UnorderedElementsAre(HasIdScore(url_1, 0.0f),
+                                              HasIdScore(url_2, 1.0f),
+                                              HasIdScore("untrained", 0.5f)));
+  }
+
+  // Load a new ranker from disk and ensure |url_1| hasn't been retained.
+  base::RunLoop new_run_loop;
+  auto new_ranker = std::make_unique<SearchResultRanker>(
+      profile_.get(), history_service(), dd_service_.connector());
+  new_ranker->set_json_config_parsed_for_testing(new_run_loop.QuitClosure());
+  new_ranker->InitializeRankers();
+  new_run_loop.Run();
+  Wait();
+
+  {
+    new_ranker->FetchRankings(base::UTF8ToUTF16("query"));
+    auto results = MakeSearchResults(
+        {url_1, url_2, "untrained"},
+        {ResultType::kOmnibox, ResultType::kOmnibox, ResultType::kOmnibox},
+        {0.0f, 0.0f, 0.5f});
+    new_ranker->Rank(&results);
+    EXPECT_THAT(results, UnorderedElementsAre(HasIdScore(url_1, 0.0f),
+                                              HasIdScore(url_2, 1.0f),
+                                              HasIdScore("untrained", 0.5f)));
+  }
+}
+
 }  // namespace app_list