[Sync] Remove now-unneeded initialized_mutex_.

chrome/browser/sync/engine/syncapi.cc

Index: chrome/browser/sync/engine/syncapi.cc
diff --git a/chrome/browser/sync/engine/syncapi.cc b/chrome/browser/sync/engine/syncapi.cc
index ef3f5b43659ceff68594c5bb28996f9b732dd60e..d5775f9c14d3448c8fe3bd0987c602ef32804837 100644
--- a/chrome/browser/sync/engine/syncapi.cc
+++ b/chrome/browser/sync/engine/syncapi.cc
@@ -26,7 +26,6 @@
 #include "base/sha1.h"
 #include "base/string_number_conversions.h"
 #include "base/string_util.h"
-#include "base/synchronization/lock.h"
 #include "base/task.h"
 #include "base/time.h"
 #include "base/utf_string_conversions.h"
@@ -1259,7 +1258,10 @@ class SyncManager::SyncInternal
     return connection_manager_.get();
   }
   SyncScheduler* scheduler() { return scheduler_.get(); }
-  UserShare* GetUserShare() { return &share_; }
+  UserShare* GetUserShare() {
+    DCHECK(initialized_);
+    return &share_;
+  }
 
   // Return the currently active (validated) username for use with syncable
   // types.
@@ -1280,13 +1282,6 @@ class SyncManager::SyncInternal
   // See SyncManager::Shutdown for information.
   void Shutdown();
 
-  // Whether we're initialized to the point of being able to accept changes
-  // (and hence allow transaction creation). See initialized_ for details.
-  bool initialized() const {
-    base::AutoLock lock(initialized_mutex_);
-    return initialized_;
-  }
-
   // If this is a deletion for a password, sets the legacy
   // ExtraPasswordChangeRecordData field of |buffer|. Otherwise sets
   // |buffer|'s specifics field to contain the unencrypted data.
@@ -1400,11 +1395,6 @@ class SyncManager::SyncInternal
   // available.
   void RaiseAuthNeededEvent();
 
-  // Helper to set initialized_ to true and raise an event to clients to notify
-  // that initialization is complete and it is safe to send us changes. If
-  // already initialized, this is a no-op.
-  void MarkAndNotifyInitializationComplete();
-
   // Sends notifications to peers.
   void SendNotification();
 
@@ -1583,15 +1573,8 @@ class SyncManager::SyncInternal
   // The instance is shared between the SyncManager and the Syncer.
   ModelSafeWorkerRegistrar* registrar_;
 
-  // Set to true once Init has been called, and we know of an
-  // authenticated valid) username either from a fresh authentication
-  // attempt (as in first-use case) or from a previous attempt stored
-  // in our UserSettings (as in the steady-state), and the
-  // syncable::Directory has been opened, meaning we are ready to
-  // accept changes.  Protected by initialized_mutex_ as it can get
-  // read/set by both the SyncScheduler and the AuthWatcherThread.
+  // Set to true once Init has been called.
   bool initialized_;
-  mutable base::Lock initialized_mutex_;
 
   // True if the SyncManager should be running in test mode (no sync
   // scheduler actually communicating with the server).
@@ -1743,6 +1726,7 @@ bool SyncManager::SyncInternal::Init(
     sync_notifier::SyncNotifier* sync_notifier,
     const std::string& restored_key_for_bootstrapping,
     bool setup_for_test_mode) {
+  CHECK(!initialized_);
 
   VLOG(1) << "Starting SyncInternal initialization.";
 
@@ -1792,9 +1776,15 @@ bool SyncManager::SyncInternal::Init(
         browser_sync::SyncScheduler::CONFIGURATION_MODE, NULL);
   }
 
+  initialized_ = true;
+
   // Do this once the directory is opened.
   BootstrapEncryption(restored_key_for_bootstrapping);
-  MarkAndNotifyInitializationComplete();
+
+  // Notify that initialization is complete.
+  FOR_EACH_OBSERVER(SyncManager::Observer, observers_,
+                    OnInitializationComplete());

[tim (not reviewing), 2011/06/10 01:53:20]

So, in the reviews yesterday to make our observers threadsafe, I noted I didn't
understand where the races were because we should always all init here in a
deterministic fashion.  Here, you don't do the CopyObservers trick (which was
added to MarkAndNotify yesterday), which suggests that there is in fact no race
as I thought.

Am I smoking crack?!

[akalin, 2011/06/14 17:10:25]

The races occurred because the observer add/remove was happening on the
core/sync thread and the notifies were happening from threads that
WriteTransactions are on (UI, DB, etc.).  In this case, initialization happens
on the sync thread, and all the observers are on the sync thread.  We don't even
have an AuthHandler thread anymore, do we?

[akalin, 2011/06/14 21:27:23]

Oh, I see what you were saying.  That was just a merge error, there definitely
needs to be a CopyObserver here (the unit tests caught it).

Fixed.

+
   return signed_in;
 }
 
@@ -1854,26 +1844,6 @@ void SyncManager::SyncInternal::StartSyncingNormally() {
     scheduler()->Start(SyncScheduler::NORMAL_MODE, NULL);
 }
 
-void SyncManager::SyncInternal::MarkAndNotifyInitializationComplete() {
-  // There is only one real time we need this mutex.  If we get an auth
-  // success, and before the initial sync ends we get an auth failure.  In this
-  // case we'll be listening to both the AuthWatcher and Syncer, and it's a race
-  // between their respective threads to call MarkAndNotify.  We need to make
-  // sure the observer is notified once and only once.
-  {
-    base::AutoLock lock(initialized_mutex_);
-    if (initialized_)
-      return;
-    initialized_ = true;
-  }
-
-  // Notify that initialization is complete.
-  ObserverList<SyncManager::Observer> temp_obs_list;
-  CopyObservers(&temp_obs_list);
-  FOR_EACH_OBSERVER(SyncManager::Observer, temp_obs_list,
-                    OnInitializationComplete());
-}
-
 void SyncManager::SyncInternal::SendNotification() {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   if (!sync_notifier_) {
@@ -1884,7 +1854,7 @@ void SyncManager::SyncInternal::SendNotification() {
 }
 
 bool SyncManager::SyncInternal::OpenDirectory() {
-  DCHECK(!initialized()) << "Should only happen once";
+  DCHECK(!initialized_) << "Should only happen once";
 
   bool share_opened = dir_manager()->Open(username_for_share());
   DCHECK(share_opened);
@@ -2561,7 +2531,7 @@ void SyncManager::SyncInternal::OnSyncEngineEvent(
       }
     }
 
-    if (!initialized()) {
+    if (!initialized_) {
       LOG(INFO) << "OnSyncCycleCompleted not sent because sync api is not "
                 << "initialized";
       return;
@@ -2998,7 +2968,6 @@ BaseTransaction::~BaseTransaction() {
 }
 
 UserShare* SyncManager::GetUserShare() const {
-  DCHECK(data_->initialized()) << "GetUserShare requires initialization!";
   return data_->GetUserShare();
 }