predictors: Clean up the predictor db after deleting individual urls

The autocomplete predictor database contains entries related to a page the user
has visited even after this page was removed from the history. These entries
are cleaned up after only the startup. This CL adds this clean up after every
removal from history.

Bug: 838897
Change-Id: Ia56de868cddf8bb0a28d288265b884c0e67458d5
Reviewed-on: https://chromium-review.googlesource.com/1043868
Commit-Queue: Alexandr Ilin <alexilin@chromium.org>
Reviewed-by: Benoit L <lizeb@chromium.org>
Reviewed-by: Christian Dullweber <dullweber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558326}

chrome/browser/predictors/autocomplete_action_predictor.cc

@@ -352,11 +352,11 @@ void AutocompleteActionPredictor::DeleteAllRows() {
                             DATABASE_ACTION_DELETE_ALL, DATABASE_ACTION_COUNT);
 }
 
-void AutocompleteActionPredictor::DeleteRowsWithURLs(
-    const history::URLRows& rows) {
+void AutocompleteActionPredictor::DeleteRowsFromCaches(
+    const history::URLRows& rows,
+    std::vector<AutocompleteActionPredictorTable::Row::Id>* id_list) {
   DCHECK(initialized_);
-
-  std::vector<AutocompleteActionPredictorTable::Row::Id> id_list;
+  DCHECK(id_list);
 
   for (DBCacheMap::iterator it = db_cache_.begin(); it != db_cache_.end();) {
     if (std::find_if(rows.begin(), rows.end(),
@@ -364,22 +364,13 @@ void AutocompleteActionPredictor::DeleteRowsWithURLs(
         rows.end()) {
       const DBIdCacheMap::iterator id_it = db_id_cache_.find(it->first);
       DCHECK(id_it != db_id_cache_.end());
-      id_list.push_back(id_it->second);
+      id_list->push_back(id_it->second);
       db_id_cache_.erase(id_it);
       db_cache_.erase(it++);
     } else {
       ++it;
     }
   }
-
-  if (table_.get()) {
-    table_->GetTaskRunner()->PostTask(
-        FROM_HERE, base::BindOnce(&AutocompleteActionPredictorTable::DeleteRows,
-                                  table_, id_list));
-  }
-
-  UMA_HISTOGRAM_ENUMERATION("AutocompleteActionPredictor.DatabaseAction",
-                            DATABASE_ACTION_DELETE_SOME, DATABASE_ACTION_COUNT);
 }
 
 void AutocompleteActionPredictor::AddAndUpdateRows(
@@ -488,11 +479,9 @@ void AutocompleteActionPredictor::DeleteOldIdsFromCaches(
     std::vector<AutocompleteActionPredictorTable::Row::Id>* id_list) {
   CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   DCHECK(!profile_->IsOffTheRecord());
-  DCHECK(!initialized_);
   DCHECK(url_db);
   DCHECK(id_list);
 
-  id_list->clear();
   for (DBCacheMap::iterator it = db_cache_.begin(); it != db_cache_.end();) {
     history::URLRow url_row;
 
@@ -565,10 +554,28 @@ void AutocompleteActionPredictor::OnURLsDeleted(
     const history::DeletionInfo& deletion_info) {
   DCHECK(initialized_);
 
-  if (deletion_info.IsAllHistory())
+  if (deletion_info.IsAllHistory()) {
     DeleteAllRows();
-  else
-    DeleteRowsWithURLs(deletion_info.deleted_rows());
+    return;
+  }
+
+  std::vector<AutocompleteActionPredictorTable::Row::Id> id_list;
+  DeleteRowsFromCaches(deletion_info.deleted_rows(), &id_list);
+
+  if (!deletion_info.is_from_expiration() && history_service) {
+    auto* url_db = history_service->InMemoryDatabase();
+    if (url_db)
+      DeleteOldIdsFromCaches(url_db, &id_list);
+  }
+
+  if (table_.get()) {
+    table_->GetTaskRunner()->PostTask(
+        FROM_HERE, base::BindOnce(&AutocompleteActionPredictorTable::DeleteRows,
+                                  table_, std::move(id_list)));
+  }
+
+  UMA_HISTOGRAM_ENUMERATION("AutocompleteActionPredictor.DatabaseAction",
+                            DATABASE_ACTION_DELETE_SOME, DATABASE_ACTION_COUNT);
 }
 
 void AutocompleteActionPredictor::OnHistoryServiceLoaded(

chrome/browser/predictors/autocomplete_action_predictor.h

@@ -175,8 +175,12 @@ class AutocompleteActionPredictor
   // Removes all rows from the database and caches.
   void DeleteAllRows();
 
-  // Removes rows from the database and caches that contain a URL in |rows|.
-  void DeleteRowsWithURLs(const history::URLRows& rows);
+  // Removes rows that contain a URL in |rows| from the local caches.
+  // |id_list| must not be nullptr. Every row id deleted will be added to
+  // |id_list|.
+  void DeleteRowsFromCaches(
+      const history::URLRows& rows,
+      std::vector<AutocompleteActionPredictorTable::Row::Id>* id_list);
 
   // Adds and updates rows in the database and caches.
   void AddAndUpdateRows(
@@ -198,7 +202,8 @@ class AutocompleteActionPredictor
   void DeleteOldEntries(history::URLDatabase* url_db);
 
   // Deletes any old or invalid entries from the local caches. |url_db| and
-  // |id_list| must not be NULL. Every row id deleted will be added to id_list.
+  // |id_list| must not be nullptr. Every row id deleted will be added to
+  // |id_list|.
   void DeleteOldIdsFromCaches(
       history::URLDatabase* url_db,
       std::vector<AutocompleteActionPredictorTable::Row::Id>* id_list);

chrome/browser/predictors/autocomplete_action_predictor_unittest.cc

@@ -120,11 +120,6 @@ class AutocompleteActionPredictorTest : public testing::Test {
   typedef AutocompleteActionPredictor::DBCacheMap DBCacheMap;
   typedef AutocompleteActionPredictor::DBIdCacheMap DBIdCacheMap;
 
-  void AddAllRowsToHistory() {
-    for (size_t i = 0; i < arraysize(test_url_db); ++i)
-      ASSERT_TRUE(AddRowToHistory(test_url_db[i]));
-  }
-
   history::URLID AddRowToHistory(const TestUrlInfo& test_row) {
     history::HistoryService* history = HistoryServiceFactory::GetForProfile(
         profile_.get(), ServiceAccessType::EXPLICIT_ACCESS);
@@ -155,7 +150,7 @@ class AutocompleteActionPredictorTest : public testing::Test {
   }
 
   void AddAllRows() {
-    for (size_t i = 0; i < arraysize(test_url_db); ++i)
+    for (size_t i = 0; i < base::size(test_url_db); ++i)
       AddRow(test_url_db[i]);
   }
 
@@ -177,12 +172,66 @@ class AutocompleteActionPredictorTest : public testing::Test {
         AutocompleteActionPredictorTable::Rows(1, row));
   }
 
+  void OnURLsDeletedTest(bool expired) {
+    ASSERT_NO_FATAL_FAILURE(AddAllRows());
+
+    EXPECT_EQ(base::size(test_url_db), db_cache()->size());
+    EXPECT_EQ(base::size(test_url_db), db_id_cache()->size());
+
+    std::vector<size_t> expected;
+    history::URLRows rows;
+    for (size_t i = 0; i < base::size(test_url_db); ++i) {
+      bool expect_deleted = false;
+
+      if (i < 2) {
+        rows.push_back(history::URLRow(test_url_db[i].url));
+        expect_deleted = true;
+      }
+
+      if (!expired &&
+          test_url_db[i].days_from_now > maximum_days_to_keep_entry()) {
+        expect_deleted = true;
+      }
+
+      if (i != 3 && i != 4)
+        ASSERT_TRUE(AddRowToHistory(test_url_db[i]));
+      else if (!expired)
+        expect_deleted = true;
+
+      if (!expect_deleted)
+        expected.push_back(i);
+    }
+
+    history::HistoryService* history_service =
+        HistoryServiceFactory::GetForProfile(
+            profile_.get(), ServiceAccessType::EXPLICIT_ACCESS);
+    ASSERT_TRUE(history_service);
+
+    predictor_->OnURLsDeleted(
+        history_service,
+        history::DeletionInfo(history::DeletionTimeRange::Invalid(), expired,
+                              rows, std::set<GURL>(), base::nullopt));
+
+    EXPECT_EQ(expected.size(), db_cache()->size());
+    EXPECT_EQ(expected.size(), db_id_cache()->size());
+
+    for (size_t i = 0; i < base::size(test_url_db); ++i) {
+      DBCacheKey key = {test_url_db[i].user_text, test_url_db[i].url};
+
+      bool deleted = !base::ContainsValue(expected, i);
+      EXPECT_EQ(deleted, db_cache()->find(key) == db_cache()->end());
+      EXPECT_EQ(deleted, db_id_cache()->find(key) == db_id_cache()->end());
+    }
+  }
+
   void DeleteAllRows() {
     predictor_->DeleteAllRows();
   }
 
-  void DeleteRowsWithURLs(const history::URLRows& rows) {
-    predictor_->DeleteRowsWithURLs(rows);
+  void DeleteRowsFromCaches(
+      const history::URLRows& rows,
+      std::vector<AutocompleteActionPredictorTable::Row::Id>* id_list) {
+    predictor_->DeleteRowsFromCaches(rows, id_list);
   }
 
   void DeleteOldIdsFromCaches(
@@ -195,10 +244,6 @@ class AutocompleteActionPredictorTest : public testing::Test {
     history::URLDatabase* url_db = history_service->InMemoryDatabase();
     ASSERT_TRUE(url_db);
 
-    // Reset the predictor's |initialized_| flag for the life of this call,
-    // since outside of testing this function is only supposed to be reached
-    // before initialization is completed.
-    base::AutoReset<bool> initialized_reset(&predictor_->initialized_, false);
     predictor_->DeleteOldIdsFromCaches(url_db, id_list);
   }
 
@@ -240,8 +285,8 @@ TEST_F(AutocompleteActionPredictorTest, AddRow) {
 TEST_F(AutocompleteActionPredictorTest, UpdateRow) {
   ASSERT_NO_FATAL_FAILURE(AddAllRows());
 
-  EXPECT_EQ(arraysize(test_url_db), db_cache()->size());
-  EXPECT_EQ(arraysize(test_url_db), db_id_cache()->size());
+  EXPECT_EQ(base::size(test_url_db), db_cache()->size());
+  EXPECT_EQ(base::size(test_url_db), db_id_cache()->size());
 
   // Get the data back out of the cache.
   const DBCacheKey key = { test_url_db[0].user_text, test_url_db[0].url };
@@ -276,8 +321,8 @@ TEST_F(AutocompleteActionPredictorTest, UpdateRow) {
 TEST_F(AutocompleteActionPredictorTest, DeleteAllRows) {
   ASSERT_NO_FATAL_FAILURE(AddAllRows());
 
-  EXPECT_EQ(arraysize(test_url_db), db_cache()->size());
-  EXPECT_EQ(arraysize(test_url_db), db_id_cache()->size());
+  EXPECT_EQ(base::size(test_url_db), db_cache()->size());
+  EXPECT_EQ(base::size(test_url_db), db_id_cache()->size());
 
   DeleteAllRows();
 
@@ -285,27 +330,34 @@ TEST_F(AutocompleteActionPredictorTest, DeleteAllRows) {
   EXPECT_TRUE(db_id_cache()->empty());
 }
 
-TEST_F(AutocompleteActionPredictorTest, DeleteRowsWithURLs) {
-  ASSERT_NO_FATAL_FAILURE(AddAllRows());
+TEST_F(AutocompleteActionPredictorTest, DeleteRowsFromCaches) {
+  std::vector<AutocompleteActionPredictorTable::Row::Id> all_ids;
+  history::URLRows rows;
+  for (size_t i = 0; i < base::size(test_url_db); ++i) {
+    std::string row_id = AddRow(test_url_db[i]);
+    all_ids.push_back(row_id);
 
-  EXPECT_EQ(arraysize(test_url_db), db_cache()->size());
-  EXPECT_EQ(arraysize(test_url_db), db_id_cache()->size());
+    if (i < 2)
+      rows.push_back(history::URLRow(test_url_db[i].url));
+  }
 
-  history::URLRows rows;
-  for (size_t i = 0; i < 2; ++i)
-    rows.push_back(history::URLRow(test_url_db[i].url));
+  EXPECT_EQ(base::size(test_url_db), all_ids.size());
+  EXPECT_EQ(base::size(test_url_db), db_cache()->size());
+  EXPECT_EQ(base::size(test_url_db), db_id_cache()->size());
 
-  DeleteRowsWithURLs(rows);
+  std::vector<AutocompleteActionPredictorTable::Row::Id> id_list;
+  DeleteRowsFromCaches(rows, &id_list);
 
-  EXPECT_EQ(arraysize(test_url_db) - 2, db_cache()->size());
-  EXPECT_EQ(arraysize(test_url_db) - 2, db_id_cache()->size());
+  EXPECT_EQ(base::size(test_url_db) - 2, db_cache()->size());
+  EXPECT_EQ(base::size(test_url_db) - 2, db_id_cache()->size());
 
-  for (size_t i = 0; i < arraysize(test_url_db); ++i) {
+  for (size_t i = 0; i < base::size(test_url_db); ++i) {
     DBCacheKey key = { test_url_db[i].user_text, test_url_db[i].url };
 
     bool deleted = (i < 2);
     EXPECT_EQ(deleted, db_cache()->find(key) == db_cache()->end());
     EXPECT_EQ(deleted, db_id_cache()->find(key) == db_id_cache()->end());
+    EXPECT_EQ(deleted, base::ContainsValue(id_list, all_ids[i]));
   }
 }
 
@@ -313,7 +365,7 @@ TEST_F(AutocompleteActionPredictorTest, DeleteOldIdsFromCaches) {
   std::vector<AutocompleteActionPredictorTable::Row::Id> expected;
   std::vector<AutocompleteActionPredictorTable::Row::Id> all_ids;
 
-  for (size_t i = 0; i < arraysize(test_url_db); ++i) {
+  for (size_t i = 0; i < base::size(test_url_db); ++i) {
     std::string row_id = AddRow(test_url_db[i]);
     all_ids.push_back(row_id);
 
@@ -334,15 +386,21 @@ TEST_F(AutocompleteActionPredictorTest, DeleteOldIdsFromCaches) {
   EXPECT_EQ(all_ids.size() - expected.size(), db_cache()->size());
   EXPECT_EQ(all_ids.size() - expected.size(), db_id_cache()->size());
 
-  for (std::vector<AutocompleteActionPredictorTable::Row::Id>::iterator it =
-       all_ids.begin();
-       it != all_ids.end(); ++it) {
+  for (auto it = all_ids.begin(); it != all_ids.end(); ++it) {
     bool in_expected = base::ContainsValue(expected, *it);
     bool in_list = base::ContainsValue(id_list, *it);
     EXPECT_EQ(in_expected, in_list);
   }
 }
 
+TEST_F(AutocompleteActionPredictorTest, OnURLsDeletedExpired) {
+  OnURLsDeletedTest(true);
+}
+
+TEST_F(AutocompleteActionPredictorTest, OnURLsDeletedNonExpired) {
+  OnURLsDeletedTest(false);
+}
+
 TEST_F(AutocompleteActionPredictorTest, RecommendActionURL) {
   ASSERT_NO_FATAL_FAILURE(AddAllRows());
 
@@ -352,7 +410,7 @@ TEST_F(AutocompleteActionPredictorTest, RecommendActionURL) {
   prerender::PrerenderManager::SetMode(
       prerender::PrerenderManager::PRERENDER_MODE_NOSTATE_PREFETCH);
 
-  for (size_t i = 0; i < arraysize(test_url_db); ++i) {
+  for (size_t i = 0; i < base::size(test_url_db); ++i) {
     match.destination_url = GURL(test_url_db[i].url);
     EXPECT_EQ(test_url_db[i].expected_action,
               predictor()->RecommendAction(test_url_db[i].user_text, match))
@@ -369,7 +427,7 @@ TEST_F(AutocompleteActionPredictorTest, RecommendActionSearch) {
   prerender::PrerenderManager::SetMode(
       prerender::PrerenderManager::PRERENDER_MODE_NOSTATE_PREFETCH);
 
-  for (size_t i = 0; i < arraysize(test_url_db); ++i) {
+  for (size_t i = 0; i < base::size(test_url_db); ++i) {
     match.destination_url = GURL(test_url_db[i].url);
     AutocompleteActionPredictor::Action expected_action =
         (test_url_db[i].expected_action ==