Fix ignoring pending local changes in USS tests

HasUnsyncedItems() now honors USS workers.

Bug: 836139
Change-Id: Id46bcae5a8ed66474e13da3cafba5ef32560cfbe
Reviewed-on: https://chromium-review.googlesource.com/1025713
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553117}

chrome/browser/sync/test/integration/profile_sync_service_harness.cc

@@ -513,25 +513,20 @@ std::string ProfileSyncServiceHarness::GetClientInfoString(
     service()->QueryDetailedSyncStatus(&status);
     // Capture select info from the sync session snapshot and syncer status.
     os << ", has_unsynced_items: "
-       << (service()->IsSyncActive() ? service()->HasUnsyncedItems() : 0)
+       << (service()->IsSyncActive() ? service()->HasUnsyncedItemsForTest() : 0)
        << ", did_commit: "
        << (snap.model_neutral_state().num_successful_commits == 0 &&
            snap.model_neutral_state().commit_result == syncer::SYNCER_OK)
-       << ", encryption conflicts: "
-       << snap.num_encryption_conflicts()
-       << ", hierarchy conflicts: "
-       << snap.num_hierarchy_conflicts()
-       << ", server conflicts: "
-       << snap.num_server_conflicts()
+       << ", encryption conflicts: " << snap.num_encryption_conflicts()
+       << ", hierarchy conflicts: " << snap.num_hierarchy_conflicts()
+       << ", server conflicts: " << snap.num_server_conflicts()
        << ", num_updates_downloaded : "
        << snap.model_neutral_state().num_updates_downloaded_total
        << ", passphrase_required_reason: "
        << syncer::PassphraseRequiredReasonToString(
-           service()->passphrase_required_reason())
-       << ", notifications_enabled: "
-       << status.notifications_enabled
-       << ", service_is_active: "
-       << service()->IsSyncActive();
+              service()->passphrase_required_reason())
+       << ", notifications_enabled: " << status.notifications_enabled
+       << ", service_is_active: " << service()->IsSyncActive();
   } else {
     os << "Sync service not available";
   }

chrome/browser/sync/test/integration/quiesce_status_change_checker.cc

@@ -137,7 +137,7 @@ void ProgressMarkerWatcher::UpdateHasLatestProgressMarkers() {
   const syncer::SyncCycleSnapshot& snap = service_->GetLastCycleSnapshot();
   probably_has_latest_progress_markers_ =
       snap.model_neutral_state().num_successful_commits == 0 &&
-      !service_->HasUnsyncedItems();
+      !service_->HasUnsyncedItemsForTest();
 }
 
 bool ProgressMarkerWatcher::HasLatestProgressMarkers() {

chrome/browser/sync/test/integration/sessions_helper.cc

@@ -366,6 +366,9 @@ bool WindowsMatchImpl(const T1& win1, const T2& win2) {
     for (size_t t = 0; t < i->second->wrapped_window.tabs.size(); ++t) {
       client0_tab = i->second->wrapped_window.tabs[t].get();
       client1_tab = j->second->wrapped_window.tabs[t].get();
+      if (client0_tab->navigations.size() != client1_tab->navigations.size()) {
+        return false;
+      }
       for (size_t n = 0; n < client0_tab->navigations.size(); ++n) {
         if (!NavigationEquals(client0_tab->navigations[n],
                               client1_tab->navigations[n])) {

chrome/browser/sync/test/integration/sync_auth_test.cc

@@ -60,9 +60,9 @@ TestForAuthError::TestForAuthError(browser_sync::ProfileSyncService* service)
     : SingleClientStatusChangeChecker(service) {}
 
 bool TestForAuthError::IsExitConditionSatisfied() {
-  return !service()->HasUnsyncedItems() ||
-      (service()->GetSyncTokenStatus().last_get_token_error.state() !=
-       GoogleServiceAuthError::NONE);
+  return !service()->HasUnsyncedItemsForTest() ||
+         (service()->GetSyncTokenStatus().last_get_token_error.state() !=
+          GoogleServiceAuthError::NONE);
 }
 
 std::string TestForAuthError::GetDebugMessage() const {

chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc

@@ -186,11 +186,12 @@ IN_PROC_BROWSER_TEST_P(TwoClientSessionsSyncTest, DeleteIdleSession) {
 
   // Client 1 now deletes client 0's tabs. This frees the memory of sessions1.
   DeleteForeignSession(1, sessions1[0]->session_tag);
+  ASSERT_FALSE(GetClient(0)->service()->HasUnsyncedItemsForTest());
+  ASSERT_TRUE(GetClient(1)->service()->HasUnsyncedItemsForTest());
   ASSERT_TRUE(GetClient(1)->AwaitMutualSyncCycleCompletion(GetClient(0)));
-  ASSERT_FALSE(GetSessionData(1, &sessions1));
+  EXPECT_FALSE(GetSessionData(1, &sessions1));
 }
 
-// Fails all release trybots. crbug.com/263369.
 IN_PROC_BROWSER_TEST_P(TwoClientSessionsSyncTest, DeleteActiveSession) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
 
@@ -207,12 +208,15 @@ IN_PROC_BROWSER_TEST_P(TwoClientSessionsSyncTest, DeleteActiveSession) {
 
   // Client 1 now deletes client 0's tabs. This frees the memory of sessions1.
   DeleteForeignSession(1, sessions1[0]->session_tag);
+  ASSERT_FALSE(GetClient(0)->service()->HasUnsyncedItemsForTest());
+  ASSERT_TRUE(GetClient(1)->service()->HasUnsyncedItemsForTest());
   ASSERT_TRUE(GetClient(1)->AwaitMutualSyncCycleCompletion(GetClient(0)));
   ASSERT_FALSE(GetSessionData(1, &sessions1));
 
   // Client 0 becomes active again with a new tab.
   ASSERT_TRUE(OpenTab(0, GURL(kURL2)));
   WaitForForeignSessionsToSync(0, 1);
+  EXPECT_TRUE(GetSessionData(1, &sessions1));
 }
 
 IN_PROC_BROWSER_TEST_P(TwoClientSessionsSyncTest, MultipleWindowsMultipleTabs) {

chrome/browser/sync/test/integration/updated_progress_marker_checker.cc

@@ -19,8 +19,8 @@ bool UpdatedProgressMarkerChecker::IsExitConditionSatisfied() {
   // current client did not commit anything in its previous sync cycle, then
   // this client has the latest progress markers.
   //
-  // The !service()->HasUnsyncedItems() check makes sure that we have nothing to
-  // commit.
+  // The !service()->HasUnsyncedItemsForTest() check makes sure that we have
+  // nothing to commit.
   //
   // There is a subtle race condition here.  While committing items, the syncer
   // will unset the IS_UNSYNCED bits in the directory.  However, the evidence of
@@ -36,7 +36,7 @@ bool UpdatedProgressMarkerChecker::IsExitConditionSatisfied() {
   // completes, then the snapshot is much more likely to be up to date.
   const syncer::SyncCycleSnapshot& snap = service()->GetLastCycleSnapshot();
   return snap.model_neutral_state().num_successful_commits == 0 &&
-         service()->IsSyncActive() && !service()->HasUnsyncedItems();
+         service()->IsSyncActive() && !service()->HasUnsyncedItemsForTest();
 }
 
 std::string UpdatedProgressMarkerChecker::GetDebugMessage() const {

components/browser_sync/profile_sync_service.cc

@@ -1720,10 +1720,10 @@ syncer::SyncCycleSnapshot ProfileSyncService::GetLastCycleSnapshot() const {
   return last_snapshot_;
 }
 
-bool ProfileSyncService::HasUnsyncedItems() const {
+bool ProfileSyncService::HasUnsyncedItemsForTest() const {
   DCHECK(thread_checker_.CalledOnValidThread());
   if (HasSyncingEngine() && engine_initialized_) {
-    return engine_->HasUnsyncedItems();
+    return engine_->HasUnsyncedItemsForTest();
   }
   NOTREACHED();
   return false;

components/browser_sync/profile_sync_service.h

@@ -460,7 +460,7 @@ class ProfileSyncService : public syncer::SyncServiceBase,
   // Returns whether or not the underlying sync engine has made any
   // local changes to items that have not yet been synced with the
   // server.
-  bool HasUnsyncedItems() const;
+  bool HasUnsyncedItemsForTest() const;
 
   // Used by ProfileSyncServiceHarness.  May return null.
   syncer::BackendMigrator* GetBackendMigratorForTest();

components/sync/driver/glue/sync_backend_host_impl.cc

@@ -220,9 +220,9 @@ SyncBackendHostImpl::Status SyncBackendHostImpl::GetDetailedStatus() {
   return core_->sync_manager()->GetDetailedStatus();
 }
 
-bool SyncBackendHostImpl::HasUnsyncedItems() const {
+bool SyncBackendHostImpl::HasUnsyncedItemsForTest() const {
   DCHECK(initialized());
-  return core_->sync_manager()->HasUnsyncedItems();
+  return core_->sync_manager()->HasUnsyncedItemsForTest();
 }
 
 bool SyncBackendHostImpl::IsCryptographerReady(

components/sync/driver/glue/sync_backend_host_impl.h

@@ -81,7 +81,7 @@ class SyncBackendHostImpl : public SyncEngine, public InvalidationHandler {
   void EnableEncryptEverything() override;
   UserShare* GetUserShare() const override;
   Status GetDetailedStatus() override;
-  bool HasUnsyncedItems() const override;
+  bool HasUnsyncedItemsForTest() const override;
   bool IsCryptographerReady(const BaseTransaction* trans) const override;
   void GetModelSafeRoutingInfo(ModelSafeRoutingInfo* out) const override;
   void FlushDirectory() const override;

components/sync/engine/fake_sync_engine.cc

@@ -68,7 +68,7 @@ SyncEngine::Status FakeSyncEngine::GetDetailedStatus() {
   return SyncEngine::Status();
 }
 
-bool FakeSyncEngine::HasUnsyncedItems() const {
+bool FakeSyncEngine::HasUnsyncedItemsForTest() const {
   return false;
 }
 

components/sync/engine/fake_sync_engine.h

@@ -68,7 +68,7 @@ class FakeSyncEngine : public SyncEngine {
 
   Status GetDetailedStatus() override;
 
-  bool HasUnsyncedItems() const override;
+  bool HasUnsyncedItemsForTest() const override;
 
   bool IsCryptographerReady(const BaseTransaction* trans) const override;
 

components/sync/engine/fake_sync_manager.cc

@@ -218,7 +218,7 @@ bool FakeSyncManager::ReceivedExperiment(Experiments* experiments) {
   return false;
 }
 
-bool FakeSyncManager::HasUnsyncedItems() {
+bool FakeSyncManager::HasUnsyncedItemsForTest() {
   NOTIMPLEMENTED();
   return false;
 }

components/sync/engine/fake_sync_manager.h

@@ -104,7 +104,7 @@ class FakeSyncManager : public SyncManager {
   std::unique_ptr<ModelTypeConnector> GetModelTypeConnectorProxy() override;
   const std::string cache_guid() override;
   bool ReceivedExperiment(Experiments* experiments) override;
-  bool HasUnsyncedItems() override;
+  bool HasUnsyncedItemsForTest() override;
   SyncEncryptionHandler* GetEncryptionHandler() override;
   std::vector<std::unique_ptr<ProtocolEvent>> GetBufferedProtocolEvents()
       override;

components/sync/engine/sync_engine.h

@@ -158,7 +158,7 @@ class SyncEngine : public ModelTypeConfigurer {
   // Determines if the underlying sync engine has made any local changes to
   // items that have not yet been synced with the server.
   // ONLY CALL THIS IF OnInitializationComplete was called!
-  virtual bool HasUnsyncedItems() const = 0;
+  virtual bool HasUnsyncedItemsForTest() const = 0;
 
   // True if the cryptographer has any keys available to attempt decryption.
   // Could mean we've downloaded and loaded Nigori objects, or we bootstrapped

components/sync/engine/sync_manager.h

@@ -377,9 +377,8 @@ class SyncManager {
   // Note: opens a transaction.  May be called on any thread.
   virtual bool ReceivedExperiment(Experiments* experiments) = 0;
 
-  // Uses a read-only transaction to determine if the directory being synced has
-  // any remaining unsynced items.  May be called on any thread.
-  virtual bool HasUnsyncedItems() = 0;
+  // Returns whether there are remaining unsynced items.
+  virtual bool HasUnsyncedItemsForTest() = 0;
 
   // Returns the SyncManager's encryption handler.
   virtual SyncEncryptionHandler* GetEncryptionHandler() = 0;

components/sync/engine_impl/model_type_registry.cc

@@ -21,6 +21,8 @@
 #include "components/sync/engine_impl/directory_commit_contributor.h"
 #include "components/sync/engine_impl/directory_update_handler.h"
 #include "components/sync/engine_impl/model_type_worker.h"
+#include "components/sync/syncable/read_transaction.h"
+#include "components/sync/syncable/syncable_base_transaction.h"
 
 namespace syncer {
 
@@ -285,6 +287,19 @@ void ModelTypeRegistry::RequestEmitDebugInfo() {
   }
 }
 
+bool ModelTypeRegistry::HasUnsyncedItemsForTest() const {
+  // For model type workers, we ask them individually.
+  for (const auto& worker : model_type_workers_) {
+    if (worker->HasLocalChangesForTest()) {
+      return true;
+    }
+  }
+
+  // Verify directory state.
+  ReadTransaction trans(FROM_HERE, user_share_);
+  return trans.GetWrappedTrans()->directory()->unsynced_entity_count() != 0;
+}
+
 base::WeakPtr<ModelTypeConnector> ModelTypeRegistry::AsWeakPtr() {
   return weak_ptr_factory_.GetWeakPtr();
 }

components/sync/engine_impl/model_type_registry.h

@@ -105,6 +105,8 @@ class ModelTypeRegistry : public ModelTypeConnector,
       const TypeDebugInfoObserver* observer) const;
   void RequestEmitDebugInfo();
 
+  bool HasUnsyncedItemsForTest() const;
+
   base::WeakPtr<ModelTypeConnector> AsWeakPtr();
 
  private:

components/sync/engine_impl/model_type_worker.cc

@@ -292,6 +292,10 @@ std::unique_ptr<CommitContribution> ModelTypeWorker::GetContribution(
       CommitOnlyTypes().Has(GetModelType()));
 }
 
+bool ModelTypeWorker::HasLocalChangesForTest() const {
+  return has_local_changes_;
+}
+
 void ModelTypeWorker::OnCommitResponse(CommitResponseDataList* response_list) {
   DCHECK(thread_checker_.CalledOnValidThread());
 

components/sync/engine_impl/model_type_worker.h

@@ -104,6 +104,8 @@ class ModelTypeWorker : public UpdateHandler,
   std::unique_ptr<CommitContribution> GetContribution(
       size_t max_entries) override;
 
+  bool HasLocalChangesForTest() const;
+
   // An alternative way to drive sending data to the processor, that should be
   // called when a new encryption mechanism is ready.
   void EncryptionAcceptedMaybeApplyUpdates();

components/sync/engine_impl/sync_manager_impl.cc

@@ -950,9 +950,8 @@ bool SyncManagerImpl::ReceivedExperiment(Experiments* experiments) {
   return found_experiment;
 }
 
-bool SyncManagerImpl::HasUnsyncedItems() {
-  ReadTransaction trans(FROM_HERE, GetUserShare());
-  return (trans.GetWrappedTrans()->directory()->unsynced_entity_count() != 0);
+bool SyncManagerImpl::HasUnsyncedItemsForTest() {
+  return model_type_registry_->HasUnsyncedItemsForTest();
 }
 
 SyncEncryptionHandler* SyncManagerImpl::GetEncryptionHandler() {

components/sync/engine_impl/sync_manager_impl.h

@@ -93,7 +93,7 @@ class SyncManagerImpl
   std::unique_ptr<ModelTypeConnector> GetModelTypeConnectorProxy() override;
   const std::string cache_guid() override;
   bool ReceivedExperiment(Experiments* experiments) override;
-  bool HasUnsyncedItems() override;
+  bool HasUnsyncedItemsForTest() override;
   SyncEncryptionHandler* GetEncryptionHandler() override;
   std::vector<std::unique_ptr<ProtocolEvent>> GetBufferedProtocolEvents()
       override;