[Sync] Remove now-unneeded initialized_mutex_.

chrome/browser/sync/engine/syncapi.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/sync/engine/syncapi.h"
 
 #include <algorithm>
 #include <bitset>
 #include <iomanip>
 #include <list>
 #include <map>
 #include <queue>
 #include <string>
 #include <vector>
 
 #include "base/base64.h"
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/command_line.h"
 #include "base/compiler_specific.h"
 #include "base/json/json_writer.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop.h"
 #include "base/observer_list.h"
 #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"
 #include "base/values.h"
 #include "chrome/browser/sync/engine/all_status.h"
 #include "chrome/browser/sync/engine/change_reorder_buffer.h"
 #include "chrome/browser/sync/engine/model_safe_worker.h"
 #include "chrome/browser/sync/engine/nudge_source.h"
 #include "chrome/browser/sync/engine/net/server_connection_manager.h"
 #include "chrome/browser/sync/engine/net/syncapi_server_connection_manager.h"
@@ skipping 1212 matching lines @@
   bool HaveObservers() const;
   void AddObserver(SyncManager::Observer* observer);
   void RemoveObserver(SyncManager::Observer* observer);
 
   // Accessors for the private members.
   DirectoryManager* dir_manager() { return share_.dir_manager.get(); }
   SyncAPIServerConnectionManager* connection_manager() {
     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.
   const std::string& username_for_share() const {
     return share_.name;
   }
 
   Status GetStatus();
 
   void RequestNudge(const tracked_objects::Location& nudge_location);
 
   void RequestNudgeWithDataTypes(const TimeDelta& delay,
       browser_sync::NudgeSource source, const ModelTypeBitSet& types,
       const tracked_objects::Location& nudge_location);
 
   void RequestEarlyExit();
 
   // 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.
   void SetExtraChangeRecordData(int64 id,
                                 syncable::ModelType type,
                                 ChangeReorderBuffer* buffer,
                                 Cryptographer* cryptographer,
                                 const syncable::EntryKernel& original,
                                 bool existed_before,
                                 bool exists_now);
@@ skipping 93 matching lines @@
       (SyncManager::SyncInternal::*UnboundJsMessageHandler)(
           const browser_sync::JsArgList&);
   typedef base::Callback<browser_sync::JsArgList(browser_sync::JsArgList)>
       JsMessageHandler;
   typedef std::map<std::string, JsMessageHandler> JsMessageHandlerMap;
 
   // Helper to call OnAuthError when no authentication credentials are
   // 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();
 
   // Determine if the parents or predecessors differ between the old and new
   // versions of an entry stored in |a| and |b|.  Note that a node's index may
   // change without its NEXT_ID changing if the node at NEXT_ID also moved (but
   // the relative order is unchanged).  To handle such cases, we rely on the
   // caller to treat a position update on any sibling as updating the positions
   // of all siblings.
   static bool VisiblePositionsDiffer(const syncable::EntryKernel& a,
@@ skipping 158 matching lines @@
   // Event listener hookup for the ServerConnectionManager.
   scoped_ptr<EventListenerHookup> connection_manager_hookup_;
 
   // The sync dir_manager to which we belong.
   SyncManager* const sync_manager_;
 
   // The entity that provides us with information about which types to sync.
   // 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).
   bool setup_for_test_mode_;
 
   ScopedRunnableMethodFactory<SyncManager::SyncInternal> method_factory_;
 
   // Map used to store the notification info to be displayed in
   // about:sync page.
   NotificationInfoMap notification_info_map_;
@@ skipping 131 matching lines @@
     const std::string& sync_server_and_path,
     int port,
     bool use_ssl,
     HttpPostProviderFactory* post_factory,
     ModelSafeWorkerRegistrar* model_safe_worker_registrar,
     const char* user_agent,
     const SyncCredentials& credentials,
     sync_notifier::SyncNotifier* sync_notifier,
     const std::string& restored_key_for_bootstrapping,
     bool setup_for_test_mode) {
+  CHECK(!initialized_);
 
   VLOG(1) << "Starting SyncInternal initialization.";
 
   sync_loop_ = MessageLoop::current();
   DCHECK(sync_loop_);
   registrar_ = model_safe_worker_registrar;
   setup_for_test_mode_ = setup_for_test_mode;
 
   sync_notifier_ = sync_notifier;
   sync_notifier_->AddObserver(this);
@@ skipping 29 matching lines @@
     scheduler_.reset(new SyncScheduler(name_, context, new Syncer()));
   }
 
   bool signed_in = SignIn(credentials);
 
   if (signed_in && scheduler()) {
     scheduler()->Start(
         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;
 }
 
 void SyncManager::SyncInternal::BootstrapEncryption(
     const std::string& restored_key_for_bootstrapping) {
   syncable::ScopedDirLookup lookup(dir_manager(), username_for_share());
   if (!lookup.good()) {
     LOG(INFO) << "BootstrapEncryption: lookup not good so bailing out";
     NOTREACHED();
     return;
@@ skipping 39 matching lines @@
 }
 
 void SyncManager::SyncInternal::StartSyncingNormally() {
   // Start the sync scheduler. This won't actually result in any
   // syncing until at least the DirectoryManager broadcasts the OPENED
   // event, and a valid server connection is detected.
   if (scheduler())  // NULL during certain unittests.
     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_) {
     VLOG(1) << "Not sending notification: sync_notifier_ is NULL";
     return;
   }
   sync_notifier_->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);
   if (!share_opened) {
     ObserverList<SyncManager::Observer> temp_obs_list;
     CopyObservers(&temp_obs_list);
     FOR_EACH_OBSERVER(SyncManager::Observer, temp_obs_list,
                       OnStopSyncingPermanently());
 
     LOG(ERROR) << "Could not open share for:" << username_for_share();
@@ skipping 656 matching lines @@
           FOR_EACH_OBSERVER(SyncManager::Observer, temp_obs_list,
                             OnPassphraseRequired(sync_api::REASON_ENCRYPTION));
         }
         // If everything is in order(we have the passphrase) then there is no
         // need to inform the listeners. They will just wait for sync
         // completion event and if no errors have been raised it means
         // encryption was succesful.
       }
     }
 
-    if (!initialized()) {
+    if (!initialized_) {
       LOG(INFO) << "OnSyncCycleCompleted not sent because sync api is not "
                 << "initialized";
       return;
     }
 
     if (!event.snapshot->has_more_to_sync) {
       VLOG(1) << "OnSyncCycleCompleted sent";
       ObserverList<SyncManager::Observer> temp_obs_list;
       CopyObservers(&temp_obs_list);
       FOR_EACH_OBSERVER(SyncManager::Observer, temp_obs_list,
@@ skipping 416 matching lines @@
                                           share->name);
   cryptographer_ = share->dir_manager->GetCryptographer(this);
   if (!(lookup_->good()))
     DCHECK(false) << "ScopedDirLookup failed on valid DirManager.";
 }
 BaseTransaction::~BaseTransaction() {
   delete lookup_;
 }
 
 UserShare* SyncManager::GetUserShare() const {
-  DCHECK(data_->initialized()) << "GetUserShare requires initialization!";
   return data_->GetUserShare();
 }
 
 syncable::ModelTypeSet SyncManager::GetEncryptedDataTypes() const {
   sync_api::ReadTransaction trans(GetUserShare());
   return GetEncryptedTypes(&trans);
 }
 
 
 bool SyncManager::HasUnsyncedItems() const {
@@ skipping 27 matching lines @@
 void SyncManager::TriggerOnIncomingNotificationForTest(
     const syncable::ModelTypeBitSet& model_types) {
   syncable::ModelTypePayloadMap model_types_with_payloads =
       syncable::ModelTypePayloadMapFromBitSet(model_types,
           std::string());
 
   data_->OnIncomingNotification(model_types_with_payloads);
 }
 
 }  // namespace sync_api