sync: Process updates responses on the sync thread

Convert ProcessUpdatesCommand from a ModelChangingSyncerCommand to a
SyncerCommand.  This means it will be executed only on the syncer
thread, rather than delegating to a collection of model threads.

In order to ensure this change is safe and will not be broken by future
CLs, it converts all transactions used by the ProcessUpdatesCommand into
ModelNeutralWriteTransactions.  When implemented correctly, this
guarantees that no change performed by this command will need to be
forwarded to the model threads.

This commit adds a constructor to ModelNeutralMutableEntry to allow it
to instantiate "update" items.  This is safe because these update items
can be saved and manipulated inside the sync directory as long as they
remain locally IS_DEL.  The native models do not need to be made aware
of them until this flag is unset when we apply the updates in a later
SyncerCommand.

BUG=299023

Review URL: https://codereview.chromium.org/24879002

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

12 files changed, 105 insertions, 139 deletions

diff --git a/sync/engine/process_commit_response_command.cc b/sync/engine/process_commit_response_command.cc
index 23a9c7e..97f403f 100644
--- a/sync/engine/process_commit_response_command.cc
+++ b/sync/engine/process_commit_response_command.cc
@@ -109,9 +109,9 @@ SyncerError ProcessCommitResponseCommand::ExecuteImpl(SyncSession* session) {
           LOG(FATAL) << "Bad return from ProcessSingleCommitResponse";
       }
     }
-  }
 
-  MarkDeletedChildrenSynced(dir, &deleted_folders);
+    MarkDeletedChildrenSynced(dir, &trans, &deleted_folders);
+  }
 
   int commit_count = static_cast<int>(commit_set_.Size());
   if (commit_count == successes) {
diff --git a/sync/engine/process_updates_command.cc b/sync/engine/process_updates_command.cc
index 4854852..d761cfc 100644
--- a/sync/engine/process_updates_command.cc
+++ b/sync/engine/process_updates_command.cc
@@ -13,10 +13,10 @@
 #include "sync/engine/syncer_util.h"
 #include "sync/sessions/sync_session.h"
 #include "sync/syncable/directory.h"
-#include "sync/syncable/mutable_entry.h"
+#include "sync/syncable/model_neutral_mutable_entry.h"
+#include "sync/syncable/syncable_model_neutral_write_transaction.h"
 #include "sync/syncable/syncable_proto_util.h"
 #include "sync/syncable/syncable_util.h"
-#include "sync/syncable/syncable_write_transaction.h"
 #include "sync/util/cryptographer.h"
 
 using std::vector;
@@ -31,21 +31,6 @@ using syncable::GET_BY_ID;
 ProcessUpdatesCommand::ProcessUpdatesCommand() {}
 ProcessUpdatesCommand::~ProcessUpdatesCommand() {}
 
-std::set<ModelSafeGroup> ProcessUpdatesCommand::GetGroupsToChange(
-    const sessions::SyncSession& session) const {
-  std::set<ModelSafeGroup> groups_with_updates;
-
-  const sync_pb::GetUpdatesResponse& updates =
-      session.status_controller().updates_response().get_updates();
-  for (int i = 0; i < updates.entries().size(); i++) {
-    groups_with_updates.insert(
-        GetGroupForModelType(GetModelType(updates.entries(i)),
-                             session.context()->routing_info()));
-  }
-
-  return groups_with_updates;
-}
-
 namespace {
 
 // This function attempts to determine whether or not this update is genuinely
@@ -102,11 +87,11 @@ bool UpdateContainsNewVersion(syncable::BaseTransaction *trans,
 
 }  // namespace
 
-SyncerError ProcessUpdatesCommand::ModelChangingExecuteImpl(
-    SyncSession* session) {
+SyncerError ProcessUpdatesCommand::ExecuteImpl(SyncSession* session) {
   syncable::Directory* dir = session->context()->directory();
 
-  syncable::WriteTransaction trans(FROM_HERE, syncable::SYNCER, dir);
+  syncable::ModelNeutralWriteTransaction trans(
+      FROM_HERE, syncable::SYNCER, dir);
 
   sessions::StatusController* status = session->mutable_status_controller();
   const sync_pb::GetUpdatesResponse& updates =
@@ -120,18 +105,6 @@ SyncerError ProcessUpdatesCommand::ModelChangingExecuteImpl(
   for (int i = 0; i < update_count; i++) {
     const sync_pb::SyncEntity& update = updates.entries(i);
 
-    // The current function gets executed on several different threads, but
-    // every call iterates over the same list of items that the server returned
-    // to us.  We're not allowed to process items unless we're on the right
-    // thread for that type.  This check will ensure we only touch the items
-    // that live on our current thread.
-    // TODO(tim): Don't allow access to objects in other ModelSafeGroups.
-    // See crbug.com/121521 .
-    ModelSafeGroup g = GetGroupForModelType(GetModelType(update),
-                                            session->context()->routing_info());
-    if (g != status->group_restriction())
-      continue;
-
     VerifyResult verify_result = VerifyUpdate(
         &trans, update, requested_types, session->context()->routing_info());
     status->increment_num_updates_downloaded_by(1);
@@ -159,8 +132,9 @@ namespace {
 // will have refused to unify the update.
 // We should not attempt to apply it at all since it violates consistency
 // rules.
-VerifyResult VerifyTagConsistency(const sync_pb::SyncEntity& entry,
-                                  const syncable::MutableEntry& same_id) {
+VerifyResult VerifyTagConsistency(
+    const sync_pb::SyncEntity& entry,
+    const syncable::ModelNeutralMutableEntry& same_id) {
   if (entry.has_client_defined_unique_tag() &&
       entry.client_defined_unique_tag() !=
           same_id.GetUniqueClientTag()) {
@@ -172,7 +146,8 @@ VerifyResult VerifyTagConsistency(const sync_pb::SyncEntity& entry,
 }  // namespace
 
 VerifyResult ProcessUpdatesCommand::VerifyUpdate(
-    syncable::WriteTransaction* trans, const sync_pb::SyncEntity& entry,
+    syncable::ModelNeutralWriteTransaction* trans,
+    const sync_pb::SyncEntity& entry,
     ModelTypeSet requested_types,
     const ModelSafeRoutingInfo& routes) {
   syncable::Id id = SyncableIdFromProto(entry.id_string());
@@ -194,7 +169,7 @@ VerifyResult ProcessUpdatesCommand::VerifyUpdate(
     }
   }
 
-  syncable::MutableEntry same_id(trans, GET_BY_ID, id);
+  syncable::ModelNeutralMutableEntry same_id(trans, GET_BY_ID, id);
   result = VerifyNewEntry(entry, &same_id, deleted);
 
   ModelType placement_type = !deleted ? GetModelType(entry)
@@ -220,8 +195,8 @@ VerifyResult ProcessUpdatesCommand::VerifyUpdate(
   // If we have an existing entry, we check here for updates that break
   // consistency rules.
   if (VERIFY_UNDECIDED == result) {
-    result = VerifyUpdateConsistency(trans, entry, &same_id,
-                                     deleted, is_directory, model_type);
+    result = VerifyUpdateConsistency(trans, entry, deleted,
+                                     is_directory, model_type, &same_id);
   }
 
   if (VERIFY_UNDECIDED == result)
@@ -232,9 +207,9 @@ VerifyResult ProcessUpdatesCommand::VerifyUpdate(
 
 namespace {
 // Returns true if the entry is still ok to process.
-bool ReverifyEntry(syncable::WriteTransaction* trans,
+bool ReverifyEntry(syncable::ModelNeutralWriteTransaction* trans,
                    const sync_pb::SyncEntity& entry,
-                   syncable::MutableEntry* same_id) {
+                   syncable::ModelNeutralMutableEntry* same_id) {
 
   const bool deleted = entry.has_deleted() && entry.deleted();
   const bool is_directory = IsFolder(entry);
@@ -242,10 +217,10 @@ bool ReverifyEntry(syncable::WriteTransaction* trans,
 
   return VERIFY_SUCCESS == VerifyUpdateConsistency(trans,
                                                    entry,
-                                                   same_id,
                                                    deleted,
                                                    is_directory,
-                                                   model_type);
+                                                   model_type,
+                                                   same_id);
 }
 }  // namespace
 
@@ -253,7 +228,7 @@ bool ReverifyEntry(syncable::WriteTransaction* trans,
 ServerUpdateProcessingResult ProcessUpdatesCommand::ProcessUpdate(
     const sync_pb::SyncEntity& update,
     const Cryptographer* cryptographer,
-    syncable::WriteTransaction* const trans) {
+    syncable::ModelNeutralWriteTransaction* const trans) {
   const syncable::Id& server_id = SyncableIdFromProto(update.id_string());
   const std::string name = SyncerProtoUtil::NameFromSyncEntity(update);
 
@@ -270,7 +245,7 @@ ServerUpdateProcessingResult ProcessUpdatesCommand::ProcessUpdate(
 
   // We take a two step approach. First we store the entries data in the
   // server fields of a local entry and then move the data to the local fields
-  syncable::MutableEntry target_entry(trans, GET_BY_ID, local_id);
+  syncable::ModelNeutralMutableEntry target_entry(trans, GET_BY_ID, local_id);
 
   // We need to run the Verify checks again; the world could have changed
   // since we last verified.
diff --git a/sync/engine/process_updates_command.h b/sync/engine/process_updates_command.h
index ce79387..0868e98 100644
--- a/sync/engine/process_updates_command.h
+++ b/sync/engine/process_updates_command.h
@@ -17,7 +17,7 @@ class SyncEntity;
 namespace syncer {
 
 namespace syncable {
-class WriteTransaction;
+class ModelNeutralWriteTransaction;
 }
 
 class Cryptographer;
@@ -28,29 +28,25 @@ class Cryptographer;
 //
 // Postconditions - All of the verified SyncEntity data will be copied to
 //                  the server fields of the corresponding syncable entries.
-class SYNC_EXPORT_PRIVATE ProcessUpdatesCommand
-    : public ModelChangingSyncerCommand {
+class SYNC_EXPORT_PRIVATE ProcessUpdatesCommand : public SyncerCommand {
  public:
   ProcessUpdatesCommand();
   virtual ~ProcessUpdatesCommand();
 
  protected:
-  // ModelChangingSyncerCommand implementation.
-  virtual std::set<ModelSafeGroup> GetGroupsToChange(
-      const sessions::SyncSession& session) const OVERRIDE;
-  virtual SyncerError ModelChangingExecuteImpl(
-      sessions::SyncSession* session) OVERRIDE;
+  // SyncerCommand implementation.
+  virtual SyncerError ExecuteImpl(sessions::SyncSession* session) OVERRIDE;
 
  private:
   VerifyResult VerifyUpdate(
-      syncable::WriteTransaction* trans,
+      syncable::ModelNeutralWriteTransaction* trans,
       const sync_pb::SyncEntity& entry,
       ModelTypeSet requested_types,
       const ModelSafeRoutingInfo& routes);
   ServerUpdateProcessingResult ProcessUpdate(
       const sync_pb::SyncEntity& proto_update,
       const Cryptographer* cryptographer,
-      syncable::WriteTransaction* const trans);
+      syncable::ModelNeutralWriteTransaction* const trans);
   DISALLOW_COPY_AND_ASSIGN(ProcessUpdatesCommand);
 };
 
diff --git a/sync/engine/process_updates_command_unittest.cc b/sync/engine/process_updates_command_unittest.cc
index b46dc78..44b308a 100644
--- a/sync/engine/process_updates_command_unittest.cc
+++ b/sync/engine/process_updates_command_unittest.cc
@@ -81,25 +81,6 @@ class ProcessUpdatesCommandTest : public SyncerCommandTest {
   DISALLOW_COPY_AND_ASSIGN(ProcessUpdatesCommandTest);
 };
 
-TEST_F(ProcessUpdatesCommandTest, GroupsToChange) {
-  std::string root = syncable::GetNullId().GetServerId();
-
-  CreateLocalItem("p1", root, PREFERENCES);
-  CreateLocalItem("a1", root, AUTOFILL);
-
-  ExpectNoGroupsToChange(command_);
-
-  sync_pb::GetUpdatesResponse* updates =
-      session()->mutable_status_controller()->
-      mutable_updates_response()->mutable_get_updates();
-  AddUpdate(updates, "p1", root, PREFERENCES);
-  AddUpdate(updates, "a1", root, AUTOFILL);
-
-  ExpectGroupsToChange(command_, GROUP_UI, GROUP_DB);
-
-  command_.ExecuteImpl(session());
-}
-
 static const char kCacheGuid[] = "IrcjZ2jyzHDV9Io4+zKcXQ==";
 
 // Test that the bookmark tag is set on newly downloaded items.
@@ -118,7 +99,7 @@ TEST_F(ProcessUpdatesCommandTest, NewBookmarkTag) {
   e->set_originator_client_item_id(client_id.GetServerId());
   e->set_position_in_parent(0);
 
-  command_.ExecuteImpl(session());
+  command_.Execute(session());
 
   syncable::ReadTransaction trans(FROM_HERE, directory());
   syncable::Entry entry(&trans, syncable::GET_BY_ID, server_id);
@@ -150,7 +131,7 @@ TEST_F(ProcessUpdatesCommandTest, ReceiveServerCreatedBookmarkFolders) {
 
   EXPECT_FALSE(SyncerProtoUtil::ShouldMaintainPosition(*e));
 
-  command_.ExecuteImpl(session());
+  command_.Execute(session());
 
   syncable::ReadTransaction trans(FROM_HERE, directory());
   syncable::Entry entry(&trans, syncable::GET_BY_ID, server_id);
@@ -175,7 +156,7 @@ TEST_F(ProcessUpdatesCommandTest, ReceiveNonBookmarkItem) {
 
   EXPECT_FALSE(SyncerProtoUtil::ShouldMaintainPosition(*e));
 
-  command_.ExecuteImpl(session());
+  command_.Execute(session());
 
   syncable::ReadTransaction trans(FROM_HERE, directory());
   syncable::Entry entry(&trans, syncable::GET_BY_ID, server_id);
diff --git a/sync/engine/syncer_util.cc b/sync/engine/syncer_util.cc
index 3960baa..2235734 100644
--- a/sync/engine/syncer_util.cc
+++ b/sync/engine/syncer_util.cc
@@ -23,8 +23,10 @@
 #include "sync/protocol/sync.pb.h"
 #include "sync/syncable/directory.h"
 #include "sync/syncable/entry.h"
+#include "sync/syncable/model_neutral_mutable_entry.h"
 #include "sync/syncable/mutable_entry.h"
 #include "sync/syncable/syncable_changes_version.h"
+#include "sync/syncable/syncable_model_neutral_write_transaction.h"
 #include "sync/syncable/syncable_proto_util.h"
 #include "sync/syncable/syncable_read_transaction.h"
 #include "sync/syncable/syncable_util.h"
@@ -305,7 +307,7 @@ namespace {
 void UpdateBookmarkSpecifics(const std::string& singleton_tag,
                              const std::string& url,
                              const std::string& favicon_bytes,
-                             MutableEntry* local_entry) {
+                             syncable::ModelNeutralMutableEntry* local_entry) {
   // In the new-style protocol, the server no longer sends bookmark info for
   // the "google_chrome" folder.  Mimic that here.
   if (singleton_tag == "google_chrome")
@@ -319,8 +321,9 @@ void UpdateBookmarkSpecifics(const std::string& singleton_tag,
   local_entry->PutServerSpecifics(pb);
 }
 
-void UpdateBookmarkPositioning(const sync_pb::SyncEntity& update,
-                               MutableEntry* local_entry) {
+void UpdateBookmarkPositioning(
+    const sync_pb::SyncEntity& update,
+    syncable::ModelNeutralMutableEntry* local_entry) {
   // Update our unique bookmark tag.  In many cases this will be identical to
   // the tag we already have.  However, clients that have recently upgraded to
   // versions that support unique positions will have incorrect tags.  See the
@@ -348,7 +351,7 @@ void UpdateBookmarkPositioning(const sync_pb::SyncEntity& update,
 }  // namespace
 
 void UpdateServerFieldsFromUpdate(
-    MutableEntry* target,
+    syncable::ModelNeutralMutableEntry* target,
     const sync_pb::SyncEntity& update,
     const std::string& name) {
   if (update.deleted()) {
@@ -418,12 +421,14 @@ void UpdateServerFieldsFromUpdate(
 }
 
 // Creates a new Entry iff no Entry exists with the given id.
-void CreateNewEntry(syncable::WriteTransaction *trans,
+void CreateNewEntry(syncable::ModelNeutralWriteTransaction *trans,
                     const syncable::Id& id) {
-  syncable::MutableEntry entry(trans, GET_BY_ID, id);
+  syncable::Entry entry(trans, GET_BY_ID, id);
   if (!entry.good()) {
-    syncable::MutableEntry new_entry(trans, syncable::CREATE_NEW_UPDATE_ITEM,
-                                     id);
+    syncable::ModelNeutralMutableEntry new_entry(
+        trans,
+        syncable::CREATE_NEW_UPDATE_ITEM,
+        id);
   }
 }
 
@@ -481,6 +486,7 @@ VerifyCommitResult ValidateCommitEntry(syncable::Entry* entry) {
 
 void MarkDeletedChildrenSynced(
     syncable::Directory* dir,
+    syncable::BaseWriteTransaction* trans,
     std::set<syncable::Id>* deleted_folders) {
   // There's two options here.
   // 1. Scan deleted unsynced entries looking up their pre-delete tree for any
@@ -492,27 +498,22 @@ void MarkDeletedChildrenSynced(
   if (deleted_folders->empty())
     return;
   Directory::Metahandles handles;
-  {
-    syncable::ReadTransaction trans(FROM_HERE, dir);
-    dir->GetUnsyncedMetaHandles(&trans, &handles);
-  }
+  dir->GetUnsyncedMetaHandles(trans, &handles);
   if (handles.empty())
     return;
   Directory::Metahandles::iterator it;
   for (it = handles.begin() ; it != handles.end() ; ++it) {
-    // Single transaction / entry we deal with.
-    WriteTransaction trans(FROM_HERE, SYNCER, dir);
-    MutableEntry entry(&trans, GET_BY_HANDLE, *it);
+    syncable::ModelNeutralMutableEntry entry(trans, GET_BY_HANDLE, *it);
     if (!entry.GetIsUnsynced() || !entry.GetIsDel())
       continue;
     syncable::Id id = entry.GetParentId();
-    while (id != trans.root_id()) {
+    while (id != trans->root_id()) {
       if (deleted_folders->find(id) != deleted_folders->end()) {
         // We've synced the deletion of this deleted entries parent.
         entry.PutIsUnsynced(false);
         break;
       }
-      Entry parent(&trans, GET_BY_ID, id);
+      Entry parent(trans, GET_BY_ID, id);
       if (!parent.good() || !parent.GetIsDel())
         break;
       id = parent.GetParentId();
@@ -539,12 +540,12 @@ VerifyResult VerifyNewEntry(
 // Assumes we have an existing entry; check here for updates that break
 // consistency rules.
 VerifyResult VerifyUpdateConsistency(
-    syncable::WriteTransaction* trans,
+    syncable::ModelNeutralWriteTransaction* trans,
     const sync_pb::SyncEntity& update,
-    syncable::MutableEntry* target,
     const bool deleted,
     const bool is_directory,
-    ModelType model_type) {
+    ModelType model_type,
+    syncable::ModelNeutralMutableEntry* target) {
 
   CHECK(target->good());
   const syncable::Id& update_id = SyncableIdFromProto(update.id_string());
@@ -612,9 +613,9 @@ VerifyResult VerifyUpdateConsistency(
 
 // Assumes we have an existing entry; verify an update that seems to be
 // expressing an 'undelete'
-VerifyResult VerifyUndelete(syncable::WriteTransaction* trans,
+VerifyResult VerifyUndelete(syncable::ModelNeutralWriteTransaction* trans,
                             const sync_pb::SyncEntity& update,
-                            syncable::MutableEntry* target) {
+                            syncable::ModelNeutralMutableEntry* target) {
   // TODO(nick): We hit this path for items deleted items that the server
   // tells us to re-create; only deleted items with positive base versions
   // will hit this path.  However, it's not clear how such an undeletion
diff --git a/sync/engine/syncer_util.h b/sync/engine/syncer_util.h
index 45b3b46..575ab11 100644
--- a/sync/engine/syncer_util.h
+++ b/sync/engine/syncer_util.h
@@ -27,6 +27,7 @@ namespace syncer {
 
 namespace syncable {
 class BaseTransaction;
+class ModelNeutralWriteTransaction;
 }  // namespace syncable
 
 class Cryptographer;
@@ -66,12 +67,12 @@ std::string GetUniqueBookmarkTagFromUpdate(const sync_pb::SyncEntity& update);
 
 // Pass in name to avoid redundant UTF8 conversion.
 void UpdateServerFieldsFromUpdate(
-    syncable::MutableEntry* local_entry,
+    syncable::ModelNeutralMutableEntry* local_entry,
     const sync_pb::SyncEntity& server_entry,
     const std::string& name);
 
 // Creates a new Entry iff no Entry exists with the given id.
-void CreateNewEntry(syncable::WriteTransaction *trans,
+void CreateNewEntry(syncable::ModelNeutralWriteTransaction *trans,
                     const syncable::Id& id);
 
 // This function is called on an entry when we can update the user-facing data
@@ -87,21 +88,23 @@ VerifyResult VerifyNewEntry(const sync_pb::SyncEntity& update,
 
 // Assumes we have an existing entry; check here for updates that break
 // consistency rules.
-VerifyResult VerifyUpdateConsistency(syncable::WriteTransaction* trans,
-                                     const sync_pb::SyncEntity& update,
-                                     syncable::MutableEntry* target,
-                                     const bool deleted,
-                                     const bool is_directory,
-                                     ModelType model_type);
+VerifyResult VerifyUpdateConsistency(
+    syncable::ModelNeutralWriteTransaction* trans,
+    const sync_pb::SyncEntity& update,
+    const bool deleted,
+    const bool is_directory,
+    ModelType model_type,
+    syncable::ModelNeutralMutableEntry* target);
 
 // Assumes we have an existing entry; verify an update that seems to be
 // expressing an 'undelete'
-VerifyResult VerifyUndelete(syncable::WriteTransaction* trans,
+VerifyResult VerifyUndelete(syncable::ModelNeutralWriteTransaction* trans,
                             const sync_pb::SyncEntity& update,
-                            syncable::MutableEntry* target);
+                            syncable::ModelNeutralMutableEntry* target);
 
 void MarkDeletedChildrenSynced(
     syncable::Directory* dir,
+    syncable::BaseWriteTransaction* trans,
     std::set<syncable::Id>* deleted_folders);
 
 }  // namespace syncer
diff --git a/sync/syncable/directory.cc b/sync/syncable/directory.cc
index 855d442..85e5107 100644
--- a/sync/syncable/directory.cc
+++ b/sync/syncable/directory.cc
@@ -343,12 +343,12 @@ EntryKernel* Directory::GetRootEntry() {
   return GetEntryById(Id());
 }
 
-bool Directory::InsertEntry(WriteTransaction* trans, EntryKernel* entry) {
+bool Directory::InsertEntry(BaseWriteTransaction* trans, EntryKernel* entry) {
   ScopedKernelLock lock(this);
   return InsertEntry(trans, entry, &lock);
 }
 
-bool Directory::InsertEntry(WriteTransaction* trans,
+bool Directory::InsertEntry(BaseWriteTransaction* trans,
                             EntryKernel* entry,
                             ScopedKernelLock* lock) {
   DCHECK(NULL != lock);
diff --git a/sync/syncable/directory.h b/sync/syncable/directory.h
index 5a07322..7a58dfc 100644
--- a/sync/syncable/directory.h
+++ b/sync/syncable/directory.h
@@ -507,9 +507,9 @@ class SYNC_EXPORT Directory {
   void HandleSaveChangesFailure(const SaveChangesSnapshot& snapshot);
 
   // For new entry creation only
-  bool InsertEntry(WriteTransaction* trans,
+  bool InsertEntry(BaseWriteTransaction* trans,
                    EntryKernel* entry, ScopedKernelLock* lock);
-  bool InsertEntry(WriteTransaction* trans, EntryKernel* entry);
+  bool InsertEntry(BaseWriteTransaction* trans, EntryKernel* entry);
 
   // Used by CheckTreeInvariants
   void GetAllMetaHandles(BaseTransaction* trans, MetahandleSet* result);
diff --git a/sync/syncable/model_neutral_mutable_entry.cc b/sync/syncable/model_neutral_mutable_entry.cc
index 9b054ef..d778aba 100644
--- a/sync/syncable/model_neutral_mutable_entry.cc
+++ b/sync/syncable/model_neutral_mutable_entry.cc
@@ -9,6 +9,7 @@
 #include "sync/internal_api/public/base/unique_position.h"
 #include "sync/syncable/directory.h"
 #include "sync/syncable/scoped_kernel_lock.h"
+#include "sync/syncable/syncable_changes_version.h"
 #include "sync/syncable/syncable_util.h"
 #include "sync/syncable/syncable_write_transaction.h"
 
@@ -18,6 +19,31 @@ namespace syncer {
 
 namespace syncable {
 
+ModelNeutralMutableEntry::ModelNeutralMutableEntry(BaseWriteTransaction* trans,
+                                                   CreateNewUpdateItem,
+                                                   const Id& id)
+    : Entry(trans), base_write_transaction_(trans) {
+  Entry same_id(trans, GET_BY_ID, id);
+  kernel_ = NULL;
+  if (same_id.good()) {
+    return;  // already have an item with this ID.
+  }
+  scoped_ptr<EntryKernel> kernel(new EntryKernel());
+
+  kernel->put(ID, id);
+  kernel->put(META_HANDLE, trans->directory()->NextMetahandle());
+  kernel->mark_dirty(&trans->directory()->kernel_->dirty_metahandles);
+  kernel->put(IS_DEL, true);
+  // We match the database defaults here
+  kernel->put(BASE_VERSION, CHANGES_VERSION);
+  if (!trans->directory()->InsertEntry(trans, kernel.get())) {
+    return;  // Failed inserting.
+  }
+  trans->TrackChangesTo(kernel.get());
+
+  kernel_ = kernel.release();
+}
+
 ModelNeutralMutableEntry::ModelNeutralMutableEntry(
     BaseWriteTransaction* trans, GetById, const Id& id)
     : Entry(trans, GET_BY_ID, id), base_write_transaction_(trans) {
diff --git a/sync/syncable/model_neutral_mutable_entry.h b/sync/syncable/model_neutral_mutable_entry.h
index c9f6c8b..e2292e7 100644
--- a/sync/syncable/model_neutral_mutable_entry.h
+++ b/sync/syncable/model_neutral_mutable_entry.h
@@ -16,6 +16,10 @@ namespace syncable {
 
 class BaseWriteTransaction;
 
+enum CreateNewUpdateItem {
+  CREATE_NEW_UPDATE_ITEM
+};
+
 // This Entry includes all the operations one can safely perform on the sync
 // thread.  In particular, it does not expose setters to make changes that need
 // to be communicated to the model (and the model's thread).  It is not possible
@@ -23,6 +27,9 @@ class BaseWriteTransaction;
 // entry.
 class SYNC_EXPORT_PRIVATE ModelNeutralMutableEntry : public Entry {
  public:
+  ModelNeutralMutableEntry(BaseWriteTransaction* trans,
+                           CreateNewUpdateItem,
+                           const Id& id);
   ModelNeutralMutableEntry(BaseWriteTransaction* trans, GetByHandle, int64);
   ModelNeutralMutableEntry(BaseWriteTransaction* trans, GetById, const Id&);
   ModelNeutralMutableEntry(
diff --git a/sync/syncable/mutable_entry.cc b/sync/syncable/mutable_entry.cc
index a4fadd2..863e65b 100644
--- a/sync/syncable/mutable_entry.cc
+++ b/sync/syncable/mutable_entry.cc
@@ -78,27 +78,8 @@ MutableEntry::MutableEntry(WriteTransaction* trans,
 
 MutableEntry::MutableEntry(WriteTransaction* trans, CreateNewUpdateItem,
                            const Id& id)
-    : ModelNeutralMutableEntry(trans), write_transaction_(trans) {
-  Entry same_id(trans, GET_BY_ID, id);
-  kernel_ = NULL;
-  if (same_id.good()) {
-    return;  // already have an item with this ID.
-  }
-  scoped_ptr<EntryKernel> kernel(new EntryKernel());
-
-  kernel->put(ID, id);
-  kernel->put(META_HANDLE, trans->directory_->NextMetahandle());
-  kernel->mark_dirty(&trans->directory_->kernel_->dirty_metahandles);
-  kernel->put(IS_DEL, true);
-  // We match the database defaults here
-  kernel->put(BASE_VERSION, CHANGES_VERSION);
-  if (!trans->directory()->InsertEntry(trans, kernel.get())) {
-    return;  // Failed inserting.
-  }
-  trans->TrackChangesTo(kernel.get());
-
-  kernel_ = kernel.release();
-}
+    : ModelNeutralMutableEntry(trans, CREATE_NEW_UPDATE_ITEM, id),
+      write_transaction_(trans) {}
 
 MutableEntry::MutableEntry(WriteTransaction* trans, GetById, const Id& id)
     : ModelNeutralMutableEntry(trans, GET_BY_ID, id),
diff --git a/sync/syncable/mutable_entry.h b/sync/syncable/mutable_entry.h
index 0b72bcc..8c2f2ab 100644
--- a/sync/syncable/mutable_entry.h
+++ b/sync/syncable/mutable_entry.h
@@ -20,10 +20,6 @@ enum Create {
   CREATE
 };
 
-enum CreateNewUpdateItem {
-  CREATE_NEW_UPDATE_ITEM
-};
-
 class WriteTransaction;
 
 // A mutable meta entry.  Changes get committed to the database when the