[Sync] Implement encryption-aware conflict resolution.

chrome/browser/sync/engine/syncer_unittest.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.
 //
 // Syncer unit tests. Unfortunately a lot of these tests
 // are outdated and need to be reworked and updated.
 
 #include <algorithm>
 #include <limits>
 #include <list>
@@ skipping 16 matching lines @@
 #include "chrome/browser/sync/engine/net/server_connection_manager.h"
 #include "chrome/browser/sync/engine/process_updates_command.h"
 #include "chrome/browser/sync/engine/nigori_util.h"
 #include "chrome/browser/sync/engine/syncer.h"
 #include "chrome/browser/sync/engine/syncer_proto_util.h"
 #include "chrome/browser/sync/engine/syncer_util.h"
 #include "chrome/browser/sync/engine/syncproto.h"
 #include "chrome/browser/sync/protocol/sync.pb.h"
 #include "chrome/browser/sync/protocol/bookmark_specifics.pb.h"
 #include "chrome/browser/sync/protocol/nigori_specifics.pb.h"
+#include "chrome/browser/sync/protocol/preference_specifics.pb.h"
 #include "chrome/browser/sync/sessions/sync_session_context.h"
 #include "chrome/browser/sync/syncable/directory_manager.h"
 #include "chrome/browser/sync/syncable/model_type.h"
 #include "chrome/browser/sync/syncable/syncable.h"
 #include "chrome/browser/sync/test/engine/fake_model_worker.h"
 #include "chrome/browser/sync/test/engine/mock_connection_manager.h"
 #include "chrome/browser/sync/test/engine/test_directory_setter_upper.h"
 #include "chrome/browser/sync/test/engine/test_id_factory.h"
 #include "chrome/browser/sync/test/engine/test_syncable_utils.h"
 #include "chrome/browser/sync/util/cryptographer.h"
@@ skipping 34 matching lines @@
 using syncable::IS_DEL;
 using syncable::IS_DIR;
 using syncable::IS_UNAPPLIED_UPDATE;
 using syncable::IS_UNSYNCED;
 using syncable::META_HANDLE;
 using syncable::MTIME;
 using syncable::NEXT_ID;
 using syncable::NON_UNIQUE_NAME;
 using syncable::PARENT_ID;
 using syncable::PREV_ID;
+using syncable::PREV_SERVER_SPECIFICS;
 using syncable::SERVER_IS_DEL;
 using syncable::SERVER_NON_UNIQUE_NAME;
 using syncable::SERVER_PARENT_ID;
 using syncable::SERVER_POSITION_IN_PARENT;
 using syncable::SERVER_SPECIFICS;
 using syncable::SERVER_VERSION;
 using syncable::UNIQUE_CLIENT_TAG;
 using syncable::UNIQUE_SERVER_TAG;
 using syncable::SPECIFICS;
 using syncable::SYNCING;
@@ skipping 71 matching lines @@
     GetModelSafeRoutingInfo(&info);
     GetWorkers(&workers);
     syncable::ModelTypePayloadMap types =
         syncable::ModelTypePayloadMapFromRoutingInfo(info, std::string());
     return new SyncSession(context_.get(), this,
         sessions::SyncSourceInfo(sync_pb::GetUpdatesCallerInfo::UNKNOWN, types),
         info, workers);
   }
 
   bool SyncShareAsDelegate() {
-    scoped_ptr<SyncSession> session(MakeSession());
-    syncer_->SyncShare(session.get(), SYNCER_BEGIN, SYNCER_END);
-    return session->HasMoreToSync();
+    session_.reset(MakeSession());
+    syncer_->SyncShare(session_.get(), SYNCER_BEGIN, SYNCER_END);
+    return session_->HasMoreToSync();
   }
 
   void LoopSyncShare() {
     bool should_loop = false;
     int loop_iterations = 0;
     do {
       ASSERT_LT(++loop_iterations, 100) << "infinite loop detected. please fix";
       should_loop = SyncShareAsDelegate();
     } while (should_loop);
   }
 
   virtual void SetUp() {
     syncdb_.SetUp();
 
     mock_server_.reset(
         new MockConnectionManager(syncdb_.manager(), syncdb_.name()));
     EnableDatatype(syncable::BOOKMARKS);
     EnableDatatype(syncable::NIGORI);
+    EnableDatatype(syncable::PREFERENCES);
+    EnableDatatype(syncable::NIGORI);
     worker_ = new FakeModelWorker(GROUP_PASSIVE);
     std::vector<SyncEngineEventListener*> listeners;
     listeners.push_back(this);
     context_.reset(new SyncSessionContext(mock_server_.get(),
         syncdb_.manager(), this, listeners, NULL));
     context_->set_account_name(syncdb_.name());
     ASSERT_FALSE(context_->resolver());
     syncer_ = new Syncer();
     session_.reset(MakeSession());
 
@@ skipping 342 matching lines @@
   expected_order.push_back(ids_.MakeLocal("b"));
   expected_order.push_back(ids_.MakeLocal("c"));
   expected_order.push_back(ids_.MakeLocal("e"));
   DoTruncationTest(dir, unsynced_handle_view, expected_order);
 }
 
 TEST_F(SyncerTest, GetCommitIdsFiltersThrottledEntries) {
   ScopedDirLookup dir(syncdb_.manager(), syncdb_.name());
   ASSERT_TRUE(dir.good());
   const syncable::ModelTypeSet throttled_types(syncable::BOOKMARKS);
-  KeyParams key_params = {"localhost", "dummy", "foobar"};
   sync_pb::EntitySpecifics bookmark_data;
   AddDefaultExtensionValue(syncable::BOOKMARKS, &bookmark_data);
 
   mock_server_->AddUpdateDirectory(1, 0, "A", 10, 10);
   SyncShareAsDelegate();
 
   {
     WriteTransaction wtrans(FROM_HERE, UNITTEST, dir);
     MutableEntry A(&wtrans, GET_BY_ID, ids_.FromNumber(1));
     ASSERT_TRUE(A.good());
@@ skipping 23 matching lines @@
   SyncShareAsDelegate();
   {
     // It should have been committed.
     ReadTransaction rtrans(FROM_HERE, dir);
     Entry entryA(&rtrans, syncable::GET_BY_ID, ids_.FromNumber(1));
     ASSERT_TRUE(entryA.good());
     EXPECT_FALSE(entryA.Get(IS_UNSYNCED));
   }
 }
 
+// We use a macro so we can preserve the error location.
+#define VERIFY_ENTRY(id, is_unapplied, is_unsynced, prev_initialized, \
+                     parent_id, version, server_version, id_fac, rtrans) \
+  do { \
+    Entry entryA(rtrans, syncable::GET_BY_ID, id_fac.FromNumber(id)); \
+    ASSERT_TRUE(entryA.good()); \
+    EXPECT_EQ(is_unsynced, entryA.Get(IS_UNSYNCED)); \
+    EXPECT_EQ(is_unapplied, entryA.Get(IS_UNAPPLIED_UPDATE)); \
+    EXPECT_EQ(prev_initialized, \
+              syncable::IsRealDataType(syncable::GetModelTypeFromSpecifics( \
+                  entryA.Get(PREV_SERVER_SPECIFICS)))); \
+    EXPECT_TRUE(parent_id == -1 || \
+                entryA.Get(PARENT_ID) == id_fac.FromNumber(parent_id)); \
+    EXPECT_EQ(version, entryA.Get(BASE_VERSION)); \
+    EXPECT_EQ(server_version, entryA.Get(SERVER_VERSION)); \
+  } while (0)
+
 TEST_F(SyncerTest, GetCommitIdsFiltersUnreadyEntries) {
   ScopedDirLookup dir(syncdb_.manager(), syncdb_.name());
   ASSERT_TRUE(dir.good());
   KeyParams key_params = {"localhost", "dummy", "foobar"};
-  sync_pb::EncryptedData encrypted;
-  sync_pb::EntitySpecifics encrypted_bookmark;
-  encrypted_bookmark.mutable_encrypted();
+  KeyParams other_params = {"localhost", "dummy", "foobar2"};
+  sync_pb::EntitySpecifics bookmark, encrypted_bookmark;
+  bookmark.MutableExtension(sync_pb::bookmark)->set_url("url");
+  bookmark.MutableExtension(sync_pb::bookmark)->set_title("title");
   AddDefaultExtensionValue(syncable::BOOKMARKS, &encrypted_bookmark);
   mock_server_->AddUpdateDirectory(1, 0, "A", 10, 10);
   mock_server_->AddUpdateDirectory(2, 0, "B", 10, 10);
   mock_server_->AddUpdateDirectory(3, 0, "C", 10, 10);
   mock_server_->AddUpdateDirectory(4, 0, "D", 10, 10);
   SyncShareAsDelegate();
   // Server side change will put A in conflict.
   mock_server_->AddUpdateDirectory(1, 0, "A", 20, 20);
   {
     // Mark bookmarks as encrypted and set the cryptographer to have pending
     // keys.
     WriteTransaction wtrans(FROM_HERE, UNITTEST, dir);
     browser_sync::Cryptographer other_cryptographer;
-    other_cryptographer.AddKey(key_params);
+    other_cryptographer.AddKey(other_params);
     sync_pb::EntitySpecifics specifics;
     sync_pb::NigoriSpecifics* nigori =
         specifics.MutableExtension(sync_pb::nigori);
     other_cryptographer.GetKeys(nigori->mutable_encrypted());
     nigori->set_encrypt_bookmarks(true);
+    // Set up with an old passphrase, but have pending keys
+    syncdb_.manager()->GetCryptographer(&wtrans)->AddKey(key_params);
+    syncdb_.manager()->GetCryptographer(&wtrans)->Encrypt(
+        bookmark,
+        encrypted_bookmark.mutable_encrypted());
     syncdb_.manager()->GetCryptographer(&wtrans)->Update(*nigori);
 
     // In conflict but properly encrypted.
     MutableEntry A(&wtrans, GET_BY_ID, ids_.FromNumber(1));
     ASSERT_TRUE(A.good());
     A.Put(IS_UNSYNCED, true);
     A.Put(SPECIFICS, encrypted_bookmark);
     A.Put(NON_UNIQUE_NAME, kEncryptedString);
     // Not in conflict and properly encrypted.
     MutableEntry B(&wtrans, GET_BY_ID, ids_.FromNumber(2));
@@ skipping 10 matching lines @@
     MutableEntry D(&wtrans, GET_BY_ID, ids_.FromNumber(4));
     ASSERT_TRUE(D.good());
     D.Put(IS_UNSYNCED, true);
     D.Put(SPECIFICS, encrypted_bookmark);
     D.Put(NON_UNIQUE_NAME, "not encrypted");
   }
   SyncShareAsDelegate();
   {
     // We remove any unready entries from the status controller's unsynced
     // handles, so this should remain 0 even though the entries didn't commit.
-    ASSERT_EQ(0U, session_->status_controller().unsynced_handles().size());
+    EXPECT_EQ(0U, session_->status_controller().unsynced_handles().size());
     // Nothing should have commited due to bookmarks being encrypted and
     // the cryptographer having pending keys. A would have been resolved
     // as a simple conflict, but still be unsynced until the next sync cycle.
     ReadTransaction rtrans(FROM_HERE, dir);
-    Entry entryA(&rtrans, syncable::GET_BY_ID, ids_.FromNumber(1));
-    ASSERT_TRUE(entryA.good());
-    EXPECT_TRUE(entryA.Get(IS_UNSYNCED));
-    Entry entryB(&rtrans, syncable::GET_BY_ID, ids_.FromNumber(2));
-    ASSERT_TRUE(entryB.good());
-    EXPECT_TRUE(entryB.Get(IS_UNSYNCED));
-    Entry entryC(&rtrans, syncable::GET_BY_ID, ids_.FromNumber(3));
-    ASSERT_TRUE(entryC.good());
-    EXPECT_TRUE(entryC.Get(IS_UNSYNCED));
-    Entry entryD(&rtrans, syncable::GET_BY_ID, ids_.FromNumber(4));
-    ASSERT_TRUE(entryD.good());
-    EXPECT_TRUE(entryD.Get(IS_UNSYNCED));
+    VERIFY_ENTRY(1, false, true, false, 0, 20, 20, ids_, &rtrans);
+    VERIFY_ENTRY(2, false, true, false, 0, 10, 10, ids_, &rtrans);
+    VERIFY_ENTRY(3, false, true, false, 0, 10, 10, ids_, &rtrans);
+    VERIFY_ENTRY(4, false, true, false, 0, 10, 10, ids_, &rtrans);
 
     // Resolve the pending keys.
     syncdb_.manager()->GetCryptographer(&rtrans)->DecryptPendingKeys(
-        key_params);
+        other_params);
   }
   SyncShareAsDelegate();
   {
-    ASSERT_EQ(0U, session_->status_controller().unsynced_handles().size());
+    // 2 unsynced handles to reflect the items that committed succesfully.
+    EXPECT_EQ(2U, session_->status_controller().unsynced_handles().size());
     // All properly encrypted and non-conflicting items should commit. "A" was
     // conflicting, but last sync cycle resolved it as simple conflict, so on
     // this sync cycle it committed succesfullly.
     ReadTransaction rtrans(FROM_HERE, dir);
     // Committed successfully.
-    Entry entryA(&rtrans, syncable::GET_BY_ID, ids_.FromNumber(1));
-    ASSERT_TRUE(entryA.good());
-    EXPECT_FALSE(entryA.Get(IS_UNSYNCED));
-    EXPECT_FALSE(entryA.Get(IS_UNAPPLIED_UPDATE));
+    VERIFY_ENTRY(1, false, false, false, 0, 21, 21, ids_, &rtrans);
     // Committed successfully.
-    Entry entryB(&rtrans, syncable::GET_BY_ID, ids_.FromNumber(2));
-    ASSERT_TRUE(entryB.good());
-    EXPECT_FALSE(entryB.Get(IS_UNSYNCED));
-    EXPECT_FALSE(entryB.Get(IS_UNAPPLIED_UPDATE));
+    VERIFY_ENTRY(2, false, false, false, 0, 11, 11, ids_, &rtrans);
     // Was not properly encrypted.
-    Entry entryC(&rtrans, syncable::GET_BY_ID, ids_.FromNumber(3));
-    ASSERT_TRUE(entryC.good());
-    EXPECT_TRUE(entryC.Get(IS_UNSYNCED));
-    EXPECT_FALSE(entryC.Get(IS_UNAPPLIED_UPDATE));
+    VERIFY_ENTRY(3, false, true, false, 0, 10, 10, ids_, &rtrans);
     // Was not properly encrypted.
-    Entry entryD(&rtrans, syncable::GET_BY_ID, ids_.FromNumber(4));
-    ASSERT_TRUE(entryD.good());
-    EXPECT_TRUE(entryD.Get(IS_UNSYNCED));
-    EXPECT_FALSE(entryD.Get(IS_UNAPPLIED_UPDATE));
+    VERIFY_ENTRY(4, false, true, false, 0, 10, 10, ids_, &rtrans);
   }
   {
     // Fix the remaining items.
     WriteTransaction wtrans(FROM_HERE, UNITTEST, dir);
     MutableEntry C(&wtrans, GET_BY_ID, ids_.FromNumber(3));
     ASSERT_TRUE(C.good());
     C.Put(SPECIFICS, encrypted_bookmark);
     C.Put(NON_UNIQUE_NAME, kEncryptedString);
     MutableEntry D(&wtrans, GET_BY_ID, ids_.FromNumber(4));
     ASSERT_TRUE(D.good());
     D.Put(SPECIFICS, encrypted_bookmark);
     D.Put(NON_UNIQUE_NAME, kEncryptedString);
   }
   SyncShareAsDelegate();
   {
-    ASSERT_EQ(0U, session_->status_controller().unsynced_handles().size());
+    // We attempted to commit two items.
+    EXPECT_EQ(2U, session_->status_controller().unsynced_handles().size());
+    EXPECT_TRUE(session_->status_controller().did_commit_items());
     // None should be unsynced anymore.
     ReadTransaction rtrans(FROM_HERE, dir);
-    Entry entryA(&rtrans, syncable::GET_BY_ID, ids_.FromNumber(1));
-    ASSERT_TRUE(entryA.good());
-    EXPECT_FALSE(entryA.Get(IS_UNSYNCED));
-    EXPECT_FALSE(entryA.Get(IS_UNAPPLIED_UPDATE));
-    Entry entryB(&rtrans, syncable::GET_BY_ID, ids_.FromNumber(2));
-    ASSERT_TRUE(entryB.good());
-    EXPECT_FALSE(entryB.Get(IS_UNSYNCED));
-    EXPECT_FALSE(entryB.Get(IS_UNAPPLIED_UPDATE));
-    Entry entryC(&rtrans, syncable::GET_BY_ID, ids_.FromNumber(3));
-    ASSERT_TRUE(entryC.good());
-    EXPECT_FALSE(entryC.Get(IS_UNSYNCED));
-    EXPECT_FALSE(entryC.Get(IS_UNAPPLIED_UPDATE));
-    Entry entryD(&rtrans, syncable::GET_BY_ID, ids_.FromNumber(4));
-    ASSERT_TRUE(entryD.good());
-    EXPECT_FALSE(entryD.Get(IS_UNSYNCED));
-    EXPECT_FALSE(entryD.Get(IS_UNAPPLIED_UPDATE));
+    VERIFY_ENTRY(1, false, false, false, 0, 21, 21, ids_, &rtrans);
+    VERIFY_ENTRY(2, false, false, false, 0, 11, 11, ids_, &rtrans);
+    VERIFY_ENTRY(3, false, false, false, 0, 11, 11, ids_, &rtrans);
+    VERIFY_ENTRY(4, false, false, false, 0, 11, 11, ids_, &rtrans);
   }
 }
 
+TEST_F(SyncerTest, EncryptionAwareConflicts) {

[tim (not reviewing), 2011/12/15 20:55:11]

I like the coverage but.. it would be _REALLY_ nice to split this into multiple
subtests :/ If something breaks it's a mess to dig through...

+  ScopedDirLookup dir(syncdb_.manager(), syncdb_.name());
+  ASSERT_TRUE(dir.good());
+  KeyParams key_params = {"localhost", "dummy", "foobar"};
+  browser_sync::Cryptographer other_cryptographer;
+  other_cryptographer.AddKey(key_params);
+  sync_pb::EntitySpecifics bookmark, encrypted_bookmark, modified_bookmark;
+  bookmark.MutableExtension(sync_pb::bookmark)->set_title("title");
+  other_cryptographer.Encrypt(bookmark,
+                              encrypted_bookmark.mutable_encrypted());
+  AddDefaultExtensionValue(syncable::BOOKMARKS, &encrypted_bookmark);
+  modified_bookmark.MutableExtension(sync_pb::bookmark)->set_title("title2");
+  other_cryptographer.Encrypt(modified_bookmark,
+                              modified_bookmark.mutable_encrypted());
+  sync_pb::EntitySpecifics pref, encrypted_pref, modified_pref;
+  pref.MutableExtension(sync_pb::preference)->set_name("name");
+  AddDefaultExtensionValue(syncable::PREFERENCES, &encrypted_pref);
+  other_cryptographer.Encrypt(pref,
+                              encrypted_pref.mutable_encrypted());
+  modified_pref.MutableExtension(sync_pb::preference)->set_name("name2");
+  other_cryptographer.Encrypt(modified_pref,
+                              modified_pref.mutable_encrypted());
+  {
+    // Mark bookmarks and preferences as encrypted and set the cryptographer to
+    // have pending keys.
+    WriteTransaction wtrans(FROM_HERE, UNITTEST, dir);
+    sync_pb::EntitySpecifics specifics;
+    sync_pb::NigoriSpecifics* nigori =
+        specifics.MutableExtension(sync_pb::nigori);
+    other_cryptographer.GetKeys(nigori->mutable_encrypted());
+    nigori->set_encrypt_bookmarks(true);
+    nigori->set_encrypt_preferences(true);
+    syncdb_.manager()->GetCryptographer(&wtrans)->Update(*nigori);
+    EXPECT_TRUE(syncdb_.manager()->GetCryptographer(&wtrans)->
+                    has_pending_keys());
+  }
+
+  mock_server_->AddUpdateSpecifics(1, 0, "A", 10, 10, true, 0, bookmark);
+  mock_server_->AddUpdateSpecifics(2, 1, "B", 10, 10, false, 2, bookmark);
+  mock_server_->AddUpdateSpecifics(3, 1, "C", 10, 10, false, 1, bookmark);
+  mock_server_->AddUpdateSpecifics(4, 0, "D", 10, 10, false, 0, pref);
+  SyncShareAsDelegate();
+  {
+    EXPECT_EQ(0U, session_->status_controller().unsynced_handles().size());
+    // Initial state. Everything is normal.
+    ReadTransaction rtrans(FROM_HERE, dir);
+    VERIFY_ENTRY(1, false, false, false, 0, 10, 10, ids_, &rtrans);
+    VERIFY_ENTRY(2, false, false, false, 1, 10, 10, ids_, &rtrans);
+    VERIFY_ENTRY(3, false, false, false, 1, 10, 10, ids_, &rtrans);
+    VERIFY_ENTRY(4, false, false, false, 0, 10, 10, ids_, &rtrans);
+  }
+  // Server side encryption will not be applied due to undecryptable data.
+  // At this point, PREV_SERVER_SPECIFICS should be filled for all four items.
+  mock_server_->AddUpdateSpecifics(1, 0, kEncryptedString, 20, 20, true, 0,
+                                   encrypted_bookmark);
+  mock_server_->AddUpdateSpecifics(2, 1, kEncryptedString, 20, 20, false, 2,
+                                   encrypted_bookmark);
+  mock_server_->AddUpdateSpecifics(3, 1, kEncryptedString, 20, 20, false, 1,
+                                   encrypted_bookmark);
+  mock_server_->AddUpdateSpecifics(4, 0, kEncryptedString, 20, 20, false, 0,
+                                   encrypted_pref);
+  SyncShareAsDelegate();
+  {
+    EXPECT_EQ(0U, session_->status_controller().unsynced_handles().size());
+    // All should be unapplied due to being undecryptable and have a valid
+    // PREV_SERVER_SPECIFICS.
+    ReadTransaction rtrans(FROM_HERE, dir);
+    VERIFY_ENTRY(1, true, false, true, 0, 10, 20, ids_, &rtrans);
+    VERIFY_ENTRY(2, true, false, true, 1, 10, 20, ids_, &rtrans);
+    VERIFY_ENTRY(3, true, false, true, 1, 10, 20, ids_, &rtrans);
+    VERIFY_ENTRY(4, true, false, true, 0, 10, 20, ids_, &rtrans);
+  }
+  // Server side change that don't modify anything should not affect
+  // PREV_SERVER_SPECIFICS (such as name changes and mtime changes).
+  mock_server_->AddUpdateSpecifics(1, 0, kEncryptedString, 30, 30, true, 0,
+                                   encrypted_bookmark);
+  mock_server_->AddUpdateSpecifics(2, 1, kEncryptedString, 30, 30, false, 2,
+                                   encrypted_bookmark);
+  // Item 3 doesn't change.
+  mock_server_->AddUpdateSpecifics(4, 0, kEncryptedString, 30, 30, false, 0,
+                                   encrypted_pref);
+  SyncShareAsDelegate();
+  {
+    EXPECT_EQ(0U, session_->status_controller().unsynced_handles().size());
+    // Items 1, 2, and 4 should have newer server versions, 3 remains the same.
+    // All should remain unapplied due to be undecryptable.
+    ReadTransaction rtrans(FROM_HERE, dir);
+    VERIFY_ENTRY(1, true, false, true, 0, 10, 30, ids_, &rtrans);
+    VERIFY_ENTRY(2, true, false, true, 1, 10, 30, ids_, &rtrans);
+    VERIFY_ENTRY(3, true, false, true, 1, 10, 20, ids_, &rtrans);
+    VERIFY_ENTRY(4, true, false, true, 0, 10, 30, ids_, &rtrans);
+  }
+  // Positional changes, parent changes, and specifics changes should reset
+  // PREV_SERVER_SPECIFICS.
+  // Became unencrypted.
+  mock_server_->AddUpdateSpecifics(1, 0, "A", 40, 40, true, 0, bookmark);
+  // Reordered to after item 2.
+  mock_server_->AddUpdateSpecifics(3, 1, kEncryptedString, 30, 30, false, 3,
+                                   encrypted_bookmark);
+  SyncShareAsDelegate();
+  {
+    EXPECT_EQ(0U, session_->status_controller().unsynced_handles().size());
+    // Items 2 and 4 should be the only ones with PREV_SERVER_SPECIFICS set.
+    // Items 1 is now unencrypted, so should have applied normally.
+    ReadTransaction rtrans(FROM_HERE, dir);
+    VERIFY_ENTRY(1, false, false, false, 0, 40, 40, ids_, &rtrans);
+    VERIFY_ENTRY(2, true, false, true, 1, 10, 30, ids_, &rtrans);
+    VERIFY_ENTRY(3, true, false, false, 1, 10, 30, ids_, &rtrans);
+    VERIFY_ENTRY(4, true, false, true, 0, 10, 30, ids_, &rtrans);
+  }
+  // Make local changes, which should remain unsynced for items 2, 3, 4.
+  {
+    WriteTransaction wtrans(FROM_HERE, UNITTEST, dir);
+    MutableEntry A(&wtrans, GET_BY_ID, ids_.FromNumber(1));
+    ASSERT_TRUE(A.good());
+    A.Put(SPECIFICS, modified_bookmark);
+    A.Put(NON_UNIQUE_NAME, kEncryptedString);
+    A.Put(IS_UNSYNCED, true);
+    MutableEntry B(&wtrans, GET_BY_ID, ids_.FromNumber(2));
+    ASSERT_TRUE(B.good());
+    B.Put(SPECIFICS, modified_bookmark);
+    B.Put(NON_UNIQUE_NAME, kEncryptedString);
+    B.Put(IS_UNSYNCED, true);
+    MutableEntry C(&wtrans, GET_BY_ID, ids_.FromNumber(3));
+    ASSERT_TRUE(C.good());
+    C.Put(SPECIFICS, modified_bookmark);
+    C.Put(NON_UNIQUE_NAME, kEncryptedString);
+    C.Put(IS_UNSYNCED, true);
+    MutableEntry D(&wtrans, GET_BY_ID, ids_.FromNumber(4));
+    ASSERT_TRUE(D.good());
+    D.Put(SPECIFICS, modified_pref);
+    D.Put(NON_UNIQUE_NAME, kEncryptedString);
+    D.Put(IS_UNSYNCED, true);
+  }
+  SyncShareAsDelegate();
+  {
+    EXPECT_EQ(0U, session_->status_controller().unsynced_handles().size());
+    // Item 1 remains unsynced due to there being pending keys.
+    // Items 2, 3, 4 should remain unsynced since they were not up to date.
+    ReadTransaction rtrans(FROM_HERE, dir);
+    VERIFY_ENTRY(1, false, true, false, 0, 40, 40, ids_, &rtrans);
+    VERIFY_ENTRY(2, true, true, true, 1, 10, 30, ids_, &rtrans);
+    VERIFY_ENTRY(3, true, true, false, 1, 10, 30, ids_, &rtrans);
+    VERIFY_ENTRY(4, true, true, true, 0, 10, 30, ids_, &rtrans);
+  }
+  {
+    ReadTransaction rtrans(FROM_HERE, dir);
+    // Resolve the pending keys.
+    syncdb_.manager()->GetCryptographer(&rtrans)->DecryptPendingKeys(
+        key_params);
+  }
+  // First cycle resolves conflicts, second cycle commits changes.
+  SyncShareAsDelegate();
+  EXPECT_EQ(2, session_->status_controller().syncer_status().
+      num_server_overwrites);
+  EXPECT_EQ(1, session_->status_controller().syncer_status().
+      num_local_overwrites);
+  // We attempted to commit item 1.
+  EXPECT_EQ(1U, session_->status_controller().unsynced_handles().size());
+  EXPECT_TRUE(session_->status_controller().did_commit_items());
+  SyncShareAsDelegate();
+  {
+    // Everything should be resolved now. The local changes should have
+    // overwritten the server changes for 2 and 4, while the server changes
+    // overwrote the local for entry 3.
+    // We attempted to commit two handles.
+    EXPECT_EQ(0, session_->status_controller().syncer_status().
+        num_server_overwrites);
+    EXPECT_EQ(0, session_->status_controller().syncer_status().
+        num_local_overwrites);
+    EXPECT_EQ(2U, session_->status_controller().unsynced_handles().size());
+    EXPECT_TRUE(session_->status_controller().did_commit_items());
+    ReadTransaction rtrans(FROM_HERE, dir);
+    VERIFY_ENTRY(1, false, false, false, 0, 41, 41, ids_, &rtrans);
+    VERIFY_ENTRY(2, false, false, false, 1, 31, 31, ids_, &rtrans);
+    VERIFY_ENTRY(3, false, false, false, 1, 30, 30, ids_, &rtrans);
+    VERIFY_ENTRY(4, false, false, false, 0, 31, 31, ids_, &rtrans);
+  }
+}
+
 TEST_F(SyncerTest, NigoriConflicts) {
   ScopedDirLookup dir(syncdb_.manager(), syncdb_.name());
   ASSERT_TRUE(dir.good());
   KeyParams local_key_params = {"localhost", "dummy", "blargle"};
   KeyParams other_key_params = {"localhost", "dummy", "foobar"};
   browser_sync::Cryptographer other_cryptographer;
   other_cryptographer.AddKey(other_key_params);
   syncable::ModelTypeSet encrypted_types(syncable::PASSWORDS, syncable::NIGORI);
   sync_pb::EntitySpecifics initial_nigori_specifics;
   initial_nigori_specifics.MutableExtension(sync_pb::nigori);
@@ skipping 102 matching lines @@
         our_encrypted_specifics.encrypted()));
     EXPECT_FALSE(syncdb_.manager()->GetCryptographer(&wtrans)->
         CanDecryptUsingDefaultKey(our_encrypted_specifics.encrypted()));
     EXPECT_TRUE(syncdb_.manager()->GetCryptographer(&wtrans)->CanDecrypt(
         other_encrypted_specifics.encrypted()));
     EXPECT_TRUE(syncdb_.manager()->GetCryptographer(&wtrans)->
         CanDecryptUsingDefaultKey(other_encrypted_specifics.encrypted()));
   }
 }
 
+#undef VERIFY_ENTRY
 
 // TODO(chron): More corner case unit tests around validation.
 TEST_F(SyncerTest, TestCommitMetahandleIterator) {
   ScopedDirLookup dir(syncdb_.manager(), syncdb_.name());
   ASSERT_TRUE(dir.good());
   StatusController* status = session_->mutable_status_controller();
   const vector<int64>& unsynced(status->unsynced_handles());
 
   {
     WriteTransaction wtrans(FROM_HERE, UNITTEST, dir);
@@ skipping 4050 matching lines @@
     EXPECT_GE(0, Get(metahandle_, BASE_VERSION));
     EXPECT_GE(0, Get(metahandle_, SERVER_VERSION));
   }
 
  protected:
   const std::string client_tag_;
   int64 metahandle_;
 };
 
 TEST_F(SyncerUndeletionTest, UndeleteDuringCommit) {
-  const StatusController& status = session_->status_controller();
-
   Create();
   ExpectUnsyncedCreation();
   SyncShareAsDelegate();
 
   EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests());
-  EXPECT_EQ(0, status.TotalNumConflictingItems());
+  EXPECT_EQ(0, session_->status_controller().TotalNumConflictingItems());
   ExpectSyncedAndCreated();
 
   // Delete, begin committing the delete, then undelete while committing.
   Delete();
   ExpectUnsyncedDeletion();
   mock_server_->SetMidCommitCallback(
       NewCallback<SyncerUndeletionTest>(this,
           &SyncerUndeletionTest::Undelete));
   SyncShareAsDelegate();
 
   // The item ought to exist as an unsynced undeletion (meaning,
   // we think that the next commit ought to be a recreation commit).
-  EXPECT_EQ(0, status.TotalNumConflictingItems());
+  EXPECT_EQ(0, session_->status_controller().TotalNumConflictingItems());
   EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests());
   ExpectUnsyncedUndeletion();
 
   // Now, encounter a GetUpdates corresponding to the deletion from
   // the server.  The undeletion should prevail again and be committed.
   // None of this should trigger any conflict detection -- it is perfectly
   // normal to recieve updates from our own commits.
   mock_server_->SetMidCommitCallback(NULL);
   mock_server_->AddUpdateTombstone(Get(metahandle_, ID));
   SyncShareAsDelegate();
-  EXPECT_EQ(0, status.TotalNumConflictingItems());
+  EXPECT_EQ(0, session_->status_controller().TotalNumConflictingItems());
   EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests());
   ExpectSyncedAndCreated();
 }
 
 TEST_F(SyncerUndeletionTest, UndeleteBeforeCommit) {
-  const StatusController& status = session_->status_controller();
-
   Create();
   ExpectUnsyncedCreation();
   SyncShareAsDelegate();
 
   EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests());
-  EXPECT_EQ(0, status.TotalNumConflictingItems());
+  EXPECT_EQ(0, session_->status_controller().TotalNumConflictingItems());
   ExpectSyncedAndCreated();
 
   // Delete and undelete, then sync to pick up the result.
   Delete();
   ExpectUnsyncedDeletion();
   Undelete();
   ExpectUnsyncedEdit();  // Edit, not undelete: server thinks it exists.
   SyncShareAsDelegate();
 
   // The item ought to have committed successfully.
-  EXPECT_EQ(0, status.TotalNumConflictingItems());
+  EXPECT_EQ(0, session_->status_controller().TotalNumConflictingItems());
   EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests());
   ExpectSyncedAndCreated();
   EXPECT_EQ(2, Get(metahandle_, BASE_VERSION));
 
   // Now, encounter a GetUpdates corresponding to the just-committed
   // update.
   mock_server_->AddUpdateFromLastCommit();
   SyncShareAsDelegate();
-  EXPECT_EQ(0, status.TotalNumConflictingItems());
+  EXPECT_EQ(0, session_->status_controller().TotalNumConflictingItems());
   EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests());
   ExpectSyncedAndCreated();
 }
 
 TEST_F(SyncerUndeletionTest, UndeleteAfterCommitButBeforeGetUpdates) {
-  const StatusController& status = session_->status_controller();
-
   Create();
   ExpectUnsyncedCreation();
   SyncShareAsDelegate();
 
   EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests());
-  EXPECT_EQ(0, status.TotalNumConflictingItems());
+  EXPECT_EQ(0, session_->status_controller().TotalNumConflictingItems());
   ExpectSyncedAndCreated();
 
   // Delete and commit.
   Delete();
   ExpectUnsyncedDeletion();
   SyncShareAsDelegate();
 
   // The item ought to have committed successfully.
-  EXPECT_EQ(0, status.TotalNumConflictingItems());
+  EXPECT_EQ(0, session_->status_controller().TotalNumConflictingItems());
   EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests());
   ExpectSyncedAndDeleted();
 
   // Before the GetUpdates, the item is locally undeleted.
   Undelete();
   ExpectUnsyncedUndeletion();
 
   // Now, encounter a GetUpdates corresponding to the just-committed
   // deletion update.  The undeletion should prevail.
   mock_server_->AddUpdateFromLastCommit();
   SyncShareAsDelegate();
-  EXPECT_EQ(0, status.TotalNumConflictingItems());
+  EXPECT_EQ(0, session_->status_controller().TotalNumConflictingItems());
   EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests());
   ExpectSyncedAndCreated();
 }
 
 TEST_F(SyncerUndeletionTest, UndeleteAfterDeleteAndGetUpdates) {
-  const StatusController& status = session_->status_controller();
-
   Create();
   ExpectUnsyncedCreation();
   SyncShareAsDelegate();
 
   EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests());
-  EXPECT_EQ(0, status.TotalNumConflictingItems());
+  EXPECT_EQ(0, session_->status_controller().TotalNumConflictingItems());
   ExpectSyncedAndCreated();
 
   mock_server_->AddUpdateFromLastCommit();
   SyncShareAsDelegate();
   EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests());
-  EXPECT_EQ(0, status.TotalNumConflictingItems());
+  EXPECT_EQ(0, session_->status_controller().TotalNumConflictingItems());
   ExpectSyncedAndCreated();
 
   // Delete and commit.
   Delete();
   ExpectUnsyncedDeletion();
   SyncShareAsDelegate();
 
   // The item ought to have committed successfully.
-  EXPECT_EQ(0, status.TotalNumConflictingItems());
+  EXPECT_EQ(0, session_->status_controller().TotalNumConflictingItems());
   EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests());
   ExpectSyncedAndDeleted();
 
   // Now, encounter a GetUpdates corresponding to the just-committed
   // deletion update.  Should be consistent.
   mock_server_->AddUpdateFromLastCommit();
   SyncShareAsDelegate();
-  EXPECT_EQ(0, status.TotalNumConflictingItems());
+  EXPECT_EQ(0, session_->status_controller().TotalNumConflictingItems());
   EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests());
   ExpectSyncedAndDeleted();
 
   // After the GetUpdates, the item is locally undeleted.
   Undelete();
   ExpectUnsyncedUndeletion();
 
   // Now, encounter a GetUpdates corresponding to the just-committed
   // deletion update.  The undeletion should prevail.
   SyncShareAsDelegate();
-  EXPECT_EQ(0, status.TotalNumConflictingItems());
+  EXPECT_EQ(0, session_->status_controller().TotalNumConflictingItems());
   EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests());
   ExpectSyncedAndCreated();
 }
 
 // Test processing of undeletion GetUpdateses.
 TEST_F(SyncerUndeletionTest, UndeleteAfterOtherClientDeletes) {
-  const StatusController& status = session_->status_controller();
-
   Create();
   ExpectUnsyncedCreation();
   SyncShareAsDelegate();
 
   EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests());
-  EXPECT_EQ(0, status.TotalNumConflictingItems());
+  EXPECT_EQ(0, session_->status_controller().TotalNumConflictingItems());
   ExpectSyncedAndCreated();
 
   // Add a delete from the server.
   mock_server_->AddUpdateFromLastCommit();
   SyncShareAsDelegate();
   EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests());
-  EXPECT_EQ(0, status.TotalNumConflictingItems());
+  EXPECT_EQ(0, session_->status_controller().TotalNumConflictingItems());
   ExpectSyncedAndCreated();
 
   // Some other client deletes the item.
   mock_server_->AddUpdateTombstone(Get(metahandle_, ID));
   SyncShareAsDelegate();
 
   // The update ought to have applied successfully.
-  EXPECT_EQ(0, status.TotalNumConflictingItems());
+  EXPECT_EQ(0, session_->status_controller().TotalNumConflictingItems());
   EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests());
   ExpectSyncedAndDeleted();
 
   // Undelete it locally.
   Undelete();
   ExpectUnsyncedUndeletion();
   SyncShareAsDelegate();
-  EXPECT_EQ(0, status.TotalNumConflictingItems());
+  EXPECT_EQ(0, session_->status_controller().TotalNumConflictingItems());
   EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests());
   ExpectSyncedAndCreated();
 
   // Now, encounter a GetUpdates corresponding to the just-committed
   // deletion update.  The undeletion should prevail.
   mock_server_->AddUpdateFromLastCommit();
   SyncShareAsDelegate();
-  EXPECT_EQ(0, status.TotalNumConflictingItems());
+  EXPECT_EQ(0, session_->status_controller().TotalNumConflictingItems());
   EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests());
   ExpectSyncedAndCreated();
 }
 
 TEST_F(SyncerUndeletionTest, UndeleteAfterOtherClientDeletesImmediately) {
-  const StatusController& status = session_->status_controller();
-
   Create();
   ExpectUnsyncedCreation();
   SyncShareAsDelegate();
 
   EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests());
-  EXPECT_EQ(0, status.TotalNumConflictingItems());
+  EXPECT_EQ(0, session_->status_controller().TotalNumConflictingItems());
   ExpectSyncedAndCreated();
 
   // Some other client deletes the item before we get a chance
   // to GetUpdates our original request.
   mock_server_->AddUpdateTombstone(Get(metahandle_, ID));
   SyncShareAsDelegate();
 
   // The update ought to have applied successfully.
-  EXPECT_EQ(0, status.TotalNumConflictingItems());
+  EXPECT_EQ(0, session_->status_controller().TotalNumConflictingItems());
   EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests());
   ExpectSyncedAndDeleted();
 
   // Undelete it locally.
   Undelete();
   ExpectUnsyncedUndeletion();
   SyncShareAsDelegate();
-  EXPECT_EQ(0, status.TotalNumConflictingItems());
+  EXPECT_EQ(0, session_->status_controller().TotalNumConflictingItems());
   EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests());
   ExpectSyncedAndCreated();
 
   // Now, encounter a GetUpdates corresponding to the just-committed
   // deletion update.  The undeletion should prevail.
   mock_server_->AddUpdateFromLastCommit();
   SyncShareAsDelegate();
-  EXPECT_EQ(0, status.TotalNumConflictingItems());
+  EXPECT_EQ(0, session_->status_controller().TotalNumConflictingItems());
   EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests());
   ExpectSyncedAndCreated();
 }
 
 TEST_F(SyncerUndeletionTest, OtherClientUndeletes) {
-  const StatusController& status = session_->status_controller();
-
   Create();
   ExpectUnsyncedCreation();
   SyncShareAsDelegate();
 
   EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests());
-  EXPECT_EQ(0, status.TotalNumConflictingItems());
+  EXPECT_EQ(0, session_->status_controller().TotalNumConflictingItems());
   ExpectSyncedAndCreated();
 
   // Get the updates of our just-committed entry.
   mock_server_->AddUpdateFromLastCommit();
   SyncShareAsDelegate();
   EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests());
-  EXPECT_EQ(0, status.TotalNumConflictingItems());
+  EXPECT_EQ(0, session_->status_controller().TotalNumConflictingItems());
   ExpectSyncedAndCreated();
 
   // We delete the item.
   Delete();
   ExpectUnsyncedDeletion();
   SyncShareAsDelegate();
 
   // The update ought to have applied successfully.
-  EXPECT_EQ(0, status.TotalNumConflictingItems());
+  EXPECT_EQ(0, session_->status_controller().TotalNumConflictingItems());
   EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests());
   ExpectSyncedAndDeleted();
 
   // Now, encounter a GetUpdates corresponding to the just-committed
   // deletion update.
   mock_server_->AddUpdateFromLastCommit();
   SyncShareAsDelegate();
-  EXPECT_EQ(0, status.TotalNumConflictingItems());
+  EXPECT_EQ(0, session_->status_controller().TotalNumConflictingItems());
   EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests());
   ExpectSyncedAndDeleted();
 
   // Some other client undeletes the item.
   mock_server_->AddUpdateBookmark(Get(metahandle_, ID),
                                   Get(metahandle_, PARENT_ID),
                                   "Thadeusz", 100, 1000);
   mock_server_->SetLastUpdateClientTag(client_tag_);
   SyncShareAsDelegate();
-  EXPECT_EQ(0, status.TotalNumConflictingItems());
+  EXPECT_EQ(0, session_->status_controller().TotalNumConflictingItems());
   EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests());
   ExpectSyncedAndCreated();
   EXPECT_EQ("Thadeusz", Get(metahandle_, NON_UNIQUE_NAME));
 }
 
 TEST_F(SyncerUndeletionTest, OtherClientUndeletesImmediately) {
-  const StatusController& status = session_->status_controller();
-
   Create();
   ExpectUnsyncedCreation();
   SyncShareAsDelegate();
 
   EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests());
-  EXPECT_EQ(0, status.TotalNumConflictingItems());
+  EXPECT_EQ(0, session_->status_controller().TotalNumConflictingItems());
   ExpectSyncedAndCreated();
 
   // Get the updates of our just-committed entry.
   mock_server_->AddUpdateFromLastCommit();
   SyncShareAsDelegate();
   EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests());
-  EXPECT_EQ(0, status.TotalNumConflictingItems());
+  EXPECT_EQ(0, session_->status_controller().TotalNumConflictingItems());
   ExpectSyncedAndCreated();
 
   // We delete the item.
   Delete();
   ExpectUnsyncedDeletion();
   SyncShareAsDelegate();
 
   // The update ought to have applied successfully.
-  EXPECT_EQ(0, status.TotalNumConflictingItems());
+  EXPECT_EQ(0, session_->status_controller().TotalNumConflictingItems());
   EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests());
   ExpectSyncedAndDeleted();
 
   // Some other client undeletes before we see the update from our
   // commit.
   mock_server_->AddUpdateBookmark(Get(metahandle_, ID),
                                   Get(metahandle_, PARENT_ID),
                                   "Thadeusz", 100, 1000);
   mock_server_->SetLastUpdateClientTag(client_tag_);
   SyncShareAsDelegate();
-  EXPECT_EQ(0, status.TotalNumConflictingItems());
+  EXPECT_EQ(0, session_->status_controller().TotalNumConflictingItems());
   EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests());
   ExpectSyncedAndCreated();
   EXPECT_EQ("Thadeusz", Get(metahandle_, NON_UNIQUE_NAME));
 }
 
 // A group of tests exercising the syncer's handling of sibling ordering, as
 // represented in the sync protocol.
 class SyncerPositionUpdateTest : public SyncerTest {
  public:
   SyncerPositionUpdateTest() : next_update_id_(1), next_revision_(1) {}
@@ skipping 207 matching lines @@
   Add(low_id_);
   Add(high_id_);
   SyncShareAsDelegate();
   ExpectLocalOrderIsByServerId();
 }
 
 const SyncerTest::CommitOrderingTest
 SyncerTest::CommitOrderingTest::LAST_COMMIT_ITEM = {-1, TestIdFactory::root()};
 
 }  // namespace browser_sync