[NTP][RQ] Makes in-memory URLDatabase deletion tasks asynchronous

crrev.com/c/2531654 made deletion of queries from the in-memory URL DB
asynchronous by posting the deletion task to a sequenced task runner on
the UI thread. The said CL was later reverted in crrev.com/c/2532543
for making RepeatableQueriesServiceTest.SignedOut_Deletion flaky. This
CL fixes the flakiness by providing a way for the tests to flush the
scheduled tasks after deletion from the in-memory URL DB and TearDown()

Bug: 1148612
Change-Id: I3f5c7b598a5d6907794b9d6ca1598626156516c5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2536355Reviewed-by: Ramya Nagarajan <ramyan@chromium.org>
Commit-Queue: Moe Ahmadi <mahmadi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#828854}

components/search/repeatable_queries/repeatable_queries_service.cc

@@ -12,6 +12,8 @@
 #include "base/metrics/histogram_functions.h"
 #include "base/scoped_observation.h"
 #include "base/stl_util.h"
+#include "base/task/task_traits.h"
+#include "base/task/thread_pool.h"
 #include "base/values.h"
 #include "components/history/core/browser/history_service.h"
 #include "components/history/core/browser/history_types.h"
@@ -128,7 +130,8 @@ RepeatableQueriesService::RepeatableQueriesService(
       search_provider_observer_(std::make_unique<SearchProviderObserver>(
           template_url_service,
           base::BindRepeating(&RepeatableQueriesService::SearchProviderChanged,
-                              base::Unretained(this)))) {
+                              base::Unretained(this)))),
+      deletion_task_runner_(base::ThreadPool::CreateSequencedTaskRunner({})) {
   DCHECK(history_service_);
   DCHECK(template_url_service_);
   DCHECK(url_loader_factory_);
@@ -256,6 +259,11 @@ GURL RepeatableQueriesService::GetRequestURL() {
                                                     search_terms_data));
 }
 
+void RepeatableQueriesService::FlushForTesting(base::OnceClosure flushed) {
+  deletion_task_runner_->PostTaskAndReply(FROM_HERE, base::DoNothing(),
+                                          std::move(flushed));
+}
+
 void RepeatableQueriesService::GetRepeatableQueriesFromServer() {
   net::NetworkTrafficAnnotationTag traffic_annotation =
       net::DefineNetworkTrafficAnnotation("repeatable_queries_service", R"(
@@ -356,7 +364,7 @@ void RepeatableQueriesService::RepeatableQueriesParsed(
 void RepeatableQueriesService::GetRepeatableQueriesFromURLDatabase() {
   repeatable_queries_.clear();
 
-  // Fail if the in-memory URL database is not available.
+  // Fail if the in-memory URLDatabase is not available.
   history::URLDatabase* url_db = history_service_->InMemoryDatabase();
   if (!url_db)
     return;
@@ -461,13 +469,23 @@ void RepeatableQueriesService::DeletionResponseLoaded(
 
 void RepeatableQueriesService::DeleteRepeatableQueryFromURLDatabase(
     const base::string16& query) {
-  // Fail if the in-memory URL database is not available.
-  history::URLDatabase* url_db = history_service_->InMemoryDatabase();
+  deletion_task_runner_->PostTask(
+      FROM_HERE,
+      base::BindOnce(
+          &RepeatableQueriesService::DeleteRepeatableQueryFromURLDatabaseTask,
+          weak_ptr_factory_.GetWeakPtr(), query,
+          history_service_->InMemoryDatabase()));
+}
+
+void RepeatableQueriesService::DeleteRepeatableQueryFromURLDatabaseTask(
+    const base::string16& query,
+    history::URLDatabase* url_db) {
+  // Fail if the in-memory URLDatabase is not available.
   if (!url_db)
     return;
 
   // Delete all the search terms matching the repeatable query suggestion from
-  // the in-memory URL database.
+  // the in-memory URLDatabase.
   url_db->DeleteKeywordSearchTermForNormalizedTerm(
       template_url_service_->GetDefaultSearchProvider()->id(), query);
 }

components/search/repeatable_queries/repeatable_queries_service.h

@@ -9,6 +9,7 @@
 #include <string>
 #include <vector>
 
+#include "base/callback_forward.h"
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
 #include "components/keyed_service/core/keyed_service.h"
@@ -19,8 +20,13 @@
 class SearchProviderObserver;
 class TemplateURLService;
 
+namespace base {
+class SequencedTaskRunner;
+}  // namespace base
+
 namespace history {
 class HistoryService;
+class URLDatabase;
 }  // namespace history
 
 namespace network {
@@ -59,8 +65,8 @@ class RepeatableQuery {
 
 // Provides repeatable query suggestions to be shown in the NTP Most Visited
 // tiles when Google is the default search provider. The repeatable queries are
-// requested from the server for signed-in users and extracted from the URL
-// database for unauthenticated users.
+// requested from the server for signed-in users and extracted from the
+// in-memory URLDatabase for unauthenticated users.
 class RepeatableQueriesService : public KeyedService {
  public:
   RepeatableQueriesService(
@@ -85,7 +91,7 @@ class RepeatableQueriesService : public KeyedService {
 
   // If Google is the default search provider, asynchronously requests
   // repeatable query suggestions from the server for signed-in users and
-  // synchronously extracts them from the URL database for
+  // synchronously extracts them from the in-memory URLDatabase for
   // unauthenticated users. Regardless of success, observers are notified via
   // RepeatableQueriesServiceObserver::OnRepeatableQueriesUpdated.
   void Refresh();
@@ -122,6 +128,8 @@ class RepeatableQueriesService : public KeyedService {
   // Returns the server request URL.
   GURL GetRequestURL();
 
+  void FlushForTesting(base::OnceClosure flushed);
+
  private:
   // Requests repeatable queries from the server. Called for signed-in users.
   void GetRepeatableQueriesFromServer();
@@ -135,6 +143,8 @@ class RepeatableQueriesService : public KeyedService {
 
   // Deletes |query| from the in-memory URLDatabase.
   void DeleteRepeatableQueryFromURLDatabase(const base::string16& query);
+  void DeleteRepeatableQueryFromURLDatabaseTask(const base::string16& query,
+                                                history::URLDatabase* url_db);
 
   // Deletes the query with |deletion_url| from the server.
   void DeleteRepeatableQueryFromServer(const std::string& deletion_url);
@@ -169,6 +179,9 @@ class RepeatableQueriesService : public KeyedService {
 
   std::vector<std::unique_ptr<network::SimpleURLLoader>> loaders_;
 
+  // The TaskRunner to which in-memory URLDatabase deletion tasks are posted.
+  scoped_refptr<base::SequencedTaskRunner> deletion_task_runner_;
+
   base::WeakPtrFactory<RepeatableQueriesService> weak_ptr_factory_{this};
 };
 

components/search/repeatable_queries/repeatable_queries_service_unittest.cc

@@ -146,6 +146,18 @@ class TestRepeatableQueriesService : public RepeatableQueriesService {
     return &search_provider_observer_;
   }
 
+  void SearchProviderChanged() {
+    RepeatableQueriesService::SearchProviderChanged();
+  }
+
+  void SigninStatusChanged() {
+    RepeatableQueriesService::SigninStatusChanged();
+  }
+
+  void FlushForTesting(base::OnceClosure flushed) {
+    RepeatableQueriesService::FlushForTesting(std::move(flushed));
+  }
+
   GURL GetQueryDestinationURL(const base::string16& query) {
     return RepeatableQueriesService::GetQueryDestinationURL(query);
   }
@@ -156,14 +168,6 @@ class TestRepeatableQueriesService : public RepeatableQueriesService {
 
   GURL GetRequestURL() { return RepeatableQueriesService::GetRequestURL(); }
 
-  void SearchProviderChanged() {
-    RepeatableQueriesService::SearchProviderChanged();
-  }
-
-  void SigninStatusChanged() {
-    RepeatableQueriesService::SigninStatusChanged();
-  }
-
   testing::NiceMock<MockSearchProviderObserver> search_provider_observer_;
 };
 
@@ -220,8 +224,10 @@ class RepeatableQueriesServiceTest : public ::testing::Test,
     // InMemoryURLIndex must be explicitly shut down or it will DCHECK() in
     // its destructor.
     in_memory_url_index_->Shutdown();
-    // Needed to prevent leaks due to posted history index rebuild task.
-    task_environment_.RunUntilIdle();
+
+    WaitForRepeatableQueriesService();
+    WaitForHistoryService();
+    WaitForInMemoryURLIndex();
   }
 
   const TemplateURL* default_search_provider() {
@@ -284,12 +290,20 @@ class RepeatableQueriesServiceTest : public ::testing::Test,
     }
   }
 
+  // Waits for RepeatableQueriesService's async operations.
+  void WaitForRepeatableQueriesService() {
+    base::RunLoop run_loop;
+    service_->FlushForTesting(run_loop.QuitClosure());
+    run_loop.Run();
+  }
+
   // Waits for history::HistoryService's async operations.
   void WaitForHistoryService() {
     history::BlockUntilHistoryProcessesPendingRequests(history_service_.get());
+  }
 
-    // MemoryURLIndex schedules tasks to rebuild its index on the history
-    // thread. Block here to make sure they are complete.
+  // Waits for InMemoryURLIndex's async operations.
+  void WaitForInMemoryURLIndex() {
     BlockUntilInMemoryURLIndexIsRefreshed(in_memory_url_index_.get());
   }
 
@@ -651,6 +665,9 @@ TEST_F(RepeatableQueriesServiceTest, SignedOut_Deletion) {
   // The deleted suggestion is not offered anymore.
   EXPECT_EQ(expected_local_queries, service()->repeatable_queries());
 
+  // Make sure there is no pending deletion task.
+  WaitForRepeatableQueriesService();
+
   // Request a refresh.
   RefreshAndMaybeWaitForService();
   expected_local_queries = {{base::ASCIIToUTF16("local query 2"),