Fix sync leaks and some more good stuff.

This is a continuation of zork's change http://codereview.chromium.org/1354001 that adds some preventitive DCHECKs througout the sync code to make sure stuff happens on the UI thread.

This also includes a leak fix in the ProfileSyncServiceTypedUrlTest.

The final change is changing the TestingProfile to return a ProfileSyncServiceMock rather than a real ProfileSyncService.  This should help prevent random test failes due to other tests that need to use the PSS.

BUG=38490,38487

Review URL: http://codereview.chromium.org/1383002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@43102 0039d316-1c4b-4281-b951-d872f2087c98

16 files changed, 67 insertions, 10 deletions

diff --git a/chrome/browser/sync/glue/autofill_data_type_controller.cc b/chrome/browser/sync/glue/autofill_data_type_controller.cc
index 3857459..10fb9ec 100644
--- a/chrome/browser/sync/glue/autofill_data_type_controller.cc
+++ b/chrome/browser/sync/glue/autofill_data_type_controller.cc
@@ -27,12 +27,14 @@ AutofillDataTypeController::AutofillDataTypeController(
       sync_service_(sync_service),
       state_(NOT_RUNNING),
       merge_allowed_(false) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
   DCHECK(profile_sync_factory);
   DCHECK(profile);
   DCHECK(sync_service);
 }
 
 AutofillDataTypeController::~AutofillDataTypeController() {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
 }
 
 void AutofillDataTypeController::Start(bool merge_allowed,
diff --git a/chrome/browser/sync/glue/autofill_model_associator.cc b/chrome/browser/sync/glue/autofill_model_associator.cc
index 06d3427..25175e7 100644
--- a/chrome/browser/sync/glue/autofill_model_associator.cc
+++ b/chrome/browser/sync/glue/autofill_model_associator.cc
@@ -7,6 +7,7 @@
 #include <vector>
 
 #include "base/utf_string_conversions.h"
+#include "chrome/browser/chrome_thread.h"
 #include "chrome/browser/profile.h"
 #include "chrome/browser/sync/engine/syncapi.h"
 #include "chrome/browser/sync/glue/autofill_change_processor.h"
@@ -27,11 +28,16 @@ AutofillModelAssociator::AutofillModelAssociator(
       web_database_(web_database),
       error_handler_(error_handler),
       autofill_node_id_(sync_api::kInvalidId) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::DB));
   DCHECK(sync_service_);
   DCHECK(web_database_);
   DCHECK(error_handler_);
 }
 
+AutofillModelAssociator::~AutofillModelAssociator() {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::DB));
+}
+
 bool AutofillModelAssociator::AssociateModels() {
   LOG(INFO) << "Associating Autofill Models";
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::DB));
@@ -193,6 +199,7 @@ int64 AutofillModelAssociator::GetSyncIdFromChromeId(
 
 void AutofillModelAssociator::Associate(
     const AutofillKey* autofill, int64 sync_id) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::DB));
   DCHECK_NE(sync_api::kInvalidId, sync_id);
   DCHECK(id_map_.find(*autofill) == id_map_.end());
   DCHECK(id_map_inverse_.find(sync_id) == id_map_inverse_.end());
@@ -201,6 +208,7 @@ void AutofillModelAssociator::Associate(
 }
 
 void AutofillModelAssociator::Disassociate(int64 sync_id) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::DB));
   SyncIdToAutofillMap::iterator iter = id_map_inverse_.find(sync_id);
   if (iter == id_map_inverse_.end())
     return;
diff --git a/chrome/browser/sync/glue/autofill_model_associator.h b/chrome/browser/sync/glue/autofill_model_associator.h
index 41a6387..b9031ed 100644
--- a/chrome/browser/sync/glue/autofill_model_associator.h
+++ b/chrome/browser/sync/glue/autofill_model_associator.h
@@ -38,7 +38,7 @@ class AutofillModelAssociator
   AutofillModelAssociator(ProfileSyncService* sync_service,
                           WebDatabase* web_database,
                           UnrecoverableErrorHandler* error_handler);
-  virtual ~AutofillModelAssociator() { }
+  virtual ~AutofillModelAssociator();
 
   // PerDataTypeAssociatorInterface implementation.
   //
diff --git a/chrome/browser/sync/glue/bookmark_change_processor.cc b/chrome/browser/sync/glue/bookmark_change_processor.cc
index 5a84923..92a633f 100644
--- a/chrome/browser/sync/glue/bookmark_change_processor.cc
+++ b/chrome/browser/sync/glue/bookmark_change_processor.cc
@@ -9,6 +9,7 @@
 
 #include "base/string_util.h"
 #include "chrome/browser/bookmarks/bookmark_utils.h"
+#include "chrome/browser/chrome_thread.h"
 #include "chrome/browser/favicon_service.h"
 #include "chrome/browser/profile.h"
 #include "chrome/browser/sync/profile_sync_service.h"
@@ -23,11 +24,13 @@ BookmarkChangeProcessor::BookmarkChangeProcessor(
     : ChangeProcessor(error_handler),
       bookmark_model_(NULL),
       model_associator_(model_associator) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
   DCHECK(model_associator);
   DCHECK(error_handler);
 }
 
 void BookmarkChangeProcessor::StartImpl(Profile* profile) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
   DCHECK(!bookmark_model_);
   bookmark_model_ = profile->GetBookmarkModel();
   DCHECK(bookmark_model_->IsLoaded());
@@ -35,6 +38,7 @@ void BookmarkChangeProcessor::StartImpl(Profile* profile) {
 }
 
 void BookmarkChangeProcessor::StopImpl() {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
   DCHECK(bookmark_model_);
   bookmark_model_->RemoveObserver(this);
   bookmark_model_ = NULL;
@@ -128,8 +132,8 @@ void BookmarkChangeProcessor::BookmarkModelBeingDeleted(
 }
 
 void BookmarkChangeProcessor::BookmarkNodeAdded(BookmarkModel* model,
-                                                  const BookmarkNode* parent,
-                                                  int index) {
+                                                const BookmarkNode* parent,
+                                                int index) {
   DCHECK(running());
   DCHECK(share_handle());
 
diff --git a/chrome/browser/sync/glue/bookmark_data_type_controller.cc b/chrome/browser/sync/glue/bookmark_data_type_controller.cc
index 60ed68d..3705b31 100644
--- a/chrome/browser/sync/glue/bookmark_data_type_controller.cc
+++ b/chrome/browser/sync/glue/bookmark_data_type_controller.cc
@@ -29,12 +29,14 @@ BookmarkDataTypeController::BookmarkDataTypeController(
       state_(NOT_RUNNING),
       merge_allowed_(false),
       unrecoverable_error_detected_(false) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
   DCHECK(profile_sync_factory);
   DCHECK(profile);
   DCHECK(sync_service);
 }
 
 BookmarkDataTypeController::~BookmarkDataTypeController() {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
 }
 
 void BookmarkDataTypeController::Start(bool merge_allowed,
@@ -103,12 +105,14 @@ void BookmarkDataTypeController::OnUnrecoverableError() {
 void BookmarkDataTypeController::Observe(NotificationType type,
                                          const NotificationSource& source,
                                          const NotificationDetails& details) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
   DCHECK_EQ(NotificationType::BOOKMARK_MODEL_LOADED, type.value);
   registrar_.RemoveAll();
   Associate();
 }
 
 void BookmarkDataTypeController::Associate() {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
   DCHECK_EQ(state_, MODEL_STARTING);
   state_ = ASSOCIATING;
 
@@ -148,11 +152,13 @@ void BookmarkDataTypeController::Associate() {
 }
 
 void BookmarkDataTypeController::FinishStart(StartResult result) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
   start_callback_->Run(result);
   start_callback_.reset();
 }
 
 void BookmarkDataTypeController::StartFailed(StartResult result) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
   model_associator_.reset();
   change_processor_.reset();
   state_ = NOT_RUNNING;
diff --git a/chrome/browser/sync/glue/bookmark_model_associator.cc b/chrome/browser/sync/glue/bookmark_model_associator.cc
index aaddd40..ab38942 100644
--- a/chrome/browser/sync/glue/bookmark_model_associator.cc
+++ b/chrome/browser/sync/glue/bookmark_model_associator.cc
@@ -10,6 +10,7 @@
 #include "base/message_loop.h"
 #include "base/task.h"
 #include "chrome/browser/bookmarks/bookmark_model.h"
+#include "chrome/browser/chrome_thread.h"
 #include "chrome/browser/profile.h"
 #include "chrome/browser/sync/engine/syncapi.h"
 #include "chrome/browser/sync/glue/bookmark_change_processor.h"
@@ -158,9 +159,14 @@ BookmarkModelAssociator::BookmarkModelAssociator(
     : sync_service_(sync_service),
       error_handler_(error_handler),
       ALLOW_THIS_IN_INITIALIZER_LIST(persist_associations_(this)) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
   DCHECK(sync_service_);
 }
 
+BookmarkModelAssociator::~BookmarkModelAssociator() {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
+}
+
 bool BookmarkModelAssociator::DisassociateModels() {
   id_map_.clear();
   id_map_inverse_.clear();
@@ -194,6 +200,7 @@ bool BookmarkModelAssociator::InitSyncNodeFromChromeId(
 
 void BookmarkModelAssociator::Associate(const BookmarkNode* node,
                                         int64 sync_id) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
   int64 node_id = node->id();
   DCHECK_NE(sync_id, sync_api::kInvalidId);
   DCHECK(id_map_.find(node_id) == id_map_.end());
@@ -205,6 +212,7 @@ void BookmarkModelAssociator::Associate(const BookmarkNode* node,
 }
 
 void BookmarkModelAssociator::Disassociate(int64 sync_id) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
   SyncIdToBookmarkNodeMap::iterator iter = id_map_inverse_.find(sync_id);
   if (iter == id_map_inverse_.end())
     return;
diff --git a/chrome/browser/sync/glue/bookmark_model_associator.h b/chrome/browser/sync/glue/bookmark_model_associator.h
index 2fd96bd..9df77cd 100644
--- a/chrome/browser/sync/glue/bookmark_model_associator.h
+++ b/chrome/browser/sync/glue/bookmark_model_associator.h
@@ -38,7 +38,7 @@ class BookmarkModelAssociator
   static syncable::ModelType model_type() { return syncable::BOOKMARKS; }
   BookmarkModelAssociator(ProfileSyncService* sync_service,
                           UnrecoverableErrorHandler* error_handler);
-  virtual ~BookmarkModelAssociator() { }
+  virtual ~BookmarkModelAssociator();
 
   // AssociatorInterface implementation.
   //
diff --git a/chrome/browser/sync/glue/preference_change_processor.cc b/chrome/browser/sync/glue/preference_change_processor.cc
index d363c06..00ca62c 100644
--- a/chrome/browser/sync/glue/preference_change_processor.cc
+++ b/chrome/browser/sync/glue/preference_change_processor.cc
@@ -9,6 +9,7 @@
 
 #include "base/json/json_reader.h"
 #include "base/utf_string_conversions.h"
+#include "chrome/browser/chrome_thread.h"
 #include "chrome/browser/profile.h"
 #include "chrome/browser/sync/glue/preference_model_associator.h"
 #include "chrome/browser/sync/profile_sync_service.h"
@@ -25,13 +26,19 @@ PreferenceChangeProcessor::PreferenceChangeProcessor(
     : ChangeProcessor(error_handler),
       pref_service_(NULL),
       model_associator_(model_associator) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
   DCHECK(model_associator);
   DCHECK(error_handler);
 }
 
+PreferenceChangeProcessor::~PreferenceChangeProcessor(){
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
+}
+
 void PreferenceChangeProcessor::Observe(NotificationType type,
                                         const NotificationSource& source,
                                         const NotificationDetails& details) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
   DCHECK(running());
   DCHECK(NotificationType::PREF_CHANGED == type);
   DCHECK_EQ(pref_service_, Source<PrefService>(source).ptr());
@@ -68,6 +75,7 @@ void PreferenceChangeProcessor::ApplyChangesFromSyncModel(
     const sync_api::BaseTransaction* trans,
     const sync_api::SyncManager::ChangeRecord* changes,
     int change_count) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
   if (!running())
     return;
   StopObserving();
@@ -146,11 +154,13 @@ Value* PreferenceChangeProcessor::ReadPreference(
 }
 
 void PreferenceChangeProcessor::StartImpl(Profile* profile) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
   pref_service_ = profile->GetPrefs();
   StartObserving();
 }
 
 void PreferenceChangeProcessor::StopImpl() {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
   StopObserving();
   pref_service_ = NULL;
 }
diff --git a/chrome/browser/sync/glue/preference_change_processor.h b/chrome/browser/sync/glue/preference_change_processor.h
index eaae008..f00ca2e 100644
--- a/chrome/browser/sync/glue/preference_change_processor.h
+++ b/chrome/browser/sync/glue/preference_change_processor.h
@@ -25,7 +25,7 @@ class PreferenceChangeProcessor : public ChangeProcessor,
  public:
   PreferenceChangeProcessor(PreferenceModelAssociator* model_associator,
                             UnrecoverableErrorHandler* error_handler);
-  virtual ~PreferenceChangeProcessor() {}
+  virtual ~PreferenceChangeProcessor();
 
   // NotificationObserver implementation.
   // PrefService -> sync_api model change application.
diff --git a/chrome/browser/sync/glue/preference_data_type_controller.cc b/chrome/browser/sync/glue/preference_data_type_controller.cc
index e7ddb1f..a3c52c6 100644
--- a/chrome/browser/sync/glue/preference_data_type_controller.cc
+++ b/chrome/browser/sync/glue/preference_data_type_controller.cc
@@ -22,11 +22,13 @@ PreferenceDataTypeController::PreferenceDataTypeController(
       sync_service_(sync_service),
       state_(NOT_RUNNING),
       unrecoverable_error_detected_(false) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
   DCHECK(profile_sync_factory);
   DCHECK(sync_service);
 }
 
 PreferenceDataTypeController::~PreferenceDataTypeController() {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
 }
 
 void PreferenceDataTypeController::Start(bool merge_allowed,
@@ -105,11 +107,13 @@ void PreferenceDataTypeController::OnUnrecoverableError() {
 }
 
 void PreferenceDataTypeController::FinishStart(StartResult result) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
   start_callback_->Run(result);
   start_callback_.reset();
 }
 
 void PreferenceDataTypeController::StartFailed(StartResult result) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
   model_associator_.reset();
   change_processor_.reset();
   start_callback_->Run(result);
diff --git a/chrome/browser/sync/glue/preference_model_associator.cc b/chrome/browser/sync/glue/preference_model_associator.cc
index ffd30f0..28ba96f 100644
--- a/chrome/browser/sync/glue/preference_model_associator.cc
+++ b/chrome/browser/sync/glue/preference_model_associator.cc
@@ -8,6 +8,7 @@
 #include "base/logging.h"
 #include "base/values.h"
 #include "base/utf_string_conversions.h"
+#include "chrome/browser/chrome_thread.h"
 #include "chrome/browser/pref_service.h"
 #include "chrome/browser/profile.h"
 #include "chrome/browser/sync/engine/syncapi.h"
@@ -24,6 +25,7 @@ PreferenceModelAssociator::PreferenceModelAssociator(
     : sync_service_(sync_service),
       error_handler_(error_handler),
       preferences_node_id_(sync_api::kInvalidId) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
   DCHECK(sync_service_);
   synced_preferences_.insert(prefs::kHomePageIsNewTabPage);
   synced_preferences_.insert(prefs::kHomePage);
@@ -33,7 +35,12 @@ PreferenceModelAssociator::PreferenceModelAssociator(
   synced_preferences_.insert(prefs::kShowHomeButton);
 }
 
+PreferenceModelAssociator::~PreferenceModelAssociator() {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
+}
+
 bool PreferenceModelAssociator::AssociateModels() {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
   PrefService* pref_service = sync_service_->profile()->GetPrefs();
 
   int64 root_id;
@@ -167,6 +174,7 @@ int64 PreferenceModelAssociator::GetSyncIdFromChromeId(
 
 void PreferenceModelAssociator::Associate(
     const PrefService::Preference* preference, int64 sync_id) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
   DCHECK_NE(sync_api::kInvalidId, sync_id);
   DCHECK(id_map_.find(preference->name()) == id_map_.end());
   DCHECK(id_map_inverse_.find(sync_id) == id_map_inverse_.end());
@@ -175,6 +183,7 @@ void PreferenceModelAssociator::Associate(
 }
 
 void PreferenceModelAssociator::Disassociate(int64 sync_id) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
   SyncIdToPreferenceNameMap::iterator iter = id_map_inverse_.find(sync_id);
   if (iter == id_map_inverse_.end())
     return;
diff --git a/chrome/browser/sync/glue/preference_model_associator.h b/chrome/browser/sync/glue/preference_model_associator.h
index 8f0bbca..ca66e30 100644
--- a/chrome/browser/sync/glue/preference_model_associator.h
+++ b/chrome/browser/sync/glue/preference_model_associator.h
@@ -32,7 +32,7 @@ class PreferenceModelAssociator
   static syncable::ModelType model_type() { return syncable::PREFERENCES; }
   PreferenceModelAssociator(ProfileSyncService* sync_service,
                             UnrecoverableErrorHandler* error_handler);
-  virtual ~PreferenceModelAssociator() { }
+  virtual ~PreferenceModelAssociator();
 
   // Returns the list of preference names that should be monitored for changes.
   const std::set<std::wstring>& synced_preferences() {
diff --git a/chrome/browser/sync/glue/typed_url_data_type_controller.cc b/chrome/browser/sync/glue/typed_url_data_type_controller.cc
index 831f2ae..ec5c4e1 100644
--- a/chrome/browser/sync/glue/typed_url_data_type_controller.cc
+++ b/chrome/browser/sync/glue/typed_url_data_type_controller.cc
@@ -54,12 +54,14 @@ TypedUrlDataTypeController::TypedUrlDataTypeController(
       sync_service_(sync_service),
       state_(NOT_RUNNING),
       merge_allowed_(false) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
   DCHECK(profile_sync_factory);
   DCHECK(profile);
   DCHECK(sync_service);
 }
 
 TypedUrlDataTypeController::~TypedUrlDataTypeController() {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
 }
 
 void TypedUrlDataTypeController::Start(bool merge_allowed,
diff --git a/chrome/browser/sync/profile_sync_service.h b/chrome/browser/sync/profile_sync_service.h
index d9d8f65..283c4bd 100644
--- a/chrome/browser/sync/profile_sync_service.h
+++ b/chrome/browser/sync/profile_sync_service.h
@@ -104,7 +104,7 @@ class ProfileSyncService : public browser_sync::SyncFrontend,
   virtual void DisableForUser();
 
   // Whether sync is enabled by user or not.
-  bool HasSyncSetupCompleted() const;
+  virtual bool HasSyncSetupCompleted() const;
   void SetSyncSetupCompleted();
 
   // SyncFrontend implementation.
@@ -186,8 +186,8 @@ class ProfileSyncService : public browser_sync::SyncFrontend,
 
   // Adds/removes an observer. ProfileSyncService does not take ownership of
   // the observer.
-  void AddObserver(Observer* observer);
-  void RemoveObserver(Observer* observer);
+  virtual void AddObserver(Observer* observer);
+  virtual void RemoveObserver(Observer* observer);
 
   // Record stats on various events.
   static void SyncEvent(SyncEventCodes code);
diff --git a/chrome/browser/sync/profile_sync_service_mock.h b/chrome/browser/sync/profile_sync_service_mock.h
index 22e7f9e..2be8b23 100644
--- a/chrome/browser/sync/profile_sync_service_mock.h
+++ b/chrome/browser/sync/profile_sync_service_mock.h
@@ -39,6 +39,9 @@ class ProfileSyncServiceMock : public ProfileSyncService {
                     browser_sync::ChangeProcessor* change_processor));
 
   MOCK_METHOD0(InitializeBackend, void());
+  MOCK_METHOD1(AddObserver, void(Observer*));
+  MOCK_METHOD1(RemoveObserver, void(Observer*));
+  MOCK_CONST_METHOD0(HasSyncSetupCompleted, bool());
 };
 
 #endif  // CHROME_BROWSER_SYNC_PROFILE_SYNC_SERVICE_MOCK_H_
diff --git a/chrome/browser/sync/profile_sync_service_typed_url_unittest.cc b/chrome/browser/sync/profile_sync_service_typed_url_unittest.cc
index fe04d94ac3..716cbe2 100644
--- a/chrome/browser/sync/profile_sync_service_typed_url_unittest.cc
+++ b/chrome/browser/sync/profile_sync_service_typed_url_unittest.cc
@@ -93,10 +93,11 @@ class RunOnDBThreadTask : public Task {
       : backend_(backend), task_(task) {}
   virtual void Run() {
     task_->RunOnDBThread(backend_, NULL);
+    task_ = NULL;
   }
  private:
   HistoryBackend* backend_;
-  HistoryDBTask* task_;
+  scoped_refptr<HistoryDBTask> task_;
 };
 
 ACTION_P2(RunTaskOnDBThread, thread, backend) {