Remove dependencies of InMemoryURLIndex on Profile

Pass to InMemoryURLIndex a pointer to the owning HistoryService and
remove the reference to Profile that is no longer necessary.

Fix the unit tests to no longer pass the Profile, and to call the
Shutdown method before destroying the object so that it properly
remove itself from the HistoryService observer list.

BUG=370850

Review URL: https://codereview.chromium.org/879763002

Cr-Commit-Position: refs/heads/master@{#313485}

chrome/browser/history/history_service.cc

@@ -963,7 +963,7 @@ bool HistoryService::Init(
     std::string languages =
         profile_->GetPrefs()->GetString(prefs::kAcceptLanguages);
     in_memory_url_index_.reset(new history::InMemoryURLIndex(
-        profile_, this, history_dir_, languages, history_client_));
+        this, history_dir_, languages, history_client_));
     in_memory_url_index_->Init();
   }
 

chrome/browser/history/in_memory_url_index.cc

@@ -8,9 +8,7 @@
 #include "base/files/file_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/history/history_service.h"
-#include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/history/url_index_private_data.h"
-#include "chrome/browser/profiles/profile.h"
 #include "chrome/common/url_constants.h"
 #include "components/history/core/browser/url_database.h"
 #include "content/public/browser/browser_thread.h"
@@ -81,13 +79,11 @@ InMemoryURLIndex::RebuildPrivateDataFromHistoryDBTask::
 
 // InMemoryURLIndex ------------------------------------------------------------
 
-InMemoryURLIndex::InMemoryURLIndex(Profile* profile,
-                                   HistoryService* history_service,
+InMemoryURLIndex::InMemoryURLIndex(HistoryService* history_service,
                                    const base::FilePath& history_dir,
                                    const std::string& languages,
                                    HistoryClient* history_client)
-    : profile_(profile),
-      history_service_(history_service),
+    : history_service_(history_service),
       history_client_(history_client),
       history_dir_(history_dir),
       languages_(languages),
@@ -96,33 +92,19 @@ InMemoryURLIndex::InMemoryURLIndex(Profile* profile,
       save_cache_observer_(NULL),
       shutdown_(false),
       restored_(false),
-      needs_to_be_cached_(false),
-      history_service_observer_(this) {
+      needs_to_be_cached_(false) {
   InitializeSchemeWhitelist(&scheme_whitelist_);
   // TODO(mrossetti): Register for language change notifications.
   if (history_service_)
-    history_service_observer_.Add(history_service_);
-}
-
-// Called only by unit tests.
-InMemoryURLIndex::InMemoryURLIndex()
-    : profile_(NULL),
-      history_service_(nullptr),
-      history_client_(NULL),
-      private_data_(new URLIndexPrivateData),
-      restore_cache_observer_(NULL),
-      save_cache_observer_(NULL),
-      shutdown_(false),
-      restored_(false),
-      needs_to_be_cached_(false),
-      history_service_observer_(this) {
-  InitializeSchemeWhitelist(&scheme_whitelist_);
+    history_service_->AddObserver(this);
 }
 
 InMemoryURLIndex::~InMemoryURLIndex() {
   // If there was a history directory (which there won't be for some unit tests)
   // then insure that the cache has already been saved.
   DCHECK(history_dir_.empty() || !needs_to_be_cached_);
+  DCHECK(!history_service_);
+  DCHECK(shutdown_);
 }
 
 void InMemoryURLIndex::Init() {
@@ -130,7 +112,10 @@ void InMemoryURLIndex::Init() {
 }
 
 void InMemoryURLIndex::ShutDown() {
-  history_service_observer_.RemoveAll();
+  if (history_service_) {
+    history_service_->RemoveObserver(this);
+    history_service_ = nullptr;
+  }
   cache_reader_tracker_.TryCancelAll();
   shutdown_ = true;
   base::FilePath path;
@@ -263,7 +248,7 @@ void InMemoryURLIndex::OnCacheLoadDone(
     restored_ = true;
     if (restore_cache_observer_)
       restore_cache_observer_->OnCacheRestoreFinished(true);
-  } else if (profile_) {
+  } else if (history_service_) {
     // When unable to restore from the cache file delete the cache file, if
     // it exists, and then rebuild from the history database if it's available,
     // otherwise wait until the history database loaded and then rebuild.
@@ -272,15 +257,8 @@ void InMemoryURLIndex::OnCacheLoadDone(
       return;
     content::BrowserThread::PostBlockingPoolTask(
         FROM_HERE, base::Bind(DeleteCacheFile, path));
-    HistoryService* service =
-        HistoryServiceFactory::GetForProfileWithoutCreating(profile_);
-    if (service) {
-      if (!service->backend_loaded()) {
-        if (!history_service_observer_.IsObserving(service))
-          history_service_observer_.Add(service);
-      } else {
-        ScheduleRebuildFromHistory();
-      }
+    if (history_service_->backend_loaded()) {
+      ScheduleRebuildFromHistory();
     }
   }
 }
@@ -288,9 +266,8 @@ void InMemoryURLIndex::OnCacheLoadDone(
 // Restoring from the History DB -----------------------------------------------
 
 void InMemoryURLIndex::ScheduleRebuildFromHistory() {
-  HistoryService* service = HistoryServiceFactory::GetForProfile(
-      profile_, ServiceAccessType::EXPLICIT_ACCESS);
-  service->ScheduleDBTask(
+  DCHECK(history_service_);
+  history_service_->ScheduleDBTask(
       scoped_ptr<history::HistoryDBTask>(
           new InMemoryURLIndex::RebuildPrivateDataFromHistoryDBTask(
               this, languages_, scheme_whitelist_)),

chrome/browser/history/in_memory_url_index.h

@@ -16,7 +16,6 @@
 #include "base/gtest_prod_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
-#include "base/scoped_observer.h"
 #include "base/strings/string16.h"
 #include "base/task/cancelable_task_tracker.h"
 #include "chrome/browser/history/scored_history_match.h"
@@ -27,7 +26,6 @@
 
 class HistoryService;
 class HistoryQuickProviderTest;
-class Profile;
 
 namespace base {
 class Time;
@@ -91,13 +89,13 @@ class InMemoryURLIndex : public HistoryServiceObserver,
     virtual void OnCacheSaveFinished(bool succeeded) = 0;
   };
 
-  // |profile|, which may be NULL during unit testing, is used to register for
-  // history changes. |history_dir| is a path to the directory containing the
-  // history database within the profile wherein the cache and transaction
-  // journals will be stored. |languages| gives a list of language encodings by
-  // which URLs and omnibox searches are broken down into words and characters.
-  InMemoryURLIndex(Profile* profile,
-                   HistoryService* history_service,
+  // |history_service| which may be null during unit testing is used to register
+  // |as an HistoryServiceObserver. |history_dir| is a path to the directory
+  // containing the history database within the profile wherein the cache and
+  // transaction journals will be stored. |languages| gives a list of language
+  // encodings by which URLs and omnibox searches are broken down into words and
+  // characters.
+  InMemoryURLIndex(HistoryService* history_service,
                    const base::FilePath& history_dir,
                    const std::string& languages,
                    HistoryClient* client);
@@ -148,9 +146,6 @@ class InMemoryURLIndex : public HistoryServiceObserver,
   FRIEND_TEST_ALL_PREFIXES(InMemoryURLIndexTest, ExpireRow);
   FRIEND_TEST_ALL_PREFIXES(LimitedInMemoryURLIndexTest, Initialization);
 
-  // Creating one of me without a history path is not allowed (tests excepted).
-  InMemoryURLIndex();
-
   // HistoryDBTask used to rebuild our private data from the history database.
   class RebuildPrivateDataFromHistoryDBTask : public HistoryDBTask {
    public:
@@ -167,7 +162,7 @@ class InMemoryURLIndex : public HistoryServiceObserver,
     ~RebuildPrivateDataFromHistoryDBTask() override;
 
     InMemoryURLIndex* index_;  // Call back to this index at completion.
-    std::string languages_;  // Languages for word-breaking.
+    std::string languages_;    // Languages for word-breaking.
     std::set<std::string> scheme_whitelist_;  // Schemes to be indexed.
     bool succeeded_;  // Indicates if the rebuild was successful.
     scoped_refptr<URLIndexPrivateData> data_;  // The rebuilt private data.
@@ -185,8 +180,8 @@ class InMemoryURLIndex : public HistoryServiceObserver,
   // provided as a hook for unit testing.)
   bool GetCacheFilePath(base::FilePath* file_path);
 
-  // Restores the index's private data from the cache file stored in the
-  // profile directory.
+  // Restores the index's private data from the cache file stored in the history
+  // directory.
   void PostRestoreFromCacheFileTask();
 
   // Schedules a history task to rebuild our private data from the history
@@ -219,7 +214,7 @@ class InMemoryURLIndex : public HistoryServiceObserver,
   void OnCacheRestored(URLIndexPrivateData* private_data);
 
   // Posts a task to cache the index private data and write the cache file to
-  // the profile directory.
+  // the history directory.
   void PostSaveToCacheFileTask();
 
   // Saves private_data_ to the given |path|. Runs on the UI thread.
@@ -263,16 +258,15 @@ class InMemoryURLIndex : public HistoryServiceObserver,
   // Returns the set of whitelisted schemes. For unit testing only.
   const std::set<std::string>& scheme_whitelist() { return scheme_whitelist_; }
 
-  // The profile, may be null when testing.
-  Profile* profile_;
+  // The HistoryService; may be null when testing.
   HistoryService* history_service_;
 
-  // The HistoryClient; may be NULL when testing.
+  // The HistoryClient; may be null when testing.
   HistoryClient* history_client_;
 
   // Directory where cache file resides. This is, except when unit testing,
-  // the same directory in which the profile's history database is found. It
-  // should never be empty.
+  // the same directory in which the history database is found. It should never
+  // be empty.
   base::FilePath history_dir_;
 
   // Languages used during the word-breaking process during indexing.
@@ -303,9 +297,6 @@ class InMemoryURLIndex : public HistoryServiceObserver,
   // index has been destructed.
   bool needs_to_be_cached_;
 
-  ScopedObserver<HistoryService, HistoryServiceObserver>
-      history_service_observer_;
-
   DISALLOW_COPY_AND_ASSIGN(InMemoryURLIndex);
 };
 

chrome/browser/history/in_memory_url_index_unittest.cc

@@ -35,6 +35,7 @@ using base::ASCIIToUTF16;
 
 namespace {
 const size_t kMaxMatches = 3;
+const char kTestLanguages[] = "en,ja,hi,zh";
 }  // namespace
 
 // The test version of the history url database table ('url') is contained in
@@ -88,11 +89,18 @@ class InMemoryURLIndexTest : public testing::Test {
  protected:
   // Test setup.
   void SetUp() override;
+  void TearDown() override;
 
   // Allows the database containing the test data to be customized by
   // subclasses.
   virtual base::FilePath::StringType TestDBName() const;
 
+  // Allows the test to control when the InMemoryURLIndex is initialized.
+  virtual bool InitializeInMemoryURLIndexInSetUp() const;
+
+  // Initialize the InMemoryURLIndex for the tests.
+  void InitializeInMemoryURLIndex();
+
   // Validates that the given |term| is contained in |cache| and that it is
   // marked as in-use.
   void CheckTerm(const URLIndexPrivateData::SearchTermCacheMap& cache,
@@ -123,10 +131,9 @@ class InMemoryURLIndexTest : public testing::Test {
                               const URLIndexPrivateData& actual);
 
   content::TestBrowserThreadBundle thread_bundle_;
+  scoped_ptr<InMemoryURLIndex> url_index_;
   TestingProfile profile_;
   HistoryService* history_service_;
-
-  scoped_ptr<InMemoryURLIndex> url_index_;
   HistoryDatabase* history_database_;
 };
 
@@ -273,19 +280,34 @@ void InMemoryURLIndexTest::SetUp() {
     transaction.Commit();
   }
 
-  url_index_.reset(new InMemoryURLIndex(&profile_,
-                                        history_service_,
-                                        base::FilePath(),
-                                        "en,ja,hi,zh",
-                                        history_service_->history_client()));
-  url_index_->Init();
-  url_index_->RebuildFromHistory(history_database_);
+  if (InitializeInMemoryURLIndexInSetUp())
+    InitializeInMemoryURLIndex();
+}
+
+void InMemoryURLIndexTest::TearDown() {
+  // Ensure that the InMemoryURLIndex no longer observer HistoryService before
+  // it is destroyed in order to prevent HistoryService calling dead observer.
+  if (url_index_)
+    url_index_->ShutDown();
 }
 
 base::FilePath::StringType InMemoryURLIndexTest::TestDBName() const {
     return FILE_PATH_LITERAL("url_history_provider_test.db.txt");
 }
 
+bool InMemoryURLIndexTest::InitializeInMemoryURLIndexInSetUp() const {
+  return true;
+}
+
+void InMemoryURLIndexTest::InitializeInMemoryURLIndex() {
+  DCHECK(!url_index_);
+  url_index_.reset(new InMemoryURLIndex(history_service_, base::FilePath(),
+                                        kTestLanguages,
+                                        history_service_->history_client()));
+  url_index_->Init();
+  url_index_->RebuildFromHistory(history_database_);
+}
+
 void InMemoryURLIndexTest::CheckTerm(
     const URLIndexPrivateData::SearchTermCacheMap& cache,
     base::string16 term) const {
@@ -420,12 +442,17 @@ void InMemoryURLIndexTest::ExpectPrivateDataEqual(
 class LimitedInMemoryURLIndexTest : public InMemoryURLIndexTest {
  protected:
   base::FilePath::StringType TestDBName() const override;
+  bool InitializeInMemoryURLIndexInSetUp() const override;
 };
 
 base::FilePath::StringType LimitedInMemoryURLIndexTest::TestDBName() const {
   return FILE_PATH_LITERAL("url_history_provider_test_limited.db.txt");
 }
 
+bool LimitedInMemoryURLIndexTest::InitializeInMemoryURLIndexInSetUp() const {
+  return false;
+}
+
 TEST_F(LimitedInMemoryURLIndexTest, Initialization) {
   // Verify that the database contains the expected number of items, which
   // is the pre-filtered count, i.e. all of the items.
@@ -434,13 +461,8 @@ TEST_F(LimitedInMemoryURLIndexTest, Initialization) {
   uint64 row_count = 0;
   while (statement.Step()) ++row_count;
   EXPECT_EQ(1U, row_count);
-  url_index_.reset(new InMemoryURLIndex(&profile_,
-                                        history_service_,
-                                        base::FilePath(),
-                                        "en,ja,hi,zh",
-                                        history_service_->history_client()));
-  url_index_->Init();
-  url_index_->RebuildFromHistory(history_database_);
+
+  InitializeInMemoryURLIndex();
   URLIndexPrivateData& private_data(*GetPrivateData());
 
   // history_info_map_ should have the same number of items as were filtered.
@@ -1173,6 +1195,7 @@ class InMemoryURLIndexCacheTest : public testing::Test {
 
  protected:
   void SetUp() override;
+  void TearDown() override;
 
   // Pass-through functions to simplify our friendship with InMemoryURLIndex.
   void set_history_dir(const base::FilePath& dir_path);
@@ -1184,10 +1207,14 @@ class InMemoryURLIndexCacheTest : public testing::Test {
 
 void InMemoryURLIndexCacheTest::SetUp() {
   ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
-  HistoryClient history_client;
   base::FilePath path(temp_dir_.path());
-  url_index_.reset(new InMemoryURLIndex(
-      NULL, nullptr, path, "en,ja,hi,zh", &history_client));
+  url_index_.reset(
+      new InMemoryURLIndex(nullptr, path, kTestLanguages, nullptr));
+}
+
+void InMemoryURLIndexCacheTest::TearDown() {
+  if (url_index_)
+    url_index_->ShutDown();
 }
 
 void InMemoryURLIndexCacheTest::set_history_dir(