[Sync] Add SyncStatusObserver to sync engine.

This patch removes mutex from AllStatus class and introduces
SyncStatusObserver instead of getting of status directly from AllStatus
from different threads. AllStatus becomes single-threaded now.

Status for debugging is stored in SyncEngineImpl.

Bug: 1058773
Change-Id: Ib1d30d5ff6bc5bc5e1f24e94499de949f1e2e053
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2089695
Commit-Queue: Rushan Suleymanov <rushans@google.com>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: vitaliii <vitaliii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#748611}

components/sync/BUILD.gn

@@ -101,6 +101,8 @@ jumbo_static_library("rest_of_sync") {
     "engine/sync_manager_factory.h",
     "engine/sync_status.cc",
     "engine/sync_status.h",
+    "engine/sync_status_observer.cc",
+    "engine/sync_status_observer.h",
     "engine/sync_string_conversions.cc",
     "engine/sync_string_conversions.h",
     "engine/ui_model_worker.cc",

components/sync/driver/glue/sync_engine_backend.cc

@@ -225,6 +225,11 @@ void SyncEngineBackend::OnStatusCountersUpdated(
       counters);
 }
 
+void SyncEngineBackend::OnSyncStatusChanged(const SyncStatus& status) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  host_.Call(FROM_HERE, &SyncEngineImpl::HandleSyncStatusChanged, status);
+}
+
 void SyncEngineBackend::OnActionableError(const SyncProtocolError& sync_error) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   host_.Call(FROM_HERE,
@@ -385,6 +390,7 @@ void SyncEngineBackend::DoInitialize(SyncEngine::InitParams params) {
   args.cache_guid = params.cache_guid;
   args.birthday = params.birthday;
   args.bag_of_chips = params.bag_of_chips;
+  args.sync_status_observers.push_back(this);
   sync_manager_->Init(&args);
   base::trace_event::MemoryDumpManager::GetInstance()
       ->RegisterDumpProviderWithSequencedTaskRunner(

components/sync/driver/glue/sync_engine_backend.h

@@ -27,6 +27,7 @@
 #include "components/sync/engine/model_type_configurer.h"
 #include "components/sync/engine/shutdown_reason.h"
 #include "components/sync/engine/sync_encryption_handler.h"
+#include "components/sync/engine/sync_status_observer.h"
 #include "components/sync/syncable/user_share.h"
 #include "url/gurl.h"
 
@@ -44,7 +45,8 @@ class NigoriHandlerProxy;
 class SyncEngineBackend : public base::RefCountedThreadSafe<SyncEngineBackend>,
                           public base::trace_event::MemoryDumpProvider,
                           public SyncManager::Observer,
-                          public TypeDebugInfoObserver {
+                          public TypeDebugInfoObserver,
+                          public SyncStatusObserver {
  public:
   using AllNodesCallback =
       base::OnceCallback<void(const ModelType,
@@ -79,6 +81,9 @@ class SyncEngineBackend : public base::RefCountedThreadSafe<SyncEngineBackend>,
   void OnStatusCountersUpdated(ModelType type,
                                const StatusCounters& counters) override;
 
+  // SyncStatusObserver implementation.
+  void OnSyncStatusChanged(const SyncStatus& status) override;
+
   // Forwards an invalidation state change to the sync manager.
   void DoOnInvalidatorStateChange(InvalidatorState state);
 

components/sync/driver/glue/sync_engine_impl.cc

@@ -230,8 +230,9 @@ UserShare* SyncEngineImpl::GetUserShare() const {
 }
 
 SyncEngineImpl::Status SyncEngineImpl::GetDetailedStatus() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(IsInitialized());
-  return backend_->sync_manager()->GetDetailedStatus();
+  return cached_status_;
 }
 
 void SyncEngineImpl::HasUnsyncedItemsForTest(
@@ -435,6 +436,11 @@ void SyncEngineImpl::UpdateInvalidationVersions(
   sync_prefs_->UpdateInvalidationVersions(invalidation_versions);
 }
 
+void SyncEngineImpl::HandleSyncStatusChanged(const SyncStatus& status) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  cached_status_ = status;
+}
+
 void SyncEngineImpl::OnCookieJarChanged(bool account_mismatch,
                                         bool empty_jar,
                                         base::OnceClosure callback) {

components/sync/driver/glue/sync_engine_impl.h

@@ -160,6 +160,9 @@ class SyncEngineImpl : public SyncEngine, public InvalidationHandler {
   void UpdateInvalidationVersions(
       const std::map<ModelType, int64_t>& invalidation_versions);
 
+  // Stores the new |status| in local cache.
+  void HandleSyncStatusChanged(const SyncStatus& status);
+
  private:
   friend class SyncEngineBackend;
 
@@ -216,6 +219,8 @@ class SyncEngineImpl : public SyncEngine, public InvalidationHandler {
   ModelTypeSet last_enabled_types_;
   bool sessions_invalidation_enabled_ = false;
 
+  SyncStatus cached_status_;
+
   // Checks that we're on the same thread this was constructed on (UI thread).
   SEQUENCE_CHECKER(sequence_checker_);
 

components/sync/engine/fake_sync_manager.cc

@@ -183,11 +183,6 @@ void FakeSyncManager::RemoveObserver(Observer* observer) {
   observers_.RemoveObserver(observer);
 }
 
-SyncStatus FakeSyncManager::GetDetailedStatus() const {
-  NOTIMPLEMENTED();
-  return SyncStatus();
-}
-
 void FakeSyncManager::SaveChanges() {
   // Do nothing.
 }

components/sync/engine/fake_sync_manager.h

@@ -97,7 +97,6 @@ class FakeSyncManager : public SyncManager {
   void SetInvalidatorEnabled(bool invalidator_enabled) override;
   void AddObserver(Observer* observer) override;
   void RemoveObserver(Observer* observer) override;
-  SyncStatus GetDetailedStatus() const override;
   void SaveChanges() override;
   void ShutdownOnSyncThread() override;
   UserShare* GetUserShare() override;

components/sync/engine/sync_manager.h

@@ -51,6 +51,7 @@ class JsBackend;
 class JsEventHandler;
 class ProtocolEvent;
 class SyncCycleSnapshot;
+class SyncStatusObserver;
 class TypeDebugInfoObserver;
 class UnrecoverableErrorHandler;
 struct UserShare;
@@ -255,6 +256,9 @@ class SyncManager {
     std::string cache_guid;
     std::string birthday;
     std::string bag_of_chips;
+
+    // List of observers to be added to AllStatus.
+    std::vector<SyncStatusObserver*> sync_status_observers;
   };
 
   // The state of sync the feature. If the user turned on sync explicitly, it
@@ -332,9 +336,6 @@ class SyncManager {
   // potentially dereference garbage.
   virtual void RemoveObserver(Observer* observer) = 0;
 
-  // Status-related getter.  May be called on any thread.
-  virtual SyncStatus GetDetailedStatus() const = 0;
-
   // Call periodically from a database-safe thread to persist recent changes
   // to the syncapi model.
   virtual void SaveChanges() = 0;

components/sync/engine/sync_status_observer.cc

+// Copyright 2020 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 "components/sync/engine/sync_status_observer.h"
+
+namespace syncer {
+
+SyncStatusObserver::SyncStatusObserver() = default;
+
+SyncStatusObserver::~SyncStatusObserver() = default;
+
+}  // namespace syncer

components/sync/engine/sync_status_observer.h

+// Copyright 2020 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.
+
+#ifndef COMPONENTS_SYNC_ENGINE_SYNC_STATUS_OBSERVER_H_
+#define COMPONENTS_SYNC_ENGINE_SYNC_STATUS_OBSERVER_H_
+
+#include "components/sync/engine/sync_status.h"
+
+namespace syncer {
+
+class SyncStatusObserver {
+ public:
+  SyncStatusObserver();
+  virtual ~SyncStatusObserver();
+
+  // This event is sent when SyncStatus changes.
+  virtual void OnSyncStatusChanged(const SyncStatus& status) = 0;
+};
+
+}  // namespace syncer
+
+#endif  // COMPONENTS_SYNC_ENGINE_SYNC_STATUS_OBSERVER_H_

components/sync/engine_impl/all_status.cc

@@ -5,8 +5,10 @@
 #include "components/sync/engine_impl/all_status.h"
 
 #include <algorithm>
+#include <utility>
 
 #include "base/logging.h"
+#include "components/sync/engine/sync_status_observer.h"
 #include "components/sync/engine_impl/net/server_connection_manager.h"
 #include "components/sync/engine_impl/sync_cycle_event.h"
 
@@ -18,7 +20,9 @@ AllStatus::AllStatus() {
   status_.crypto_has_pending_keys = false;
 }
 
-AllStatus::~AllStatus() {}
+AllStatus::~AllStatus() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+}
 
 SyncStatus AllStatus::CreateBlankStatus() const {
   // Status is initialized with the previous status value.  Variables
@@ -69,8 +73,15 @@ SyncStatus AllStatus::CalcSyncing(const SyncCycleEvent& event) const {
   return status;
 }
 
+void AllStatus::AddObserver(SyncStatusObserver* observer) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DCHECK(observer);
+  sync_status_observers_.push_back(observer);
+  observer->OnSyncStatusChanged(status_);
+}
+
 void AllStatus::OnSyncCycleEvent(const SyncCycleEvent& event) {
-  ScopedStatusLock lock(this);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   switch (event.what_happened) {
     case SyncCycleEvent::SYNC_CYCLE_BEGIN:
     case SyncCycleEvent::STATUS_CHANGED:
@@ -81,102 +92,114 @@ void AllStatus::OnSyncCycleEvent(const SyncCycleEvent& event) {
       LOG(ERROR) << "Unrecognized Syncer Event: " << event.what_happened;
       break;
   }
+  NotifyStatusChanged();
 }
 
 void AllStatus::OnActionableError(
     const SyncProtocolError& sync_protocol_error) {
-  ScopedStatusLock lock(this);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   status_ = CreateBlankStatus();
   status_.sync_protocol_error = sync_protocol_error;
+  NotifyStatusChanged();
 }
 
 void AllStatus::OnRetryTimeChanged(base::Time retry_time) {
-  ScopedStatusLock lock(this);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   status_.retry_time = retry_time;
+  NotifyStatusChanged();
 }
 
 void AllStatus::OnThrottledTypesChanged(ModelTypeSet throttled_types) {
-  ScopedStatusLock lock(this);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   status_.throttled_types = throttled_types;
+  NotifyStatusChanged();
 }
 
 void AllStatus::OnBackedOffTypesChanged(ModelTypeSet backed_off_types) {
-  ScopedStatusLock lock(this);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   status_.backed_off_types = backed_off_types;
+  NotifyStatusChanged();
 }
 
-void AllStatus::OnMigrationRequested(ModelTypeSet) {}
-
-void AllStatus::OnProtocolEvent(const ProtocolEvent&) {}
+void AllStatus::OnMigrationRequested(ModelTypeSet) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+}
 
-SyncStatus AllStatus::status() const {
-  base::AutoLock lock(mutex_);
-  return status_;
+void AllStatus::OnProtocolEvent(const ProtocolEvent&) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 }
 
 void AllStatus::SetNotificationsEnabled(bool notifications_enabled) {
-  ScopedStatusLock lock(this);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   status_.notifications_enabled = notifications_enabled;
+  NotifyStatusChanged();
 }
 
 void AllStatus::IncrementNotificationsReceived() {
-  ScopedStatusLock lock(this);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   ++status_.notifications_received;
+  NotifyStatusChanged();
 }
 
 void AllStatus::SetEncryptedTypes(ModelTypeSet types) {
-  ScopedStatusLock lock(this);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   status_.encrypted_types = types;
+  NotifyStatusChanged();
 }
 
 void AllStatus::SetCryptographerCanEncrypt(bool can_encrypt) {
-  ScopedStatusLock lock(this);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   status_.cryptographer_can_encrypt = can_encrypt;
+  NotifyStatusChanged();
 }
 
 void AllStatus::SetCryptoHasPendingKeys(bool has_pending_keys) {
-  ScopedStatusLock lock(this);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   status_.crypto_has_pending_keys = has_pending_keys;
+  NotifyStatusChanged();
 }
 
 void AllStatus::SetPassphraseType(PassphraseType type) {
-  ScopedStatusLock lock(this);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   status_.passphrase_type = type;
+  NotifyStatusChanged();
 }
 
 void AllStatus::SetHasKeystoreKey(bool has_keystore_key) {
-  ScopedStatusLock lock(this);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   status_.has_keystore_key = has_keystore_key;
+  NotifyStatusChanged();
 }
 
 void AllStatus::SetKeystoreMigrationTime(const base::Time& migration_time) {
-  ScopedStatusLock lock(this);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   status_.keystore_migration_time = migration_time;
+  NotifyStatusChanged();
 }
 
 void AllStatus::SetSyncId(const std::string& sync_id) {
-  ScopedStatusLock lock(this);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   status_.sync_id = sync_id;
+  NotifyStatusChanged();
 }
 
 void AllStatus::SetInvalidatorClientId(
     const std::string& invalidator_client_id) {
-  ScopedStatusLock lock(this);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   status_.invalidator_client_id = invalidator_client_id;
+  NotifyStatusChanged();
 }
 
 void AllStatus::SetLocalBackendFolder(const std::string& folder) {
-  ScopedStatusLock lock(this);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   status_.local_sync_folder = folder;
+  NotifyStatusChanged();
 }
 
-ScopedStatusLock::ScopedStatusLock(AllStatus* allstatus)
-    : allstatus_(allstatus) {
-  allstatus->mutex_.Acquire();
-}
-
-ScopedStatusLock::~ScopedStatusLock() {
-  allstatus_->mutex_.Release();
+void AllStatus::NotifyStatusChanged() {
+  for (SyncStatusObserver* observer : sync_status_observers_) {
+    observer->OnSyncStatusChanged(status_);
+  }
 }
 
 }  // namespace syncer

components/sync/engine_impl/all_status.h

@@ -7,10 +7,11 @@
 
 #include <map>
 #include <string>
+#include <vector>
 
 #include "base/compiler_specific.h"
 #include "base/macros.h"
-#include "base/synchronization/lock.h"
+#include "base/sequence_checker.h"
 #include "components/sync/base/model_type.h"
 #include "components/sync/engine/sync_status.h"
 #include "components/sync/engine_impl/nudge_source.h"
@@ -19,12 +20,12 @@
 
 namespace syncer {
 
-class ScopedStatusLock;
+class SyncStatusObserver;
 struct SyncCycleEvent;
 
 // This class watches various sync engine components, updating its internal
-// state upon change. It can return a snapshot of this state as a SyncerStatus
-// object, aggregating all this data into one place.
+// state upon change. It can send to observers a snapshot of this state as a
+// SyncStatus object, aggregating all this data into one place.
 //
 // Most of this data ends up on the about:sync page.  But the page is only
 // 'pinged' to update itself at the end of a sync cycle.  A user could refresh
@@ -37,6 +38,11 @@ class AllStatus : public SyncEngineEventListener {
   AllStatus();
   ~AllStatus() override;
 
+  // Adds an observer which will receive a call whenever this object changes.
+  // The |observer| gets notification after being added right away. |observer|
+  // must not be null and must outlive this object.
+  void AddObserver(SyncStatusObserver* observer);
+
   // SyncEngineEventListener implementation.
   void OnSyncCycleEvent(const SyncCycleEvent& event) override;
   void OnActionableError(const SyncProtocolError& error) override;
@@ -46,8 +52,6 @@ class AllStatus : public SyncEngineEventListener {
   void OnMigrationRequested(ModelTypeSet types) override;
   void OnProtocolEvent(const ProtocolEvent& event) override;
 
-  SyncStatus status() const;
-
   void SetNotificationsEnabled(bool notifications_enabled);
 
   void IncrementNotificationsReceived();
@@ -72,21 +76,17 @@ class AllStatus : public SyncEngineEventListener {
 
   SyncStatus status_;
 
-  mutable base::Lock mutex_;  // Protects all data members.
-
  private:
   friend class ScopedStatusLock;
 
-  DISALLOW_COPY_AND_ASSIGN(AllStatus);
-};
+  // Notifies all observers about changing of status.
+  void NotifyStatusChanged();
 
-class ScopedStatusLock {
- public:
-  explicit ScopedStatusLock(AllStatus* allstatus);
-  ~ScopedStatusLock();
+  std::vector<SyncStatusObserver*> sync_status_observers_;
 
- protected:
-  AllStatus* allstatus_;
+  SEQUENCE_CHECKER(sequence_checker_);
+
+  DISALLOW_COPY_AND_ASSIGN(AllStatus);
 };
 
 }  // namespace syncer

components/sync/engine_impl/sync_manager_impl.cc

@@ -340,6 +340,12 @@ void SyncManagerImpl::Init(InitArgs* args) {
   DVLOG(1) << "Setting invalidator client ID: " << args->invalidator_client_id;
   allstatus_.SetInvalidatorClientId(args->invalidator_client_id);
 
+  // Add observers after all initializations. Each observer will get initialized
+  // status while being added.
+  for (SyncStatusObserver* observer : args->sync_status_observers) {
+    allstatus_.AddObserver(observer);
+  }
+
   model_type_registry_ = std::make_unique<ModelTypeRegistry>(
       args->workers, share_, this, base::BindRepeating(&MigrateDirectoryData),
       args->cancelation_signal,
@@ -918,10 +924,6 @@ void SyncManagerImpl::RefreshTypes(ModelTypeSet types) {
   }
 }
 
-SyncStatus SyncManagerImpl::GetDetailedStatus() const {
-  return allstatus_.status();
-}
-
 void SyncManagerImpl::SaveChanges() {
   directory()->SaveChanges();
 }

components/sync/engine_impl/sync_manager_impl.h

@@ -87,7 +87,6 @@ class SyncManagerImpl
       std::unique_ptr<InvalidationInterface> invalidation) override;
   void AddObserver(SyncManager::Observer* observer) override;
   void RemoveObserver(SyncManager::Observer* observer) override;
-  SyncStatus GetDetailedStatus() const override;
   void SaveChanges() override;
   void ShutdownOnSyncThread() override;
   UserShare* GetUserShare() override;