[Sync] Support for non-blocking conflicts and encryption conflicts.

This patch introduces the notion of non-blocking conflicts. These are conflicts that do not result in the
syncer getting stuck. They affect the HasConflictingUpdates status call, so we attempt to reapply updates
when they occur, but they are not passed to the conflict resolver, and hence do not result in the syncer being stuck. All cases where we have a conflict due to encryption/decryption result in a new 
ENCRYPTION_CONFLICT, which are treated as nonblocking conflicts.

Updated sync's protocol_version to 27 so the server can tell which clients understand encryption.

Also, due to this change, it's more likely that local/server changes will be overwritten in the conflict
resolver (since encryption conflicts are kept as unapplied while they can't be decrypted). Added
counters for how often this happens, which will appear on about:sync, and filed follow up bug
crbug.com/76596.

BUG=59242
TEST=new session sync_integration tests, modified apply_updates_command unit tests


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

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

23 files changed, 313 insertions, 66 deletions

diff --git a/chrome/browser/sync/backend_migrator_unittest.cc b/chrome/browser/sync/backend_migrator_unittest.cc
index 39b2ef0..de7063e 100644
--- a/chrome/browser/sync/backend_migrator_unittest.cc
+++ b/chrome/browser/sync/backend_migrator_unittest.cc
@@ -53,7 +53,7 @@ class BackendMigratorTest : public testing::Test {
 
     snap_.reset(new SyncSessionSnapshot(SyncerStatus(), ErrorCounters(),
         0, false, syncable::ModelTypeBitSet(), download_progress_markers,
-        false, false, 0, 0, false, sessions::SyncSourceInfo()));
+        false, false, 0, 0, 0, false, sessions::SyncSourceInfo()));
     EXPECT_CALL(service_, GetLastSessionSnapshot())
         .WillOnce(Return(snap_.get()));
   }
diff --git a/chrome/browser/sync/engine/all_status.cc b/chrome/browser/sync/engine/all_status.cc
index 35bc033..83d148a 100644
--- a/chrome/browser/sync/engine/all_status.cc
+++ b/chrome/browser/sync/engine/all_status.cc
@@ -75,6 +75,10 @@ sync_api::SyncManager::Status AllStatus::CalcSyncing(
         snapshot->syncer_status.num_updates_downloaded_total;
     status.tombstone_updates_received +=
         snapshot->syncer_status.num_tombstone_updates_downloaded_total;
+    status.num_local_overwrites_total +=
+        snapshot->syncer_status.num_local_overwrites;
+    status.num_server_overwrites_total +=
+        snapshot->syncer_status.num_server_overwrites;
   }
   return status;
 }
diff --git a/chrome/browser/sync/engine/apply_updates_command_unittest.cc b/chrome/browser/sync/engine/apply_updates_command_unittest.cc
index f9c06bd..db2321a 100644
--- a/chrome/browser/sync/engine/apply_updates_command_unittest.cc
+++ b/chrome/browser/sync/engine/apply_updates_command_unittest.cc
@@ -258,13 +258,22 @@ TEST_F(ApplyUpdatesCommandTest, UndecryptablePassword) {
   apply_updates_command_.ExecuteImpl(session());
 
   sessions::StatusController* status = session()->status_controller();
-  sessions::ScopedModelSafeGroupRestriction r(status, GROUP_PASSIVE);
-  EXPECT_EQ(1, status->update_progress().AppliedUpdatesSize())
-      << "All updates should have been attempted";
-  EXPECT_EQ(1, status->conflict_progress().ConflictingItemsSize())
-      << "The updates that can't be decrypted should be in conflict";
-  EXPECT_EQ(0, status->update_progress().SuccessfullyAppliedUpdateCount())
-      << "No update that can't be decrypted should be applied";
+  EXPECT_TRUE(status->HasConflictingUpdates())
+    << "Updates that can't be decrypted should trigger the syncer to have "
+    << "conflicting updates.";
+  {
+    sessions::ScopedModelSafeGroupRestriction r(status, GROUP_PASSIVE);
+    EXPECT_EQ(1, status->update_progress().AppliedUpdatesSize())
+        << "All updates should have been attempted";
+    EXPECT_EQ(0, status->conflict_progress().ConflictingItemsSize())
+        << "The updates that can't be decrypted should not be in regular "
+        << "conflict";
+    EXPECT_EQ(1, status->conflict_progress().NonblockingConflictingItemsSize())
+        << "The updates that can't be decrypted should be in nonblocking "
+        << "conflict";
+    EXPECT_EQ(0, status->update_progress().SuccessfullyAppliedUpdateCount())
+        << "No update that can't be decrypted should be applied";
+  }
 }
 
 TEST_F(ApplyUpdatesCommandTest, SomeUndecryptablePassword) {
@@ -306,13 +315,22 @@ TEST_F(ApplyUpdatesCommandTest, SomeUndecryptablePassword) {
   apply_updates_command_.ExecuteImpl(session());
 
   sessions::StatusController* status = session()->status_controller();
-  sessions::ScopedModelSafeGroupRestriction r(status, GROUP_PASSIVE);
-  EXPECT_EQ(2, status->update_progress().AppliedUpdatesSize())
-      << "All updates should have been attempted";
-  EXPECT_EQ(1, status->conflict_progress().ConflictingItemsSize())
-      << "The decryptable password update should be applied";
-  EXPECT_EQ(1, status->update_progress().SuccessfullyAppliedUpdateCount())
-      << "The undecryptable password update shouldn't be applied";
+  EXPECT_TRUE(status->HasConflictingUpdates())
+    << "Updates that can't be decrypted should trigger the syncer to have "
+    << "conflicting updates.";
+  {
+    sessions::ScopedModelSafeGroupRestriction r(status, GROUP_PASSIVE);
+    EXPECT_EQ(2, status->update_progress().AppliedUpdatesSize())
+        << "All updates should have been attempted";
+    EXPECT_EQ(0, status->conflict_progress().ConflictingItemsSize())
+        << "The updates that can't be decrypted should not be in regular "
+        << "conflict";
+    EXPECT_EQ(1, status->conflict_progress().NonblockingConflictingItemsSize())
+        << "The updates that can't be decrypted should be in nonblocking "
+        << "conflict";
+    EXPECT_EQ(1, status->update_progress().SuccessfullyAppliedUpdateCount())
+        << "The undecryptable password update shouldn't be applied";
+  }
 }
 
 TEST_F(ApplyUpdatesCommandTest, NigoriUpdate) {
@@ -432,7 +450,9 @@ TEST_F(ApplyUpdatesCommandTest, EncryptUnsyncedChanges) {
   EXPECT_EQ(1, status->update_progress().AppliedUpdatesSize())
       << "All updates should have been attempted";
   EXPECT_EQ(0, status->conflict_progress().ConflictingItemsSize())
-      << "The nigori update shouldn't be in conflict";
+      << "No updates should be in conflict";
+  EXPECT_EQ(0, status->conflict_progress().NonblockingConflictingItemsSize())
+      << "No updates should be in conflict";
   EXPECT_EQ(1, status->update_progress().SuccessfullyAppliedUpdateCount())
       << "The nigori update should be applied";
   EXPECT_FALSE(cryptographer->has_pending_keys());
@@ -526,8 +546,12 @@ TEST_F(ApplyUpdatesCommandTest, CannotEncryptUnsyncedChanges) {
   sessions::ScopedModelSafeGroupRestriction r(status, GROUP_PASSIVE);
   EXPECT_EQ(1, status->update_progress().AppliedUpdatesSize())
       << "All updates should have been attempted";
-  EXPECT_EQ(1, status->conflict_progress().ConflictingItemsSize())
-      << "The unsynced chnages trigger a conflict with the nigori update.";
+  EXPECT_EQ(0, status->conflict_progress().ConflictingItemsSize())
+      << "The unsynced changes don't trigger a blocking conflict with the "
+      << "nigori update.";
+  EXPECT_EQ(1, status->conflict_progress().NonblockingConflictingItemsSize())
+      << "The unsynced changes trigger a non-blocking conflict with the "
+      << "nigori update.";
   EXPECT_EQ(0, status->update_progress().SuccessfullyAppliedUpdateCount())
       << "The nigori update should not be applied";
   EXPECT_FALSE(cryptographer->is_ready());
diff --git a/chrome/browser/sync/engine/conflict_resolver.cc b/chrome/browser/sync/engine/conflict_resolver.cc
index 3b06078..b15222c 100644
--- a/chrome/browser/sync/engine/conflict_resolver.cc
+++ b/chrome/browser/sync/engine/conflict_resolver.cc
@@ -1,9 +1,10 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// 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/conflict_resolver.h"
 
+#include <algorithm>
 #include <map>
 #include <set>
 
@@ -62,7 +63,8 @@ void ConflictResolver::OverwriteServerChanges(WriteTransaction* trans,
 
 ConflictResolver::ProcessSimpleConflictResult
 ConflictResolver::ProcessSimpleConflict(WriteTransaction* trans,
-                                        const Id& id) {
+                                        const Id& id,
+                                        StatusController* status) {
   MutableEntry entry(trans, syncable::GET_BY_ID, id);
   // Must be good as the entry won't have been cleaned up.
   CHECK(entry.good());
@@ -101,6 +103,7 @@ ConflictResolver::ProcessSimpleConflict(WriteTransaction* trans,
     // be nice if we could route this back to ModelAssociator code to pick one
     // of three options: CLIENT, SERVER, or MERGE.  Some datatypes (autofill)
     // are easily mergeable.
+    // See http://crbug.com/77339.
     bool name_matches = entry.Get(syncable::NON_UNIQUE_NAME) ==
                         entry.Get(syncable::SERVER_NON_UNIQUE_NAME);
     bool parent_matches = entry.Get(syncable::PARENT_ID) ==
@@ -108,13 +111,18 @@ ConflictResolver::ProcessSimpleConflict(WriteTransaction* trans,
     bool entry_deleted = entry.Get(syncable::IS_DEL);
 
     if (!entry_deleted && name_matches && parent_matches) {
+      // TODO(zea): We may prefer to choose the local changes over the server
+      // if we know the local changes happened before (or vice versa).
+      // See http://crbug.com/76596
       VLOG(1) << "Resolving simple conflict, ignoring local changes for:"
               << entry;
       IgnoreLocalChanges(&entry);
+      status->increment_num_local_overwrites();
     } else {
       VLOG(1) << "Resolving simple conflict, overwriting server changes for:"
               << entry;
       OverwriteServerChanges(trans, &entry);
+      status->increment_num_server_overwrites();
     }
     return SYNC_PROGRESS;
   } else {  // SERVER_IS_DEL is true
@@ -139,6 +147,7 @@ ConflictResolver::ProcessSimpleConflict(WriteTransaction* trans,
           "know to re-create, client-tagged items should revert to version 0 "
           "when server-deleted.";
       OverwriteServerChanges(trans, &entry);
+      status->increment_num_server_overwrites();
       // Clobber the versions, just in case the above DCHECK is violated.
       entry.Put(syncable::SERVER_VERSION, 0);
       entry.Put(syncable::BASE_VERSION, 0);
@@ -338,7 +347,7 @@ bool AttemptToFixUpdateEntryInDeletedLocalTree(WriteTransaction* trans,
 
 bool AttemptToFixRemovedDirectoriesWithContent(WriteTransaction* trans,
                                                ConflictSet* conflict_set) {
-  ConflictSet::const_iterator i,j;
+  ConflictSet::const_iterator i, j;
   for (i = conflict_set->begin() ; i != conflict_set->end() ; ++i) {
     Entry entry(trans, syncable::GET_BY_ID, *i);
     if (AttemptToFixUnsyncedEntryInDeletedServerTree(trans,
@@ -433,7 +442,7 @@ bool ConflictResolver::ResolveSimpleConflicts(const ScopedDirLookup& dir,
     if (item_set_it == progress.IdToConflictSetEnd() ||
         0 == item_set_it->second) {
       // We have a simple conflict.
-      switch (ProcessSimpleConflict(&trans, id)) {
+      switch (ProcessSimpleConflict(&trans, id, status)) {
         case NO_SYNC_PROGRESS:
           {
             int conflict_count = (simple_conflict_count_map_[id] += 2);
diff --git a/chrome/browser/sync/engine/conflict_resolver.h b/chrome/browser/sync/engine/conflict_resolver.h
index 5880ae4..97bbac2 100644
--- a/chrome/browser/sync/engine/conflict_resolver.h
+++ b/chrome/browser/sync/engine/conflict_resolver.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
+// 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.
 //
@@ -9,6 +9,7 @@
 #define CHROME_BROWSER_SYNC_ENGINE_CONFLICT_RESOLVER_H_
 #pragma once
 
+#include <map>
 #include <set>
 #include <string>
 
@@ -66,7 +67,8 @@ class ConflictResolver {
 
   ProcessSimpleConflictResult ProcessSimpleConflict(
       syncable::WriteTransaction* trans,
-      const syncable::Id& id);
+      const syncable::Id& id,
+      sessions::StatusController* status);
 
   bool ResolveSimpleConflicts(const syncable::ScopedDirLookup& dir,
                               sessions::StatusController* status);
diff --git a/chrome/browser/sync/engine/syncapi.h b/chrome/browser/sync/engine/syncapi.h
index 9128889..c2bfa0e 100644
--- a/chrome/browser/sync/engine/syncapi.h
+++ b/chrome/browser/sync/engine/syncapi.h
@@ -731,6 +731,10 @@ class SyncManager {
     // Of updates_received, how many were tombstones.
     int tombstone_updates_received;
     bool disk_full;
+
+    // Total number of overwrites due to conflict resolver since browser start.
+    int num_local_overwrites_total;
+    int num_server_overwrites_total;
   };
 
   // An interface the embedding application implements to receive notifications
diff --git a/chrome/browser/sync/engine/syncer.cc b/chrome/browser/sync/engine/syncer.cc
index e3f5cec..e23c40b 100644
--- a/chrome/browser/sync/engine/syncer.cc
+++ b/chrome/browser/sync/engine/syncer.cc
@@ -225,6 +225,9 @@ void Syncer::SyncShare(sessions::SyncSession* session,
         status->reset_conflicts_resolved();
         ResolveConflictsCommand resolve_conflicts_command;
         resolve_conflicts_command.Execute(session);
+
+        // Has ConflictingUpdates includes both blocking and non-blocking
+        // conflicts. If we have either, we want to attempt to reapply.
         if (status->HasConflictingUpdates())
           next_step = APPLY_UPDATES_TO_RESOLVE_CONFLICTS;
         else
@@ -235,11 +238,17 @@ void Syncer::SyncShare(sessions::SyncSession* session,
         StatusController* status = session->status_controller();
         VLOG(1) << "Applying updates to resolve conflicts";
         ApplyUpdatesCommand apply_updates;
-        int before_conflicting_updates = status->TotalNumConflictingItems();
+
+        // We only care to resolve conflicts again if we made progress on the
+        // blocking conflicts. Whether or not we made progress on the
+        // non-blocking doesn't matter.
+        int before_blocking_conflicting_updates =
+            status->TotalNumBlockingConflictingItems();
         apply_updates.Execute(session);
-        int after_conflicting_updates = status->TotalNumConflictingItems();
-        status->update_conflicts_resolved(before_conflicting_updates >
-                                          after_conflicting_updates);
+        int after_blocking_conflicting_updates =
+            status->TotalNumBlockingConflictingItems();
+        status->update_conflicts_resolved(before_blocking_conflicting_updates >
+                                          after_blocking_conflicting_updates);
         if (status->conflicts_resolved())
           next_step = RESOLVE_CONFLICTS;
         else
diff --git a/chrome/browser/sync/engine/syncer_types.h b/chrome/browser/sync/engine/syncer_types.h
index 57e8855..99e4dc3 100644
--- a/chrome/browser/sync/engine/syncer_types.h
+++ b/chrome/browser/sync/engine/syncer_types.h
@@ -7,6 +7,7 @@
 #pragma once
 
 #include <map>
+#include <string>
 #include <vector>
 
 #include "base/observer_list.h"
@@ -33,6 +34,12 @@ enum UpdateAttemptResponse {
   // Conflicts with the local data representation. This can also mean that the
   // entry doesn't currently make sense if we applied it.
   CONFLICT,
+
+  // We were unable to decrypt/encrypt this server data. As such, we can't make
+  // forward progress on this node, but because the passphrase may not arrive
+  // until later we don't want to get the syncer stuck. See UpdateApplicator
+  // for how this is handled.
+  CONFLICT_ENCRYPTION
 };
 
 enum ServerUpdateProcessingResult {
diff --git a/chrome/browser/sync/engine/syncer_util.cc b/chrome/browser/sync/engine/syncer_util.cc
index c20e206..9e2dbef 100644
--- a/chrome/browser/sync/engine/syncer_util.cc
+++ b/chrome/browser/sync/engine/syncer_util.cc
@@ -316,9 +316,11 @@ UpdateAttemptResponse SyncerUtil::AttemptToUpdateEntry(
       // the new passphrase is entered we should be able to encrypt properly.
       // And, because this update will not be applied yet, next time around
       // we will properly encrypt all appropriate unsynced data.
+      // Note: we return CONFLICT_ENCRYPTION instead of CONFLICT. See
+      // explanation below.
       VLOG(1) << "Marking nigori node update as conflicting due to being unable"
               << " to encrypt all necessary unsynced changes.";
-      return CONFLICT;
+      return CONFLICT_ENCRYPTION;
     }
 
     // Note that we don't bother to encrypt any synced data that now requires
@@ -329,20 +331,28 @@ UpdateAttemptResponse SyncerUtil::AttemptToUpdateEntry(
     // data should be updated as necessary.
   }
 
-  // Only apply updates that we can decrypt. Updates that can't be decrypted yet
-  // will stay in conflict until the user provides a passphrase that lets the
-  // Cryptographer decrypt them.
+  // Only apply updates that we can decrypt. If we can't decrypt the update, it
+  // is likely because the passphrase has not arrived yet. Because the
+  // passphrase may not arrive within this GetUpdates, we can't just return
+  // conflict, else the syncer gets stuck. As such, we return
+  // CONFLICT_ENCRYPTION, which is treated as a non-blocking conflict. See the
+  // description in syncer_types.h.
   if (!entry->Get(SERVER_IS_DIR)) {
     if (specifics.has_encrypted() &&
         !cryptographer->CanDecrypt(specifics.encrypted())) {
       // We can't decrypt this node yet.
-      return CONFLICT;
+      VLOG(1) << "Received an undecryptable "
+              << syncable::ModelTypeToString(entry->GetServerModelType())
+              << " update, returning encryption_conflict.";
+      return CONFLICT_ENCRYPTION;
     } else if (specifics.HasExtension(sync_pb::password)) {
       // Passwords use their own legacy encryption scheme.
       const sync_pb::PasswordSpecifics& password =
           specifics.GetExtension(sync_pb::password);
       if (!cryptographer->CanDecrypt(password.encrypted())) {
-        return CONFLICT;
+        VLOG(1) << "Received an undecryptable password update, returning "
+                << "encryption_conflict.";
+        return CONFLICT_ENCRYPTION;
       }
     }
   }
diff --git a/chrome/browser/sync/engine/update_applicator.cc b/chrome/browser/sync/engine/update_applicator.cc
index 04eca43..78614d7 100644
--- a/chrome/browser/sync/engine/update_applicator.cc
+++ b/chrome/browser/sync/engine/update_applicator.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -29,11 +29,11 @@ UpdateApplicator::UpdateApplicator(ConflictResolver* resolver,
       pointer_(begin),
       group_filter_(group_filter),
       progress_(false),
-      routing_info_(routes) {
-    size_t item_count = end - begin;
-    VLOG(1) << "UpdateApplicator created for " << item_count << " items.";
-    successful_ids_.reserve(item_count);
-  }
+      routing_info_(routes),
+      application_results_(end - begin) {
+  size_t item_count = end - begin;
+  VLOG(1) << "UpdateApplicator created for " << item_count << " items.";
+}
 
 UpdateApplicator::~UpdateApplicator() {
 }
@@ -53,7 +53,7 @@ bool UpdateApplicator::AttemptOneApplication(
     progress_ = false;
 
     // Clear the tracked failures to avoid double-counting.
-    conflicting_ids_.clear();
+    application_results_.ClearConflicts();
   }
 
   syncable::Entry read_only(trans, syncable::GET_BY_HANDLE, *pointer_);
@@ -69,11 +69,15 @@ bool UpdateApplicator::AttemptOneApplication(
     case SUCCESS:
       Advance();
       progress_ = true;
-      successful_ids_.push_back(entry.Get(syncable::ID));
+      application_results_.AddSuccess(entry.Get(syncable::ID));
       break;
     case CONFLICT:
       pointer_++;
-      conflicting_ids_.push_back(entry.Get(syncable::ID));
+      application_results_.AddConflict(entry.Get(syncable::ID));
+      break;
+    case CONFLICT_ENCRYPTION:
+      pointer_++;
+      application_results_.AddEncryptionConflict(entry.Get(syncable::ID));
       break;
     default:
       NOTREACHED();
@@ -110,7 +114,7 @@ bool UpdateApplicator::SkipUpdate(const syncable::Entry& entry) {
 }
 
 bool UpdateApplicator::AllUpdatesApplied() const {
-  return conflicting_ids_.empty() && begin_ == end_;
+  return application_results_.no_conflicts() && begin_ == end_;
 }
 
 void UpdateApplicator::SaveProgressIntoSessionState(
@@ -119,15 +123,54 @@ void UpdateApplicator::SaveProgressIntoSessionState(
   DCHECK(begin_ == end_ || ((pointer_ == end_) && !progress_))
       << "SaveProgress called before updates exhausted.";
 
+  application_results_.SaveProgress(conflict_progress, update_progress);
+}
+
+UpdateApplicator::ResultTracker::ResultTracker(size_t num_results) {
+  successful_ids_.reserve(num_results);
+}
+
+void UpdateApplicator::ResultTracker::AddConflict(syncable::Id id) {
+  conflicting_ids_.push_back(id);
+}
+
+void UpdateApplicator::ResultTracker::AddEncryptionConflict(syncable::Id id) {
+  encryption_conflict_ids_.push_back(id);
+}
+
+void UpdateApplicator::ResultTracker::AddSuccess(syncable::Id id) {
+  successful_ids_.push_back(id);
+}
+
+void UpdateApplicator::ResultTracker::SaveProgress(
+    sessions::ConflictProgress* conflict_progress,
+    sessions::UpdateProgress* update_progress) {
   vector<syncable::Id>::const_iterator i;
   for (i = conflicting_ids_.begin(); i != conflicting_ids_.end(); ++i) {
     conflict_progress->AddConflictingItemById(*i);
     update_progress->AddAppliedUpdate(CONFLICT, *i);
   }
+  for (i = encryption_conflict_ids_.begin();
+       i != encryption_conflict_ids_.end(); ++i) {
+    // Encryption conflicts should not put the syncer into a stuck state. We
+    // mark as conflict, so that we reattempt to apply updates, but add it to
+    // the list of nonblocking conflicts instead of normal conflicts.
+    conflict_progress->AddNonblockingConflictingItemById(*i);
+    update_progress->AddAppliedUpdate(CONFLICT, *i);
+  }
   for (i = successful_ids_.begin(); i != successful_ids_.end(); ++i) {
     conflict_progress->EraseConflictingItemById(*i);
     update_progress->AddAppliedUpdate(SUCCESS, *i);
   }
 }
 
+void UpdateApplicator::ResultTracker::ClearConflicts() {
+  conflicting_ids_.clear();
+  encryption_conflict_ids_.clear();
+}
+
+bool UpdateApplicator::ResultTracker::no_conflicts() const {
+  return conflicting_ids_.empty();
+}
+
 }  // namespace browser_sync
diff --git a/chrome/browser/sync/engine/update_applicator.h b/chrome/browser/sync/engine/update_applicator.h
index f0c5c0b..05a9777 100644
--- a/chrome/browser/sync/engine/update_applicator.h
+++ b/chrome/browser/sync/engine/update_applicator.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// 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.
 //
@@ -56,6 +56,28 @@ class UpdateApplicator {
       sessions::UpdateProgress* update_progress);
 
  private:
+  // Track the status of all applications.
+  // We treat encryption conflicts as nonblocking conflict items when we save
+  // progress.
+  class ResultTracker {
+   public:
+     explicit ResultTracker(size_t num_results);
+     void AddConflict(syncable::Id);
+     void AddEncryptionConflict(syncable::Id);
+     void AddSuccess(syncable::Id);
+     void SaveProgress(sessions::ConflictProgress* conflict_progress,
+                       sessions::UpdateProgress* update_progress);
+     void ClearConflicts();
+
+     // Returns true iff conflicting_ids_ is empty. Does not check
+     // encryption_conflict_ids_.
+     bool no_conflicts() const;
+   private:
+    std::vector<syncable::Id> conflicting_ids_;
+    std::vector<syncable::Id> successful_ids_;
+    std::vector<syncable::Id> encryption_conflict_ids_;
+  };
+
   // If true, AttemptOneApplication will skip over |entry| and return true.
   bool SkipUpdate(const syncable::Entry& entry);
 
@@ -76,9 +98,8 @@ class UpdateApplicator {
 
   const ModelSafeRoutingInfo routing_info_;
 
-  // Track the result of the various items.
-  std::vector<syncable::Id> conflicting_ids_;
-  std::vector<syncable::Id> successful_ids_;
+  // Track the result of the attempts to update applications.
+  ResultTracker application_results_;
 
   DISALLOW_COPY_AND_ASSIGN(UpdateApplicator);
 };
diff --git a/chrome/browser/sync/js_sync_manager_observer_unittest.cc b/chrome/browser/sync/js_sync_manager_observer_unittest.cc
index d90c243..8a15cca 100644
--- a/chrome/browser/sync/js_sync_manager_observer_unittest.cc
+++ b/chrome/browser/sync/js_sync_manager_observer_unittest.cc
@@ -83,6 +83,7 @@ TEST_F(JsSyncManagerObserverTest, OnSyncCycleCompleted) {
                                          false,
                                          true,
                                          100,
+                                         8,
                                          5,
                                          false,
                                          sessions::SyncSourceInfo());
diff --git a/chrome/browser/sync/profile_sync_service_harness.cc b/chrome/browser/sync/profile_sync_service_harness.cc
index fdbbe01..2f6007e 100644
--- a/chrome/browser/sync/profile_sync_service_harness.cc
+++ b/chrome/browser/sync/profile_sync_service_harness.cc
@@ -278,11 +278,14 @@ bool ProfileSyncServiceHarness::RunStateChangeMachine() {
       break;
     }
     case WAITING_FOR_ENCRYPTION: {
+      // If the type whose encryption we are waiting for is now complete, there
+      // is nothing to do. Encryption can take multiple sync cycles, but we only
+      // exit out if we are fully synced or can't connect to the server.
       LogClientInfo("WAITING_FOR_ENCRYPTION", 1);
-      if (IsTypeEncrypted(waiting_for_encryption_type_)) {
-        // Encryption is complete for the type we are waiting on.
+      if (IsSynced() && IsTypeEncrypted(waiting_for_encryption_type_))
         SignalStateCompleteWithNextState(FULLY_SYNCED);
-      }
+      else if (!GetStatus().server_reachable)
+        SignalStateCompleteWithNextState(SERVER_UNREACHABLE);
       break;
     }
     case WAITING_FOR_SYNC_CONFIGURATION: {
@@ -518,7 +521,7 @@ bool ProfileSyncServiceHarness::IsSynced() {
   // TODO(rsimha): Remove additional checks of snap->has_more_to_sync and
   // snap->unsynced_count once http://crbug.com/48989 is fixed.
   return (snap &&
-          snap->num_conflicting_updates == 0 &&  // We can decrypt everything.
+          snap->num_blocking_conflicting_updates == 0 &&
           ServiceIsPushingChanges() &&
           GetStatus().notifications_enabled &&
           !service()->HasUnsyncedItems() &&
@@ -658,6 +661,8 @@ void ProfileSyncServiceHarness::LogClientInfo(const std::string& message,
                       << snap->syncer_status.num_updates_downloaded_total
                       << ", has_more_to_sync: " << snap->has_more_to_sync
                       << ", unsynced_count: " << snap->unsynced_count
+                      << ", num_blocking_conflicting_updates: "
+                      << snap->num_blocking_conflicting_updates
                       << ", num_conflicting_updates: "
                       << snap->num_conflicting_updates
                       << ", has_unsynced_items: "
@@ -695,8 +700,13 @@ bool ProfileSyncServiceHarness::EnableEncryptionForType(
   service_->EncryptDataTypes(encrypted_types);
 
   // Wait some time to let the enryption finish.
+  return WaitForTypeEncryption(type);
+}
+
+bool ProfileSyncServiceHarness::WaitForTypeEncryption(
+    syncable::ModelType type) {
+  // Wait some time to let the enryption finish.
   std::string reason = "Waiting for encryption.";
-  DCHECK_EQ(FULLY_SYNCED, wait_state_);
   wait_state_ = WAITING_FOR_ENCRYPTION;
   waiting_for_encryption_type_ = type;
   if (!AwaitStatusChangeWithTimeout(kLiveSyncOperationTimeoutMs, reason)) {
@@ -705,7 +715,6 @@ bool ProfileSyncServiceHarness::EnableEncryptionForType(
                << " seconds.";
     return false;
   }
-
   return IsTypeEncrypted(type);
 }
 
diff --git a/chrome/browser/sync/profile_sync_service_harness.h b/chrome/browser/sync/profile_sync_service_harness.h
index 1ed972f..a36e4b7 100644
--- a/chrome/browser/sync/profile_sync_service_harness.h
+++ b/chrome/browser/sync/profile_sync_service_harness.h
@@ -131,13 +131,18 @@ class ProfileSyncServiceHarness : public ProfileSyncServiceObserver {
       GetLastSessionSnapshot() const;
 
   // Encrypt the datatype |type|. This method will block while the sync backend
-  // host performs the encryption or a timeout is reached. Returns false if
-  // encryption failed, else true.
-  // Note: this method does not currently support tracking encryption status
-  // while other sync activities are being performed. Sync should be fully
-  // synced when this is called.
+  // host performs the encryption or a timeout is reached.
+  // PostCondition:
+  //   returns: True if |type| was encrypted and we are fully synced.
+  //            False if we timed out.
   bool EnableEncryptionForType(syncable::ModelType type);
 
+  // Wait until |type| is encrypted or we time out.
+  // PostCondition:
+  //   returns: True if |type| is currently encrypted and we are fully synced.
+  //            False if we timed out.
+  bool WaitForTypeEncryption(syncable::ModelType type);
+
   // Check if |type| is encrypted.
   bool IsTypeEncrypted(syncable::ModelType type);
 
diff --git a/chrome/browser/sync/protocol/sync.proto b/chrome/browser/sync/protocol/sync.proto
index c2c1f61..32a8833 100644
--- a/chrome/browser/sync/protocol/sync.proto
+++ b/chrome/browser/sync/protocol/sync.proto
@@ -404,7 +404,7 @@ message ClearUserDataResponse {
 
 message ClientToServerMessage {
   required string share = 1;
-  optional int32 protocol_version = 2 [default = 26];
+  optional int32 protocol_version = 2 [default = 27];
   enum Contents {
     COMMIT = 1;
     GET_UPDATES = 2;
diff --git a/chrome/browser/sync/sessions/session_state.cc b/chrome/browser/sync/sessions/session_state.cc
index 02b6dce..61e25bb 100644
--- a/chrome/browser/sync/sessions/session_state.cc
+++ b/chrome/browser/sync/sessions/session_state.cc
@@ -49,7 +49,9 @@ SyncerStatus::SyncerStatus()
       num_successful_commits(0),
       num_successful_bookmark_commits(0),
       num_updates_downloaded_total(0),
-      num_tombstone_updates_downloaded_total(0) {
+      num_tombstone_updates_downloaded_total(0),
+      num_local_overwrites(0),
+      num_server_overwrites(0) {
 }
 
 SyncerStatus::~SyncerStatus() {
@@ -67,6 +69,8 @@ DictionaryValue* SyncerStatus::ToValue() const {
                 num_updates_downloaded_total);
   value->SetInteger("numTombstoneUpdatesDownloadedTotal",
                 num_tombstone_updates_downloaded_total);
+  value->SetInteger("numLocalOverwrites", num_local_overwrites);
+  value->SetInteger("numServerOverwrites", num_server_overwrites);
   return value;
 }
 
@@ -114,6 +118,7 @@ SyncSessionSnapshot::SyncSessionSnapshot(
     bool more_to_sync,
     bool is_silenced,
     int64 unsynced_count,
+    int num_blocking_conflicting_updates,
     int num_conflicting_updates,
     bool did_commit_items,
     const SyncSourceInfo& source)
@@ -126,6 +131,7 @@ SyncSessionSnapshot::SyncSessionSnapshot(
       has_more_to_sync(more_to_sync),
       is_silenced(is_silenced),
       unsynced_count(unsynced_count),
+      num_blocking_conflicting_updates(num_blocking_conflicting_updates),
       num_conflicting_updates(num_conflicting_updates),
       did_commit_items(did_commit_items),
       source(source) {
@@ -155,6 +161,8 @@ DictionaryValue* SyncSessionSnapshot::ToValue() const {
   // We don't care too much if we lose precision here, also.
   value->SetInteger("unsyncedCount",
                     static_cast<int>(unsynced_count));
+  value->SetInteger("numBlockingConflictingUpdates",
+                    num_blocking_conflicting_updates);
   value->SetInteger("numConflictingUpdates", num_conflicting_updates);
   value->SetBoolean("didCommitItems", did_commit_items);
   value->Set("source", source.ToValue());
@@ -232,6 +240,21 @@ void ConflictProgress::EraseConflictingItemById(const syncable::Id& the_id) {
     *dirty_ = true;
 }
 
+void ConflictProgress::AddNonblockingConflictingItemById(
+    const syncable::Id& the_id) {
+  std::pair<std::set<syncable::Id>::iterator, bool> ret =
+      nonblocking_conflicting_item_ids_.insert(the_id);
+  if (ret.second)
+    *dirty_ = true;
+}
+
+void ConflictProgress::EraseNonblockingConflictingItemById(
+    const syncable::Id& the_id) {
+  int items_erased = nonblocking_conflicting_item_ids_.erase(the_id);
+  if (items_erased != 0)
+    *dirty_ = true;
+}
+
 void ConflictProgress::MergeSets(const syncable::Id& id1,
                                  const syncable::Id& id2) {
   // There are no single item sets, we just leave those entries == 0
diff --git a/chrome/browser/sync/sessions/session_state.h b/chrome/browser/sync/sessions/session_state.h
index 4256cd5..c0c5f39 100644
--- a/chrome/browser/sync/sessions/session_state.h
+++ b/chrome/browser/sync/sessions/session_state.h
@@ -80,6 +80,10 @@ struct SyncerStatus {
   // If the syncer encountered a MIGRATION_DONE code, these are the types that
   // the client must now "migrate", by purging and re-downloading all updates.
   syncable::ModelTypeSet types_needing_local_migration;
+
+  // Overwrites due to conflict resolution counters.
+  int num_local_overwrites;
+  int num_server_overwrites;
 };
 
 // Counters for various errors that can occur repeatedly during a sync session.
@@ -120,6 +124,7 @@ struct SyncSessionSnapshot {
       bool more_to_sync,
       bool is_silenced,
       int64 unsynced_count,
+      int num_blocking_conflicting_updates,
       int num_conflicting_updates,
       bool did_commit_items,
       const SyncSourceInfo& source);
@@ -137,6 +142,7 @@ struct SyncSessionSnapshot {
   const bool has_more_to_sync;
   const bool is_silenced;
   const int64 unsynced_count;
+  const int num_blocking_conflicting_updates;
   const int num_conflicting_updates;
   const bool did_commit_items;
   const SyncSourceInfo source;
@@ -166,6 +172,13 @@ class ConflictProgress {
   std::set<syncable::Id>::const_iterator ConflictingItemsBeginConst() const;
   std::set<syncable::Id>::const_iterator ConflictingItemsEnd() const;
 
+  // Mutators for nonblocking conflicting items (see description below).
+  void AddNonblockingConflictingItemById(const syncable::Id& the_id);
+  void EraseNonblockingConflictingItemById(const syncable::Id& the_id);
+  int NonblockingConflictingItemsSize() const {
+    return nonblocking_conflicting_item_ids_.size();
+  }
+
   void MergeSets(const syncable::Id& set1, const syncable::Id& set2);
   void CleanupSets();
 
@@ -175,6 +188,19 @@ class ConflictProgress {
   std::map<syncable::Id, ConflictSet*> id_to_conflict_set_;
   std::set<ConflictSet*> conflict_sets_;
 
+  // Nonblocking conflicts are those which should not block forward progress
+  // (they will not result in the syncer being stuck). This currently only
+  // includes entries we cannot yet decrypt because the passphrase has not
+  // arrived.
+  // With nonblocking conflicts, we want to go to the syncer's
+  // APPLY_UPDATES_TO_RESOLVE_CONFLICTS step, but we want to ignore them after.
+  // Because they are not passed to the conflict resolver, they do not trigger
+  // syncer_stuck.
+  // TODO(zea): at some point we may have nonblocking conflicts that should be
+  // resolved in the conflict resolver. We'll need to change this then.
+  // See http://crbug.com/76596.
+  std::set<syncable::Id> nonblocking_conflicting_item_ids_;
+
   // Whether a conflicting item was added or removed since
   // the last call to reset_progress_changed(), if any. In practice this
   // points to StatusController::is_dirty_.
diff --git a/chrome/browser/sync/sessions/session_state_unittest.cc b/chrome/browser/sync/sessions/session_state_unittest.cc
index 6ad378b..3ee33d0 100644
--- a/chrome/browser/sync/sessions/session_state_unittest.cc
+++ b/chrome/browser/sync/sessions/session_state_unittest.cc
@@ -50,9 +50,11 @@ TEST_F(SessionStateTest, SyncerStatusToValue) {
   status.num_successful_bookmark_commits = 10;
   status.num_updates_downloaded_total = 100;
   status.num_tombstone_updates_downloaded_total = 200;
+  status.num_local_overwrites = 15;
+  status.num_server_overwrites = 18;
 
   scoped_ptr<DictionaryValue> value(status.ToValue());
-  EXPECT_EQ(7u, value->size());
+  EXPECT_EQ(9u, value->size());
   ExpectDictBooleanValue(status.invalid_store, *value, "invalidStore");
   ExpectDictBooleanValue(status.syncer_stuck, *value, "syncerStuck");
   ExpectDictBooleanValue(status.syncing, *value, "syncing");
@@ -64,6 +66,10 @@ TEST_F(SessionStateTest, SyncerStatusToValue) {
                          *value, "numUpdatesDownloadedTotal");
   ExpectDictIntegerValue(status.num_tombstone_updates_downloaded_total,
                          *value, "numTombstoneUpdatesDownloadedTotal");
+  ExpectDictIntegerValue(status.num_local_overwrites,
+                         *value, "numLocalOverwrites");
+  ExpectDictIntegerValue(status.num_server_overwrites,
+                         *value, "numServerOverwrites");
 }
 
 TEST_F(SessionStateTest, ErrorCountersToValue) {
@@ -134,6 +140,7 @@ TEST_F(SessionStateTest, SyncSessionSnapshotToValue) {
   const bool kHasMoreToSync = false;
   const bool kIsSilenced = true;
   const int kUnsyncedCount = 1053;
+  const int kNumBlockingConflictingUpdates = 1054;
   const int kNumConflictingUpdates = 1055;
   const bool kDidCommitItems = true;
 
@@ -149,11 +156,12 @@ TEST_F(SessionStateTest, SyncSessionSnapshotToValue) {
                                kHasMoreToSync,
                                kIsSilenced,
                                kUnsyncedCount,
+                               kNumBlockingConflictingUpdates,
                                kNumConflictingUpdates,
                                kDidCommitItems,
                                source);
   scoped_ptr<DictionaryValue> value(snapshot.ToValue());
-  EXPECT_EQ(12u, value->size());
+  EXPECT_EQ(13u, value->size());
   ExpectDictDictionaryValue(*expected_syncer_status_value, *value,
                             "syncerStatus");
   ExpectDictDictionaryValue(*expected_errors_value, *value, "errors");
@@ -167,6 +175,8 @@ TEST_F(SessionStateTest, SyncSessionSnapshotToValue) {
   ExpectDictBooleanValue(kHasMoreToSync, *value, "hasMoreToSync");
   ExpectDictBooleanValue(kIsSilenced, *value, "isSilenced");
   ExpectDictIntegerValue(kUnsyncedCount, *value, "unsyncedCount");
+  ExpectDictIntegerValue(kNumBlockingConflictingUpdates, *value,
+                         "numBlockingConflictingUpdates");
   ExpectDictIntegerValue(kNumConflictingUpdates, *value,
                          "numConflictingUpdates");
   ExpectDictBooleanValue(kDidCommitItems, *value,
diff --git a/chrome/browser/sync/sessions/status_controller.cc b/chrome/browser/sync/sessions/status_controller.cc
index 8266f0f..c938fe5 100644
--- a/chrome/browser/sync/sessions/status_controller.cc
+++ b/chrome/browser/sync/sessions/status_controller.cc
@@ -1,9 +1,11 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// 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/sessions/status_controller.h"
 
+#include <vector>
+
 #include "base/basictypes.h"
 #include "chrome/browser/sync/syncable/model_type.h"
 
@@ -139,6 +141,14 @@ void StatusController::increment_num_successful_commits() {
   shared_.syncer_status.mutate()->num_successful_commits++;
 }
 
+void StatusController::increment_num_local_overwrites() {
+  shared_.syncer_status.mutate()->num_local_overwrites++;
+}
+
+void StatusController::increment_num_server_overwrites() {
+  shared_.syncer_status.mutate()->num_server_overwrites++;
+}
+
 void StatusController::set_commit_set(const OrderedCommitSet& commit_set) {
   DCHECK(!group_restriction_in_effect_);
   shared_.commit_set = commit_set;
@@ -185,6 +195,18 @@ bool StatusController::HasConflictingUpdates() const {
   return false;
 }
 
+int StatusController::TotalNumBlockingConflictingItems() const {
+  DCHECK(!group_restriction_in_effect_)
+      << "TotalNumBlockingConflictingItems applies to all ModelSafeGroups";
+  std::map<ModelSafeGroup, PerModelSafeGroupState*>::const_iterator it =
+      per_model_group_.begin();
+  int sum = 0;
+  for (; it != per_model_group_.end(); ++it) {
+    sum += it->second->conflict_progress.ConflictingItemsSize();
+  }
+  return sum;
+}
+
 int StatusController::TotalNumConflictingItems() const {
   DCHECK(!group_restriction_in_effect_)
       << "TotalNumConflictingItems applies to all ModelSafeGroups";
@@ -193,6 +215,7 @@ int StatusController::TotalNumConflictingItems() const {
   int sum = 0;
   for (; it != per_model_group_.end(); ++it) {
     sum += it->second->conflict_progress.ConflictingItemsSize();
+    sum += it->second->conflict_progress.NonblockingConflictingItemsSize();
   }
   return sum;
 }
diff --git a/chrome/browser/sync/sessions/status_controller.h b/chrome/browser/sync/sessions/status_controller.h
index d4c912a..0f76782 100644
--- a/chrome/browser/sync/sessions/status_controller.h
+++ b/chrome/browser/sync/sessions/status_controller.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -33,6 +33,7 @@
 #define CHROME_BROWSER_SYNC_SESSIONS_STATUS_CONTROLLER_H_
 #pragma once
 
+#include <vector>
 #include <map>
 
 #include "base/stl_util-inl.h"
@@ -150,10 +151,17 @@ class StatusController {
 
   // If a GetUpdates for any data type resulted in downloading an update that
   // is in conflict, this method returns true.
+  // Note: this includes non-blocking conflicts.
   bool HasConflictingUpdates() const;
 
   // Aggregate sum of ConflictingItemSize() over all ConflictProgress objects
   // (one for each ModelSafeGroup currently in-use).
+  // Note: this does not include non-blocking conflicts.
+  int TotalNumBlockingConflictingItems() const;
+
+  // Aggregate sum of ConflictingItemSize() and NonblockingConflictingItemsSize
+  // over all ConflictProgress objects (one for each ModelSafeGroup currently
+  // in-use).
   int TotalNumConflictingItems() const;
 
   // Returns the number of updates received from the sync server.
@@ -214,6 +222,8 @@ class StatusController {
   void increment_num_tombstone_updates_downloaded_by(int value);
   void set_types_needing_local_migration(const syncable::ModelTypeSet& types);
   void set_unsynced_handles(const std::vector<int64>& unsynced_handles);
+  void increment_num_local_overwrites();
+  void increment_num_server_overwrites();
 
   void set_commit_set(const OrderedCommitSet& commit_set);
   void update_conflict_sets_built(bool built);
diff --git a/chrome/browser/sync/sessions/sync_session.cc b/chrome/browser/sync/sessions/sync_session.cc
index 3a1e9b9..fcd11f49 100644
--- a/chrome/browser/sync/sessions/sync_session.cc
+++ b/chrome/browser/sync/sessions/sync_session.cc
@@ -112,6 +112,7 @@ SyncSessionSnapshot SyncSession::TakeSnapshot() const {
       HasMoreToSync(),
       delegate_->IsSyncingCurrentlySilenced(),
       status_controller_->unsynced_handles().size(),
+      status_controller_->TotalNumBlockingConflictingItems(),
       status_controller_->TotalNumConflictingItems(),
       status_controller_->did_commit_items(),
       source_);
diff --git a/chrome/browser/sync/sync_ui_util.cc b/chrome/browser/sync/sync_ui_util.cc
index 989a996..75859d02 100644
--- a/chrome/browser/sync/sync_ui_util.cc
+++ b/chrome/browser/sync/sync_ui_util.cc
@@ -370,6 +370,12 @@ void ConstructAboutInformation(ProfileSyncService* service,
     sync_ui_util::AddIntSyncDetail(details,
                                    "Conflicting Count",
                                    full_status.conflicting_count);
+    sync_ui_util::AddIntSyncDetail(details,
+                                   "Local Overwrites",
+                                   full_status.num_local_overwrites_total);
+    sync_ui_util::AddIntSyncDetail(details,
+                                   "Server Overwrites",
+                                   full_status.num_server_overwrites_total);
     sync_ui_util::AddBoolSyncDetail(details, "Syncing", full_status.syncing);
     sync_ui_util::AddBoolSyncDetail(details,
                                     "Initial Sync Ended",
diff --git a/chrome/browser/sync/test_profile_sync_service.cc b/chrome/browser/sync/test_profile_sync_service.cc
index f4a0e78..4d0f6d7 100644
--- a/chrome/browser/sync/test_profile_sync_service.cc
+++ b/chrome/browser/sync/test_profile_sync_service.cc
@@ -64,7 +64,7 @@ void SyncBackendHostForProfileSyncTest::
   }
   core_->HandleSyncCycleCompletedOnFrontendLoop(new SyncSessionSnapshot(
       SyncerStatus(), ErrorCounters(), 0, false,
-      sync_ended, download_progress_markers, false, false, 0, 0, false,
+      sync_ended, download_progress_markers, false, false, 0, 0, 0, false,
       SyncSourceInfo()));
 }