Remove some members from SyncSession

This is part of the effort to shrink the size of the SyncSession and
SyncSessionJob.

One of the members to be removed was the write transaction.  This was
stored in the session in order to share it among various functions that
prepare a commit message.  The same goal can be accomplished by passing
the transaction in to the constructors of the SyncerCommands.

There was one complication in this change.  The new constructor meant
that it was impossible to instantiate a BuildCommitCommand without a
transaction, which made it somewhat harder to unit test.  Making some of
its methods static made it unnecessary to instantiate a
BuildCommitCommand object in the test.

In addition to giving us better control of the scope of the transaction,
this change allows BuildCommitCommand and GetCommitIdsCommand to use a
BaseTransaction rather than a WriteTransaction.

The other member removed from SyncSession is the
ExtensionsActivityMonitor's records.  This member was used to buffer the
ExtensionsActivityMonitor's records which were pending for commit and to
re-add them to the monitor if the commit failed.  Since it is only used
within the context of a commit, it is safer and simpler to declare it on
the stack in BuildAndPostCommitsImpl().

The change should have no noticeable impact on behaviour, though it does
move some of the interactions with the ExtensionsActivityMonitor from
the UI thread to the sync thread.  This should be safe; the monitor's
internal data structures are protected with a lock.  The move also allows
us to remove some of the code from StatusController that was used to
trigger the UI-thread-specific processing.

BUG=175024


Review URL: https://chromiumcodereview.appspot.com/12314103

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

18 files changed, 274 insertions, 317 deletions

diff --git a/sync/engine/build_commit_command.cc b/sync/engine/build_commit_command.cc
index fb49553..ea0346c 100644
--- a/sync/engine/build_commit_command.cc
+++ b/sync/engine/build_commit_command.cc
@@ -16,10 +16,10 @@
 #include "sync/sessions/ordered_commit_set.h"
 #include "sync/sessions/sync_session.h"
 #include "sync/syncable/directory.h"
-#include "sync/syncable/mutable_entry.h"
+#include "sync/syncable/entry.h"
+#include "sync/syncable/syncable_base_transaction.h"
 #include "sync/syncable/syncable_changes_version.h"
 #include "sync/syncable/syncable_proto_util.h"
-#include "sync/syncable/syncable_write_transaction.h"
 #include "sync/util/time.h"
 
 // TODO(vishwath): Remove this include after node positions have
@@ -40,7 +40,6 @@ using syncable::SERVER_ORDINAL_IN_PARENT;
 using syncable::IS_UNAPPLIED_UPDATE;
 using syncable::IS_UNSYNCED;
 using syncable::Id;
-using syncable::MutableEntry;
 using syncable::SPECIFICS;
 
 // static
@@ -59,9 +58,14 @@ int64 BuildCommitCommand::GetGap() {
 }
 
 BuildCommitCommand::BuildCommitCommand(
+    syncable::BaseTransaction* trans,
     const sessions::OrderedCommitSet& batch_commit_set,
-    sync_pb::ClientToServerMessage* commit_message)
-  : batch_commit_set_(batch_commit_set), commit_message_(commit_message) {
+    sync_pb::ClientToServerMessage* commit_message,
+    ExtensionsActivityMonitor::Records* extensions_activity_buffer)
+  : trans_(trans),
+    batch_commit_set_(batch_commit_set),
+    commit_message_(commit_message),
+    extensions_activity_buffer_(extensions_activity_buffer) {
 }
 
 BuildCommitCommand::~BuildCommitCommand() {}
@@ -81,10 +85,10 @@ void BuildCommitCommand::AddExtensionsActivityToMessage(
     // We will push this list of extensions activity back into the
     // ExtensionsActivityMonitor if this commit fails.  That's why we must keep
     // a copy of these records in the session.
-    monitor->GetAndClearRecords(session->mutable_extensions_activity());
+    monitor->GetAndClearRecords(extensions_activity_buffer_);
 
     const ExtensionsActivityMonitor::Records& records =
-        session->extensions_activity();
+        *extensions_activity_buffer_;
     for (ExtensionsActivityMonitor::Records::const_iterator it =
          records.begin();
          it != records.end(); ++it) {
@@ -111,7 +115,7 @@ void BuildCommitCommand::AddClientConfigParamsToMessage(
 }
 
 namespace {
-void SetEntrySpecifics(MutableEntry* meta_entry,
+void SetEntrySpecifics(Entry* meta_entry,
                        sync_pb::SyncEntity* sync_entry) {
   // Add the new style extension and the folder bit.
   sync_entry->mutable_specifics()->CopyFrom(meta_entry->Get(SPECIFICS));
@@ -126,8 +130,7 @@ SyncerError BuildCommitCommand::ExecuteImpl(SyncSession* session) {
   commit_message_->set_message_contents(sync_pb::ClientToServerMessage::COMMIT);
 
   sync_pb::CommitMessage* commit_message = commit_message_->mutable_commit();
-  commit_message->set_cache_guid(
-      session->write_transaction()->directory()->cache_guid());
+  commit_message->set_cache_guid(trans_->directory()->cache_guid());
   AddExtensionsActivityToMessage(session, commit_message);
   AddClientConfigParamsToMessage(session, commit_message);
 
@@ -143,8 +146,7 @@ SyncerError BuildCommitCommand::ExecuteImpl(SyncSession* session) {
     Id id = batch_commit_set_.GetCommitIdAt(i);
     sync_pb::SyncEntity* sync_entry = commit_message->add_entries();
     sync_entry->set_id_string(SyncableIdToProto(id));
-    MutableEntry meta_entry(session->write_transaction(),
-                            syncable::GET_BY_ID, id);
+    Entry meta_entry(trans_, syncable::GET_BY_ID, id);
     CHECK(meta_entry.good());
 
     DCHECK_NE(0UL,
@@ -173,7 +175,7 @@ SyncerError BuildCommitCommand::ExecuteImpl(SyncSession* session) {
     Id new_parent_id;
     if (meta_entry.Get(syncable::IS_DEL) &&
         !meta_entry.Get(syncable::PARENT_ID).ServerKnows()) {
-      new_parent_id = session->write_transaction()->root_id();
+      new_parent_id = trans_->root_id();
     } else {
       new_parent_id = meta_entry.Get(syncable::PARENT_ID);
     }
@@ -231,8 +233,8 @@ SyncerError BuildCommitCommand::ExecuteImpl(SyncSession* session) {
               FindAnchorPosition(syncable::PREV_ID, meta_entry),
               FindAnchorPosition(syncable::NEXT_ID, meta_entry));
         }
-        position_block.first = InterpolatePosition(position_block.first,
-                                                   position_block.second);
+        position_block.first = BuildCommitCommand::InterpolatePosition(
+            position_block.first, position_block.second);
 
         position_map[id] = position_block;
         sync_entry->set_position_in_parent(position_block.first);
@@ -261,6 +263,7 @@ int64 BuildCommitCommand::FindAnchorPosition(syncable::IdField direction,
       GetFirstPosition() : GetLastPosition();
 }
 
+// static
 int64 BuildCommitCommand::InterpolatePosition(const int64 lo,
                                               const int64 hi) {
   DCHECK_LE(lo, hi);
diff --git a/sync/engine/build_commit_command.h b/sync/engine/build_commit_command.h
index 253b6c0..a1b101d 100644
--- a/sync/engine/build_commit_command.h
+++ b/sync/engine/build_commit_command.h
@@ -11,6 +11,7 @@
 #include "sync/base/sync_export.h"
 #include "sync/engine/syncer_command.h"
 #include "sync/syncable/entry_kernel.h"
+#include "sync/util/extensions_activity_monitor.h"
 
 namespace syncer {
 
@@ -20,6 +21,7 @@ class OrderedCommitSet;
 
 namespace syncable {
 class Entry;
+class BaseTransaction;
 }
 
 // A class that contains the code used to serialize a set of sync items into a
@@ -36,8 +38,11 @@ class SYNC_EXPORT_PRIVATE BuildCommitCommand : public SyncerCommand {
   //
   // The commit_message parameter is an output parameter which will contain the
   // fully initialized commit message once ExecuteImpl() has been called.
-  BuildCommitCommand(const sessions::OrderedCommitSet& batch_commit_set,
-                     sync_pb::ClientToServerMessage* commit_message);
+  BuildCommitCommand(
+      syncable::BaseTransaction* trans,
+      const sessions::OrderedCommitSet& batch_commit_set,
+      sync_pb::ClientToServerMessage* commit_message,
+      ExtensionsActivityMonitor::Records* extensions_activity_buffer);
   virtual ~BuildCommitCommand();
 
   // SyncerCommand implementation.
@@ -67,15 +72,20 @@ class SYNC_EXPORT_PRIVATE BuildCommitCommand : public SyncerCommand {
                            const syncable::Entry& entry);
   // Given two values of the type returned by FindAnchorPosition,
   // compute a third value in between the two ranges.
-  int64 InterpolatePosition(int64 lo, int64 hi);
+  static int64 InterpolatePosition(int64 lo, int64 hi);
 
   DISALLOW_COPY_AND_ASSIGN(BuildCommitCommand);
 
+  // A pointer to a valid transaction not owned by this class.
+  syncable::BaseTransaction* trans_;
+
   // Input parameter; see constructor comment.
   const sessions::OrderedCommitSet& batch_commit_set_;
 
   // Output parameter; see constructor comment.
   sync_pb::ClientToServerMessage* commit_message_;
+
+  ExtensionsActivityMonitor::Records* extensions_activity_buffer_;
 };
 
 }  // namespace syncer
diff --git a/sync/engine/build_commit_command_unittest.cc b/sync/engine/build_commit_command_unittest.cc
index 6ad3c55..1ad8667 100644
--- a/sync/engine/build_commit_command_unittest.cc
+++ b/sync/engine/build_commit_command_unittest.cc
@@ -8,98 +8,98 @@
 namespace syncer {
 
 // A test fixture for tests exercising ClearDataCommandTest.
-class BuildCommitCommandTest : public SyncerCommandTest {
- protected:
-  BuildCommitCommandTest()
-    : batch_commit_set_(ModelSafeRoutingInfo()),
-      command_(batch_commit_set_, &commit_message_) {
-  }
-
- private:
-  sessions::OrderedCommitSet batch_commit_set_;
-  sync_pb::ClientToServerMessage commit_message_;
-
- protected:
-  BuildCommitCommand command_;
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(BuildCommitCommandTest);
-};
+class BuildCommitCommandTest : public SyncerCommandTest { };
 
 TEST_F(BuildCommitCommandTest, InterpolatePosition) {
   EXPECT_LT(BuildCommitCommand::GetFirstPosition(),
             BuildCommitCommand::GetLastPosition());
 
   // Dense ranges.
-  EXPECT_EQ(10, command_.InterpolatePosition(10, 10));
-  EXPECT_EQ(11, command_.InterpolatePosition(10, 11));
-  EXPECT_EQ(11, command_.InterpolatePosition(10, 12));
-  EXPECT_EQ(11, command_.InterpolatePosition(10, 13));
-  EXPECT_EQ(11, command_.InterpolatePosition(10, 14));
-  EXPECT_EQ(11, command_.InterpolatePosition(10, 15));
-  EXPECT_EQ(11, command_.InterpolatePosition(10, 16));
-  EXPECT_EQ(11, command_.InterpolatePosition(10, 17));
-  EXPECT_EQ(11, command_.InterpolatePosition(10, 18));
-  EXPECT_EQ(12, command_.InterpolatePosition(10, 19));
-  EXPECT_EQ(12, command_.InterpolatePosition(10, 20));
+  EXPECT_EQ(10, BuildCommitCommand::InterpolatePosition(10, 10));
+  EXPECT_EQ(11, BuildCommitCommand::InterpolatePosition(10, 11));
+  EXPECT_EQ(11, BuildCommitCommand::InterpolatePosition(10, 12));
+  EXPECT_EQ(11, BuildCommitCommand::InterpolatePosition(10, 13));
+  EXPECT_EQ(11, BuildCommitCommand::InterpolatePosition(10, 14));
+  EXPECT_EQ(11, BuildCommitCommand::InterpolatePosition(10, 15));
+  EXPECT_EQ(11, BuildCommitCommand::InterpolatePosition(10, 16));
+  EXPECT_EQ(11, BuildCommitCommand::InterpolatePosition(10, 17));
+  EXPECT_EQ(11, BuildCommitCommand::InterpolatePosition(10, 18));
+  EXPECT_EQ(12, BuildCommitCommand::InterpolatePosition(10, 19));
+  EXPECT_EQ(12, BuildCommitCommand::InterpolatePosition(10, 20));
 
   // Sparse ranges.
   EXPECT_EQ(0x32535ffe3dc97LL + BuildCommitCommand::GetGap(),
-      command_.InterpolatePosition(0x32535ffe3dc97LL, 0x61abcd323122cLL));
+      BuildCommitCommand::InterpolatePosition(
+          0x32535ffe3dc97LL, 0x61abcd323122cLL));
   EXPECT_EQ(~0x61abcd323122cLL + BuildCommitCommand::GetGap(),
-      command_.InterpolatePosition(~0x61abcd323122cLL, ~0x32535ffe3dc97LL));
+      BuildCommitCommand::InterpolatePosition(
+          ~0x61abcd323122cLL, ~0x32535ffe3dc97LL));
 
   // Lower limits
   EXPECT_EQ(BuildCommitCommand::GetFirstPosition() + 0x20,
-      command_.InterpolatePosition(
-          BuildCommitCommand::GetFirstPosition(),
-          BuildCommitCommand::GetFirstPosition() + 0x100));
+            BuildCommitCommand::InterpolatePosition(
+                BuildCommitCommand::GetFirstPosition(),
+                BuildCommitCommand::GetFirstPosition() + 0x100));
   EXPECT_EQ(BuildCommitCommand::GetFirstPosition() + 2,
-      command_.InterpolatePosition(BuildCommitCommand::GetFirstPosition() + 1,
-                                   BuildCommitCommand::GetFirstPosition() + 2));
+            BuildCommitCommand::InterpolatePosition(
+                BuildCommitCommand::GetFirstPosition() + 1,
+                BuildCommitCommand::GetFirstPosition() + 2));
   EXPECT_EQ(BuildCommitCommand::GetFirstPosition() +
             BuildCommitCommand::GetGap()/8 + 1,
-      command_.InterpolatePosition(
-          BuildCommitCommand::GetFirstPosition() + 1,
-          BuildCommitCommand::GetFirstPosition() + 1 +
-          BuildCommitCommand::GetGap()));
+            BuildCommitCommand::InterpolatePosition(
+                BuildCommitCommand::GetFirstPosition() + 1,
+                BuildCommitCommand::GetFirstPosition() + 1 +
+                BuildCommitCommand::GetGap()));
 
   // Extremal cases.
   EXPECT_EQ(0,
-      command_.InterpolatePosition(BuildCommitCommand::GetFirstPosition(),
-                                   BuildCommitCommand::GetLastPosition()));
+      BuildCommitCommand::InterpolatePosition(
+          BuildCommitCommand::GetFirstPosition(),
+          BuildCommitCommand::GetLastPosition()));
   EXPECT_EQ(BuildCommitCommand::GetFirstPosition() + 1 +
             BuildCommitCommand::GetGap(),
-      command_.InterpolatePosition(BuildCommitCommand::GetFirstPosition() + 1,
-                                   BuildCommitCommand::GetLastPosition()));
+            BuildCommitCommand::InterpolatePosition(
+                BuildCommitCommand::GetFirstPosition() + 1,
+                BuildCommitCommand::GetLastPosition()));
   EXPECT_EQ(BuildCommitCommand::GetFirstPosition() + 1 +
             BuildCommitCommand::GetGap(),
-      command_.InterpolatePosition(BuildCommitCommand::GetFirstPosition() + 1,
-                                   BuildCommitCommand::GetLastPosition() - 1));
+            BuildCommitCommand::InterpolatePosition(
+                BuildCommitCommand::GetFirstPosition() + 1,
+                BuildCommitCommand::GetLastPosition() - 1));
   EXPECT_EQ(BuildCommitCommand::GetLastPosition() - 1 -
             BuildCommitCommand::GetGap(),
-      command_.InterpolatePosition(BuildCommitCommand::GetFirstPosition(),
-                                   BuildCommitCommand::GetLastPosition() - 1));
+      BuildCommitCommand::InterpolatePosition(
+          BuildCommitCommand::GetFirstPosition(),
+          BuildCommitCommand::GetLastPosition() - 1));
 
   // Edge cases around zero.
   EXPECT_EQ(BuildCommitCommand::GetGap(),
-      command_.InterpolatePosition(0, BuildCommitCommand::GetLastPosition()));
+      BuildCommitCommand::InterpolatePosition(
+          0, BuildCommitCommand::GetLastPosition()));
   EXPECT_EQ(BuildCommitCommand::GetGap() + 1,
-      command_.InterpolatePosition(1, BuildCommitCommand::GetLastPosition()));
+      BuildCommitCommand::InterpolatePosition(
+          1, BuildCommitCommand::GetLastPosition()));
   EXPECT_EQ(BuildCommitCommand::GetGap() - 1,
-      command_.InterpolatePosition(-1, BuildCommitCommand::GetLastPosition()));
+      BuildCommitCommand::InterpolatePosition(
+          -1, BuildCommitCommand::GetLastPosition()));
   EXPECT_EQ(-BuildCommitCommand::GetGap(),
-      command_.InterpolatePosition(BuildCommitCommand::GetFirstPosition(), 0));
+      BuildCommitCommand::InterpolatePosition(
+          BuildCommitCommand::GetFirstPosition(), 0));
   EXPECT_EQ(-BuildCommitCommand::GetGap() + 1,
-      command_.InterpolatePosition(BuildCommitCommand::GetFirstPosition(), 1));
+      BuildCommitCommand::InterpolatePosition(
+          BuildCommitCommand::GetFirstPosition(), 1));
   EXPECT_EQ(-BuildCommitCommand::GetGap() - 1,
-      command_.InterpolatePosition(BuildCommitCommand::GetFirstPosition(), -1));
+      BuildCommitCommand::InterpolatePosition(
+          BuildCommitCommand::GetFirstPosition(), -1));
   EXPECT_EQ(BuildCommitCommand::GetGap() / 8,
-      command_.InterpolatePosition(0, BuildCommitCommand::GetGap()));
+      BuildCommitCommand::InterpolatePosition(
+          0, BuildCommitCommand::GetGap()));
   EXPECT_EQ(BuildCommitCommand::GetGap() / 4,
-      command_.InterpolatePosition(0, BuildCommitCommand::GetGap()*2));
+      BuildCommitCommand::InterpolatePosition(
+          0, BuildCommitCommand::GetGap()*2));
   EXPECT_EQ(BuildCommitCommand::GetGap(),
-      command_.InterpolatePosition(0, BuildCommitCommand::GetGap()*2 + 1));
+      BuildCommitCommand::InterpolatePosition(
+          0, BuildCommitCommand::GetGap()*2 + 1));
 }
 
 }  // namespace syncer
diff --git a/sync/engine/commit.cc b/sync/engine/commit.cc
index f2728cd..55fe4f5 100644
--- a/sync/engine/commit.cc
+++ b/sync/engine/commit.cc
@@ -61,20 +61,21 @@ void ClearSyncingBits(syncable::Directory* dir,
 // The ClientToServerMessage parameter is an output parameter which will contain
 // the commit message which should be sent to the server.  It is valid iff the
 // return value of this function is true.
-bool PrepareCommitMessage(sessions::SyncSession* session,
-                          sessions::OrderedCommitSet* commit_set,
-                          sync_pb::ClientToServerMessage* commit_message) {
+bool PrepareCommitMessage(
+    sessions::SyncSession* session,
+    sessions::OrderedCommitSet* commit_set,
+    sync_pb::ClientToServerMessage* commit_message,
+    ExtensionsActivityMonitor::Records* extensions_activity_buffer) {
   TRACE_EVENT0("sync", "PrepareCommitMessage");
 
   commit_set->Clear();
   commit_message->Clear();
 
   WriteTransaction trans(FROM_HERE, SYNCER, session->context()->directory());
-  sessions::ScopedSetSessionWriteTransaction set_trans(session, &trans);
 
   // Fetch the items to commit.
   const size_t batch_size = session->context()->max_commit_batch_size();
-  GetCommitIdsCommand get_commit_ids_command(batch_size, commit_set);
+  GetCommitIdsCommand get_commit_ids_command(&trans, batch_size, commit_set);
   get_commit_ids_command.Execute(session);
 
   DVLOG(1) << "Commit message will contain " << commit_set->Size() << " items.";
@@ -83,19 +84,30 @@ bool PrepareCommitMessage(sessions::SyncSession* session,
   }
 
   // Serialize the message.
-  BuildCommitCommand build_commit_command(*commit_set, commit_message);
+  BuildCommitCommand build_commit_command(&trans,
+                                          *commit_set,
+                                          commit_message,
+                                          extensions_activity_buffer);
   build_commit_command.Execute(session);
 
-  SetSyncingBits(session->write_transaction(), *commit_set);
+  SetSyncingBits(&trans, *commit_set);
   return true;
 }
 
 SyncerError BuildAndPostCommitsImpl(Syncer* syncer,
                                     sessions::SyncSession* session,
                                     sessions::OrderedCommitSet* commit_set) {
-  sync_pb::ClientToServerMessage commit_message;
-  while (!syncer->ExitRequested() &&
-         PrepareCommitMessage(session, commit_set, &commit_message)) {
+  while (!syncer->ExitRequested()) {
+    sync_pb::ClientToServerMessage commit_message;
+    ExtensionsActivityMonitor::Records extensions_activity_buffer;
+
+    if (!PrepareCommitMessage(session,
+                              commit_set,
+                              &commit_message,
+                              &extensions_activity_buffer)) {
+      break;
+    }
+
     sync_pb::ClientToServerResponse commit_response;
 
     DVLOG(1) << "Sending commit message.";
@@ -104,6 +116,9 @@ SyncerError BuildAndPostCommitsImpl(Syncer* syncer,
         &commit_message, &commit_response, session);
     TRACE_EVENT_END0("sync", "PostCommit");
 
+    // TODO(rlarocque): Put all the post-commit logic in one place.
+    // See crbug.com/196338.
+
     if (post_result != SYNCER_OK) {
       LOG(WARNING) << "Post commit failed";
       return post_result;
@@ -130,6 +145,14 @@ SyncerError BuildAndPostCommitsImpl(Syncer* syncer,
         process_response_command.Execute(session);
     TRACE_EVENT_END0("sync", "ProcessCommitResponse");
 
+    // If the commit failed, return the data to the ExtensionsActivityMonitor.
+    if (session->status_controller().
+        model_neutral_state().num_successful_bookmark_commits == 0) {
+      ExtensionsActivityMonitor* extensions_activity_monitor =
+          session->context()->extensions_monitor();
+      extensions_activity_monitor->PutRecords(extensions_activity_buffer);
+    }
+
     if (processing_result != SYNCER_OK) {
       return processing_result;
     }
diff --git a/sync/engine/get_commit_ids_command.cc b/sync/engine/get_commit_ids_command.cc
index a84024a..6b2325f 100644
--- a/sync/engine/get_commit_ids_command.cc
+++ b/sync/engine/get_commit_ids_command.cc
@@ -11,11 +11,10 @@
 #include "sync/engine/syncer_util.h"
 #include "sync/engine/throttled_data_type_tracker.h"
 #include "sync/syncable/entry.h"
-#include "sync/syncable/mutable_entry.h"
 #include "sync/syncable/nigori_handler.h"
 #include "sync/syncable/nigori_util.h"
+#include "sync/syncable/syncable_base_transaction.h"
 #include "sync/syncable/syncable_util.h"
-#include "sync/syncable/syncable_write_transaction.h"
 #include "sync/util/cryptographer.h"
 
 using std::set;
@@ -28,9 +27,11 @@ using sessions::SyncSession;
 using sessions::StatusController;
 
 GetCommitIdsCommand::GetCommitIdsCommand(
+    syncable::BaseTransaction* trans,
     const size_t commit_batch_size,
     sessions::OrderedCommitSet* commit_set)
-    : requested_commit_batch_size_(commit_batch_size),
+    : trans_(trans),
+      requested_commit_batch_size_(commit_batch_size),
       commit_set_(commit_set) {
 }
 
@@ -41,17 +42,17 @@ SyncerError GetCommitIdsCommand::ExecuteImpl(SyncSession* session) {
   // are not in the correct order for commit.
   std::set<int64> ready_unsynced_set;
   syncable::Directory::UnsyncedMetaHandles all_unsynced_handles;
-  GetUnsyncedEntries(session->write_transaction(),
+  GetUnsyncedEntries(trans_,
                      &all_unsynced_handles);
 
   ModelTypeSet encrypted_types;
   bool passphrase_missing = false;
   Cryptographer* cryptographer =
       session->context()->
-      directory()->GetCryptographer(session->write_transaction());
+      directory()->GetCryptographer(trans_);
   if (cryptographer) {
     encrypted_types = session->context()->directory()->GetNigoriHandler()->
-        GetEncryptedTypes(session->write_transaction());
+        GetEncryptedTypes(trans_);
     passphrase_missing = cryptographer->has_pending_keys();
   };
 
@@ -60,14 +61,14 @@ SyncerError GetCommitIdsCommand::ExecuteImpl(SyncSession* session) {
   // We filter out all unready entries from the set of unsynced handles. This
   // new set of ready and unsynced items (which excludes throttled items as
   // well) is then what we use to determine what is a candidate for commit.
-  FilterUnreadyEntries(session->write_transaction(),
+  FilterUnreadyEntries(trans_,
                        throttled_types,
                        encrypted_types,
                        passphrase_missing,
                        all_unsynced_handles,
                        &ready_unsynced_set);
 
-  BuildCommitIds(session->write_transaction(),
+  BuildCommitIds(trans_,
                  session->context()->routing_info(),
                  ready_unsynced_set);
 
@@ -292,7 +293,7 @@ bool GetCommitIdsCommand::IsCommitBatchFull() const {
 }
 
 void GetCommitIdsCommand::AddCreatesAndMoves(
-    syncable::WriteTransaction* write_transaction,
+    syncable::BaseTransaction* trans,
     const ModelSafeRoutingInfo& routes,
     const std::set<int64>& ready_unsynced_set) {
   // Add moves and creates, and prepend their uncommitted parents.
@@ -302,7 +303,7 @@ void GetCommitIdsCommand::AddCreatesAndMoves(
     if (commit_set_->HaveCommitItem(metahandle))
       continue;
 
-    syncable::Entry entry(write_transaction,
+    syncable::Entry entry(trans,
                           syncable::GET_BY_HANDLE,
                           metahandle);
     if (!entry.Get(syncable::IS_DEL)) {
@@ -310,12 +311,12 @@ void GetCommitIdsCommand::AddCreatesAndMoves(
       // dependencies are not in conflict.
       OrderedCommitSet item_dependencies(routes);
       if (AddUncommittedParentsAndTheirPredecessors(
-              write_transaction,
+              trans,
               routes,
               ready_unsynced_set,
               entry,
               &item_dependencies) &&
-          AddPredecessorsThenItem(write_transaction,
+          AddPredecessorsThenItem(trans,
                                   routes,
                                   ready_unsynced_set,
                                   entry,
@@ -331,7 +332,7 @@ void GetCommitIdsCommand::AddCreatesAndMoves(
 }
 
 void GetCommitIdsCommand::AddDeletes(
-    syncable::WriteTransaction* write_transaction,
+    syncable::BaseTransaction* trans,
     const std::set<int64>& ready_unsynced_set) {
   set<syncable::Id> legal_delete_parents;
 
@@ -341,11 +342,11 @@ void GetCommitIdsCommand::AddDeletes(
     if (commit_set_->HaveCommitItem(metahandle))
       continue;
 
-    syncable::Entry entry(write_transaction, syncable::GET_BY_HANDLE,
+    syncable::Entry entry(trans, syncable::GET_BY_HANDLE,
                           metahandle);
 
     if (entry.Get(syncable::IS_DEL)) {
-      syncable::Entry parent(write_transaction, syncable::GET_BY_ID,
+      syncable::Entry parent(trans, syncable::GET_BY_ID,
                              entry.Get(syncable::PARENT_ID));
       // If the parent is deleted and unsynced, then any children of that
       // parent don't need to be added to the delete queue.
@@ -402,8 +403,8 @@ void GetCommitIdsCommand::AddDeletes(
     int64 metahandle = *iter;
     if (commit_set_->HaveCommitItem(metahandle))
       continue;
-    syncable::MutableEntry entry(write_transaction, syncable::GET_BY_HANDLE,
-                                 metahandle);
+    syncable::Entry entry(trans, syncable::GET_BY_HANDLE,
+                          metahandle);
     if (entry.Get(syncable::IS_DEL)) {
       syncable::Id parent_id = entry.Get(syncable::PARENT_ID);
       if (legal_delete_parents.count(parent_id)) {
@@ -415,7 +416,7 @@ void GetCommitIdsCommand::AddDeletes(
 }
 
 void GetCommitIdsCommand::BuildCommitIds(
-    syncable::WriteTransaction* write_transaction,
+    syncable::BaseTransaction* trans,
     const ModelSafeRoutingInfo& routes,
     const std::set<int64>& ready_unsynced_set) {
   // Commits follow these rules:
@@ -428,10 +429,10 @@ void GetCommitIdsCommand::BuildCommitIds(
   // delete trees.
 
   // Add moves and creates, and prepend their uncommitted parents.
-  AddCreatesAndMoves(write_transaction, routes, ready_unsynced_set);
+  AddCreatesAndMoves(trans, routes, ready_unsynced_set);
 
   // Add all deletes.
-  AddDeletes(write_transaction, ready_unsynced_set);
+  AddDeletes(trans, ready_unsynced_set);
 }
 
 }  // namespace syncer
diff --git a/sync/engine/get_commit_ids_command.h b/sync/engine/get_commit_ids_command.h
index 86abf24..a41dd00 100644
--- a/sync/engine/get_commit_ids_command.h
+++ b/sync/engine/get_commit_ids_command.h
@@ -12,7 +12,6 @@
 #include "sync/base/sync_export.h"
 #include "sync/engine/syncer_command.h"
 #include "sync/engine/syncer_util.h"
-#include "sync/sessions/ordered_commit_set.h"
 #include "sync/sessions/sync_session.h"
 #include "sync/syncable/directory.h"
 
@@ -21,6 +20,10 @@ using std::vector;
 
 namespace syncer {
 
+namespace sessions {
+class OrderedCommitSet;
+}
+
 // A class that contains the code used to search the syncable::Directory for
 // locally modified items that are ready to be committed to the server.
 //
@@ -36,7 +39,8 @@ class SYNC_EXPORT_PRIVATE GetCommitIdsCommand : public SyncerCommand {
   // set of items that are ready to commit.  Its size shall not exceed the
   // provided batch_size.  This contents of this "set" will be ordered; see the
   // comments in this class' implementation for details.
-  GetCommitIdsCommand(const size_t commit_batch_size,
+  GetCommitIdsCommand(syncable::BaseTransaction* trans,
+                      const size_t commit_batch_size,
                       sessions::OrderedCommitSet* ordered_commit_set);
   virtual ~GetCommitIdsCommand();
 
@@ -44,7 +48,7 @@ class SYNC_EXPORT_PRIVATE GetCommitIdsCommand : public SyncerCommand {
   virtual SyncerError ExecuteImpl(sessions::SyncSession* session) OVERRIDE;
 
   // Builds a vector of IDs that should be committed.
-  void BuildCommitIds(syncable::WriteTransaction* write_transaction,
+  void BuildCommitIds(syncable::BaseTransaction* write_transaction,
                       const ModelSafeRoutingInfo& routes,
                       const std::set<int64>& ready_unsynced_set);
 
@@ -119,13 +123,16 @@ class SYNC_EXPORT_PRIVATE GetCommitIdsCommand : public SyncerCommand {
 
   bool IsCommitBatchFull() const;
 
-  void AddCreatesAndMoves(syncable::WriteTransaction* write_transaction,
+  void AddCreatesAndMoves(syncable::BaseTransaction* write_transaction,
                           const ModelSafeRoutingInfo& routes,
                           const std::set<int64>& ready_unsynced_set);
 
-  void AddDeletes(syncable::WriteTransaction* write_transaction,
+  void AddDeletes(syncable::BaseTransaction* write_transaction,
                   const std::set<int64>& ready_unsynced_set);
 
+  // A pointer to a valid transaction not owned by this class.
+  syncable::BaseTransaction* trans_;
+
   // Input parameter; see constructor comment.
   const size_t requested_commit_batch_size_;
 
diff --git a/sync/engine/process_commit_response_command.cc b/sync/engine/process_commit_response_command.cc
index 49b1a4d..96a82bd 100644
--- a/sync/engine/process_commit_response_command.cc
+++ b/sync/engine/process_commit_response_command.cc
@@ -84,25 +84,6 @@ std::set<ModelSafeGroup> ProcessCommitResponseCommand::GetGroupsToChange(
 
 SyncerError ProcessCommitResponseCommand::ModelChangingExecuteImpl(
     SyncSession* session) {
-  SyncerError result = ProcessCommitResponse(session);
-  ExtensionsActivityMonitor* monitor = session->context()->extensions_monitor();
-
-  // This is to be run on one model only: the bookmark model.
-  if (session->status_controller().HasBookmarkCommitActivity()) {
-    // If the commit failed, return the data to the ExtensionsActivityMonitor.
-    if (session->status_controller().
-        model_neutral_state().num_successful_bookmark_commits == 0) {
-      monitor->PutRecords(session->extensions_activity());
-    }
-    // Clear our cached data in either case.
-    session->mutable_extensions_activity()->clear();
-  }
-
-  return result;
-}
-
-SyncerError ProcessCommitResponseCommand::ProcessCommitResponse(
-    SyncSession* session) {
   syncable::Directory* dir = session->context()->directory();
   StatusController* status = session->mutable_status_controller();
   const CommitResponse& cr = commit_response_.commit();
diff --git a/sync/engine/process_commit_response_command.h b/sync/engine/process_commit_response_command.h
index f7efcfc..155a47d 100644
--- a/sync/engine/process_commit_response_command.h
+++ b/sync/engine/process_commit_response_command.h
@@ -73,9 +73,6 @@ class SYNC_EXPORT_PRIVATE ProcessCommitResponseCommand
       const syncable::Id& pre_commit_id,
       std::set<syncable::Id>* deleted_folders);
 
-  // Actually does the work of execute.
-  SyncerError ProcessCommitResponse(sessions::SyncSession* session);
-
   void ProcessSuccessfulCommitResponse(
       const sync_pb::SyncEntity& committed_entry,
       const sync_pb::CommitResponse_EntryResponse& entry_response,
diff --git a/sync/engine/process_commit_response_command_unittest.cc b/sync/engine/process_commit_response_command_unittest.cc
index 3fb7017..5c96424 100644
--- a/sync/engine/process_commit_response_command_unittest.cc
+++ b/sync/engine/process_commit_response_command_unittest.cc
@@ -349,82 +349,4 @@ TEST_F(ProcessCommitResponseCommandTest, NewFolderCommitKeepsChildOrder) {
       << "Too few or too many children in parent folder after commit.";
 }
 
-// This test fixture runs across a Cartesian product of per-type fail/success
-// possibilities.
-enum {
-  TEST_PARAM_BOOKMARK_ENABLE_BIT,
-  TEST_PARAM_AUTOFILL_ENABLE_BIT,
-  TEST_PARAM_BIT_COUNT
-};
-class MixedResult :
-    public ProcessCommitResponseCommandTest,
-    public ::testing::WithParamInterface<int> {
- protected:
-  bool ShouldFailBookmarkCommit() {
-    return (GetParam() & (1 << TEST_PARAM_BOOKMARK_ENABLE_BIT)) == 0;
-  }
-  bool ShouldFailAutofillCommit() {
-    return (GetParam() & (1 << TEST_PARAM_AUTOFILL_ENABLE_BIT)) == 0;
-  }
-};
-INSTANTIATE_TEST_CASE_P(ProcessCommitResponse,
-                        MixedResult,
-                        testing::Range(0, 1 << TEST_PARAM_BIT_COUNT));
-
-// This test commits 2 items (one bookmark, one autofill) and validates what
-// happens to the extensions activity records.  Commits could fail or succeed,
-// depending on the test parameter.
-TEST_P(MixedResult, ExtensionActivity) {
-  sessions::OrderedCommitSet commit_set(session()->context()->routing_info());
-  sync_pb::ClientToServerMessage request;
-  sync_pb::ClientToServerResponse response;
-
-  EXPECT_NE(routing_info().find(BOOKMARKS)->second,
-            routing_info().find(AUTOFILL)->second)
-      << "To not be lame, this test requires more than one active group.";
-
-  // Bookmark item setup.
-  CreateUnprocessedCommitResult(
-      id_factory_.NewServerId(),
-      id_factory_.root(), "Some bookmark", false,
-      BOOKMARKS, &commit_set, &request, &response);
-  if (ShouldFailBookmarkCommit())
-    SetLastErrorCode(CommitResponse::TRANSIENT_ERROR, &response);
-  // Autofill item setup.
-  CreateUnprocessedCommitResult(
-      id_factory_.NewServerId(),
-      id_factory_.root(), "Some autofill", false,
-      AUTOFILL, &commit_set, &request, &response);
-  if (ShouldFailAutofillCommit())
-    SetLastErrorCode(CommitResponse::TRANSIENT_ERROR, &response);
-
-  // Put some extensions activity in the session.
-  {
-    ExtensionsActivityMonitor::Records* records =
-        session()->mutable_extensions_activity();
-    (*records)["ABC"].extension_id = "ABC";
-    (*records)["ABC"].bookmark_write_count = 2049U;
-    (*records)["xyz"].extension_id = "xyz";
-    (*records)["xyz"].bookmark_write_count = 4U;
-  }
-  ProcessCommitResponseCommand command(commit_set, request, response);
-  command.ExecuteImpl(session());
-  ExpectGroupsToChange(command, GROUP_UI, GROUP_DB);
-
-  ExtensionsActivityMonitor::Records final_monitor_records;
-  context()->extensions_monitor()->GetAndClearRecords(&final_monitor_records);
-
-  if (ShouldFailBookmarkCommit()) {
-    ASSERT_EQ(2U, final_monitor_records.size())
-        << "Should restore records after unsuccessful bookmark commit.";
-    EXPECT_EQ("ABC", final_monitor_records["ABC"].extension_id);
-    EXPECT_EQ("xyz", final_monitor_records["xyz"].extension_id);
-    EXPECT_EQ(2049U, final_monitor_records["ABC"].bookmark_write_count);
-    EXPECT_EQ(4U,    final_monitor_records["xyz"].bookmark_write_count);
-  } else {
-    EXPECT_TRUE(final_monitor_records.empty())
-        << "Should not restore records after successful bookmark commit.";
-  }
-}
-
 }  // namespace syncer
diff --git a/sync/engine/syncer_unittest.cc b/sync/engine/syncer_unittest.cc
index 304dd5a..1bdc13a 100644
--- a/sync/engine/syncer_unittest.cc
+++ b/sync/engine/syncer_unittest.cc
@@ -105,7 +105,6 @@ using syncable::SPECIFICS;
 using syncable::SYNCING;
 using syncable::UNITTEST;
 
-using sessions::ScopedSetSessionWriteTransaction;
 using sessions::StatusController;
 using sessions::SyncSessionContext;
 using sessions::SyncSession;
@@ -409,18 +408,16 @@ class SyncerTest : public testing::Test,
                         const vector<syncable::Id>& expected_id_order) {
     for (size_t limit = expected_id_order.size() + 2; limit > 0; --limit) {
       WriteTransaction wtrans(FROM_HERE, UNITTEST, directory());
-      ScopedSetSessionWriteTransaction set_trans(session_.get(), &wtrans);
 
       ModelSafeRoutingInfo routes;
       GetModelSafeRoutingInfo(&routes);
       sessions::OrderedCommitSet output_set(routes);
-      GetCommitIdsCommand command(limit, &output_set);
+      GetCommitIdsCommand command(&wtrans, limit, &output_set);
       std::set<int64> ready_unsynced_set;
       command.FilterUnreadyEntries(&wtrans, ModelTypeSet(),
                                    ModelTypeSet(), false,
                                    unsynced_handle_view, &ready_unsynced_set);
-      command.BuildCommitIds(session_->write_transaction(), routes,
-                             ready_unsynced_set);
+      command.BuildCommitIds(&wtrans, routes, ready_unsynced_set);
       size_t truncated_size = std::min(limit, expected_id_order.size());
       ASSERT_EQ(truncated_size, output_set.Size());
       for (size_t i = 0; i < truncated_size; ++i) {
@@ -4924,4 +4921,76 @@ TEST_F(SyncerPositionTiebreakingTest, MidLowHigh) {
   ExpectLocalOrderIsByServerId();
 }
 
+enum {
+  TEST_PARAM_BOOKMARK_ENABLE_BIT,
+  TEST_PARAM_AUTOFILL_ENABLE_BIT,
+  TEST_PARAM_BIT_COUNT
+};
+
+class MixedResult :
+    public SyncerTest,
+    public ::testing::WithParamInterface<int> {
+ protected:
+  bool ShouldFailBookmarkCommit() {
+    return (GetParam() & (1 << TEST_PARAM_BOOKMARK_ENABLE_BIT)) == 0;
+  }
+  bool ShouldFailAutofillCommit() {
+    return (GetParam() & (1 << TEST_PARAM_AUTOFILL_ENABLE_BIT)) == 0;
+  }
+};
+
+INSTANTIATE_TEST_CASE_P(ExtensionsActivity,
+                        MixedResult,
+                        testing::Range(0, 1 << TEST_PARAM_BIT_COUNT));
+
+TEST_P(MixedResult, ExtensionsActivity) {
+  {
+    WriteTransaction wtrans(FROM_HERE, UNITTEST, directory());
+
+    MutableEntry pref(&wtrans, CREATE, PREFERENCES, wtrans.root_id(), "pref");
+    ASSERT_TRUE(pref.good());
+    pref.Put(syncable::IS_UNSYNCED, true);
+
+    MutableEntry bookmark(
+        &wtrans, CREATE, BOOKMARKS, wtrans.root_id(), "bookmark");
+    ASSERT_TRUE(bookmark.good());
+    bookmark.Put(syncable::IS_UNSYNCED, true);
+
+    if (ShouldFailBookmarkCommit()) {
+      mock_server_->SetTransientErrorId(bookmark.Get(ID));
+    }
+
+    if (ShouldFailAutofillCommit()) {
+      mock_server_->SetTransientErrorId(pref.Get(ID));
+    }
+  }
+
+
+  // Put some extenions activity records into the monitor.
+  {
+    ExtensionsActivityMonitor::Records records;
+    records["ABC"].extension_id = "ABC";
+    records["ABC"].bookmark_write_count = 2049U;
+    records["xyz"].extension_id = "xyz";
+    records["xyz"].bookmark_write_count = 4U;
+    context_->extensions_monitor()->PutRecords(records);
+  }
+
+  SyncShareNudge();
+
+  ExtensionsActivityMonitor::Records final_monitor_records;
+  context_->extensions_monitor()->GetAndClearRecords(&final_monitor_records);
+  if (ShouldFailBookmarkCommit()) {
+    ASSERT_EQ(2U, final_monitor_records.size())
+        << "Should restore records after unsuccessful bookmark commit.";
+    EXPECT_EQ("ABC", final_monitor_records["ABC"].extension_id);
+    EXPECT_EQ("xyz", final_monitor_records["xyz"].extension_id);
+    EXPECT_EQ(2049U, final_monitor_records["ABC"].bookmark_write_count);
+    EXPECT_EQ(4U,    final_monitor_records["xyz"].bookmark_write_count);
+  } else {
+    EXPECT_TRUE(final_monitor_records.empty())
+        << "Should not restore records after successful bookmark commit.";
+  }
+}
+
 }  // namespace syncer
diff --git a/sync/sessions/status_controller.cc b/sync/sessions/status_controller.cc
index 2287a51..abd6f1e 100644
--- a/sync/sessions/status_controller.cc
+++ b/sync/sessions/status_controller.cc
@@ -13,10 +13,9 @@
 namespace syncer {
 namespace sessions {
 
-StatusController::StatusController(const ModelSafeRoutingInfo& routes)
+StatusController::StatusController()
     : group_restriction_in_effect_(false),
-      group_restriction_(GROUP_PASSIVE),
-      routing_info_(routes) {
+      group_restriction_(GROUP_PASSIVE) {
 }
 
 StatusController::~StatusController() {}
diff --git a/sync/sessions/status_controller.h b/sync/sessions/status_controller.h
index bf100b0..5f8de13 100644
--- a/sync/sessions/status_controller.h
+++ b/sync/sessions/status_controller.h
@@ -43,7 +43,7 @@ namespace sessions {
 
 class SYNC_EXPORT_PRIVATE StatusController {
  public:
-  explicit StatusController(const ModelSafeRoutingInfo& routes);
+  explicit StatusController();
   ~StatusController();
 
   // ClientToServer messages.
@@ -112,10 +112,6 @@ class SYNC_EXPORT_PRIVATE StatusController {
     return sync_start_time_;
   }
 
-  bool HasBookmarkCommitActivity() const {
-    return ActiveGroupRestrictionIncludesModel(BOOKMARKS);
-  }
-
   const ModelNeutralState& model_neutral_state() const {
     return model_neutral_;
   }
@@ -159,17 +155,6 @@ class SYNC_EXPORT_PRIVATE StatusController {
  private:
   friend class ScopedModelSafeGroupRestriction;
 
-  // Check whether a particular model is included by the active group
-  // restriction.
-  bool ActiveGroupRestrictionIncludesModel(ModelType model) const {
-    if (!group_restriction_in_effect_)
-      return true;
-    ModelSafeRoutingInfo::const_iterator it = routing_info_.find(model);
-    if (it == routing_info_.end())
-      return false;
-    return group_restriction() == it->second;
-  }
-
   ModelNeutralState model_neutral_;
 
   // Used to fail read/write operations on state that don't obey the current
@@ -177,8 +162,6 @@ class SYNC_EXPORT_PRIVATE StatusController {
   bool group_restriction_in_effect_;
   ModelSafeGroup group_restriction_;
 
-  const ModelSafeRoutingInfo routing_info_;
-
   base::Time sync_start_time_;
 
   DISALLOW_COPY_AND_ASSIGN(StatusController);
diff --git a/sync/sessions/status_controller_unittest.cc b/sync/sessions/status_controller_unittest.cc
index d361a6e..e6b59e8 100644
--- a/sync/sessions/status_controller_unittest.cc
+++ b/sync/sessions/status_controller_unittest.cc
@@ -9,20 +9,13 @@
 namespace syncer {
 namespace sessions {
 
-class StatusControllerTest : public testing::Test {
- public:
-  virtual void SetUp() {
-    routes_[BOOKMARKS] = GROUP_UI;
-  }
- protected:
-  ModelSafeRoutingInfo routes_;
-};
+class StatusControllerTest : public testing::Test { };
 
 // This test is useful, as simple as it sounds, due to the copy-paste prone
 // nature of status_controller.cc (we have had bugs in the past where a set_foo
 // method was actually setting |bar_| instead!).
 TEST_F(StatusControllerTest, ReadYourWrites) {
-  StatusController status(routes_);
+  StatusController status;
   status.set_num_server_changes_remaining(13);
   EXPECT_EQ(13, status.num_server_changes_remaining());
 
@@ -39,7 +32,7 @@ TEST_F(StatusControllerTest, ReadYourWrites) {
 }
 
 TEST_F(StatusControllerTest, CountUpdates) {
-  StatusController status(routes_);
+  StatusController status;
   EXPECT_EQ(0, status.CountUpdates());
   sync_pb::ClientToServerResponse* response(status.mutable_updates_response());
   sync_pb::SyncEntity* entity1 = response->mutable_get_updates()->add_entries();
@@ -50,7 +43,7 @@ TEST_F(StatusControllerTest, CountUpdates) {
 
 // Test TotalNumConflictingItems
 TEST_F(StatusControllerTest, TotalNumConflictingItems) {
-  StatusController status(routes_);
+  StatusController status;
   EXPECT_EQ(0, status.TotalNumConflictingItems());
 
   status.increment_num_server_conflicts();
@@ -61,7 +54,7 @@ TEST_F(StatusControllerTest, TotalNumConflictingItems) {
 
 // Basic test that non group-restricted state accessors don't cause violations.
 TEST_F(StatusControllerTest, Unrestricted) {
-  StatusController status(routes_);
+  StatusController status;
   status.model_neutral_state();
   status.download_updates_succeeded();
   status.ServerSaysNothingMoreToDownload();
diff --git a/sync/sessions/sync_session.cc b/sync/sessions/sync_session.cc
index b7ff433..58cf11a 100644
--- a/sync/sessions/sync_session.cc
+++ b/sync/sessions/sync_session.cc
@@ -21,9 +21,8 @@ SyncSession::SyncSession(
     const SyncSourceInfo& source)
     : context_(context),
       source_(source),
-      write_transaction_(NULL),
       delegate_(delegate) {
-  status_controller_.reset(new StatusController(context_->routing_info()));
+  status_controller_.reset(new StatusController());
   debug_info_sources_list_.push_back(source_);
 }
 
diff --git a/sync/sessions/sync_session.h b/sync/sessions/sync_session.h
index 9272bae..126d57f 100644
--- a/sync/sessions/sync_session.h
+++ b/sync/sessions/sync_session.h
@@ -30,15 +30,10 @@
 #include "sync/sessions/ordered_commit_set.h"
 #include "sync/sessions/status_controller.h"
 #include "sync/sessions/sync_session_context.h"
-#include "sync/util/extensions_activity_monitor.h"
 
 namespace syncer {
 class ModelSafeWorker;
 
-namespace syncable {
-class WriteTransaction;
-}
-
 namespace sessions {
 
 class SYNC_EXPORT_PRIVATE SyncSession {
@@ -113,7 +108,6 @@ class SYNC_EXPORT_PRIVATE SyncSession {
   // TODO(akalin): Split this into context() and mutable_context().
   SyncSessionContext* context() const { return context_; }
   Delegate* delegate() const { return delegate_; }
-  syncable::WriteTransaction* write_transaction() { return write_transaction_; }
   const StatusController& status_controller() const {
     return *status_controller_.get();
   }
@@ -121,21 +115,9 @@ class SYNC_EXPORT_PRIVATE SyncSession {
     return status_controller_.get();
   }
 
-  const ExtensionsActivityMonitor::Records& extensions_activity() const {
-    return extensions_activity_;
-  }
-  ExtensionsActivityMonitor::Records* mutable_extensions_activity() {
-    return &extensions_activity_;
-  }
-
   const SyncSourceInfo& source() const { return source_; }
 
  private:
-  // Extend the encapsulation boundary to utilities for internal member
-  // assignments. This way, the scope of these actions is explicit, they can't
-  // be overridden, and assigning is always accompanied by unassigning.
-  friend class ScopedSetSessionWriteTransaction;
-
   // The context for this session, guaranteed to outlive |this|.
   SyncSessionContext* const context_;
 
@@ -146,12 +128,6 @@ class SYNC_EXPORT_PRIVATE SyncSession {
   // Currently used only for logging.
   std::vector<SyncSourceInfo> debug_info_sources_list_;
 
-  // Information about extensions activity since the last successful commit.
-  ExtensionsActivityMonitor::Records extensions_activity_;
-
-  // Used to allow various steps to share a transaction. Can be NULL.
-  syncable::WriteTransaction* write_transaction_;
-
   // The delegate for this session, must never be NULL.
   Delegate* const delegate_;
 
@@ -161,24 +137,6 @@ class SYNC_EXPORT_PRIVATE SyncSession {
   DISALLOW_COPY_AND_ASSIGN(SyncSession);
 };
 
-// Installs a WriteTransaction to a given session and later clears it when the
-// utility falls out of scope. Transactions are not nestable, so it is an error
-// to try and use one of these if the session already has a transaction.
-class ScopedSetSessionWriteTransaction {
- public:
-  ScopedSetSessionWriteTransaction(SyncSession* session,
-                                   syncable::WriteTransaction* trans)
-      : session_(session) {
-    DCHECK(!session_->write_transaction_);
-    session_->write_transaction_ = trans;
-  }
-  ~ScopedSetSessionWriteTransaction() { session_->write_transaction_ = NULL; }
-
- private:
-  SyncSession* session_;
-  DISALLOW_COPY_AND_ASSIGN(ScopedSetSessionWriteTransaction);
-};
-
 }  // namespace sessions
 }  // namespace syncer
 
diff --git a/sync/sessions/sync_session_unittest.cc b/sync/sessions/sync_session_unittest.cc
index bb4a3d5..c3c4d69 100644
--- a/sync/sessions/sync_session_unittest.cc
+++ b/sync/sessions/sync_session_unittest.cc
@@ -140,20 +140,6 @@ class SyncSessionTest : public testing::Test,
   scoped_ptr<ThrottledDataTypeTracker> throttled_data_type_tracker_;
 };
 
-TEST_F(SyncSessionTest, SetWriteTransaction) {
-  TestDirectorySetterUpper dir_maker;
-  dir_maker.SetUp();
-  syncable::Directory* directory = dir_maker.directory();
-
-  scoped_ptr<SyncSession> session(MakeSession());
-  EXPECT_TRUE(NULL == session->write_transaction());
-  {
-    WriteTransaction trans(FROM_HERE, syncable::UNITTEST, directory);
-    sessions::ScopedSetSessionWriteTransaction set_trans(session.get(), &trans);
-    EXPECT_TRUE(&trans == session->write_transaction());
-  }
-}
-
 TEST_F(SyncSessionTest, MoreToDownloadIfDownloadFailed) {
   status()->set_updates_request_types(ParamsMeaningAllEnabledTypes());
 
diff --git a/sync/test/engine/mock_connection_manager.cc b/sync/test/engine/mock_connection_manager.cc
index 1718699..3fd4a32 100644
--- a/sync/test/engine/mock_connection_manager.cc
+++ b/sync/test/engine/mock_connection_manager.cc
@@ -18,6 +18,7 @@
 #include "sync/test/engine/test_id_factory.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using std::find;
 using std::map;
 using std::string;
 using sync_pb::ClientToServerMessage;
@@ -224,6 +225,10 @@ void MockConnectionManager::SetCommitClientCommand(
   commit_client_command_.reset(command);
 }
 
+void MockConnectionManager::SetTransientErrorId(syncable::Id id) {
+  transient_error_ids_.push_back(id);
+}
+
 sync_pb::SyncEntity* MockConnectionManager::AddUpdateBookmark(
     int id, int parent_id,
     string name, int64 version,
@@ -545,6 +550,11 @@ bool MockConnectionManager::ShouldConflictThisCommit() {
   return conflict;
 }
 
+bool MockConnectionManager::ShouldTransientErrorThisId(syncable::Id id) {
+  return find(transient_error_ids_.begin(), transient_error_ids_.end(), id)
+      != transient_error_ids_.end();
+}
+
 void MockConnectionManager::ProcessCommit(
     sync_pb::ClientToServerMessage* csm,
     sync_pb::ClientToServerResponse* response_buffer) {
@@ -559,40 +569,47 @@ void MockConnectionManager::ProcessCommit(
   for (int i = 0; i < commit_message.entries_size() ; i++) {
     const sync_pb::SyncEntity& entry = commit_message.entries(i);
     CHECK(entry.has_id_string());
-    string id = entry.id_string();
+    string id_string = entry.id_string();
     ASSERT_LT(entry.name().length(), 256ul) << " name probably too long. True "
         "server name checking not implemented";
+    syncable::Id id;
     if (entry.version() == 0) {
       // Relies on our new item string id format. (string representation of a
       // negative number).
-      committed_ids_.push_back(syncable::Id::CreateFromClientString(id));
+      id = syncable::Id::CreateFromClientString(id_string);
     } else {
-      committed_ids_.push_back(syncable::Id::CreateFromServerId(id));
+      id = syncable::Id::CreateFromServerId(id_string);
     }
-    if (response_map.end() == response_map.find(id))
-      response_map[id] = commit_response->add_entryresponse();
-    sync_pb::CommitResponse_EntryResponse* er = response_map[id];
+    committed_ids_.push_back(id);
+
+    if (response_map.end() == response_map.find(id_string))
+      response_map[id_string] = commit_response->add_entryresponse();
+    sync_pb::CommitResponse_EntryResponse* er = response_map[id_string];
     if (ShouldConflictThisCommit()) {
       er->set_response_type(CommitResponse::CONFLICT);
       continue;
     }
+    if (ShouldTransientErrorThisId(id)) {
+      er->set_response_type(CommitResponse::TRANSIENT_ERROR);
+      continue;
+    }
     er->set_response_type(CommitResponse::SUCCESS);
     er->set_version(entry.version() + 1);
     if (!commit_time_rename_prepended_string_.empty()) {
       // Commit time rename sent down from the server.
       er->set_name(commit_time_rename_prepended_string_ + entry.name());
     }
-    string parent_id = entry.parent_id_string();
+    string parent_id_string = entry.parent_id_string();
     // Remap id's we've already assigned.
-    if (changed_ids.end() != changed_ids.find(parent_id)) {
-      parent_id = changed_ids[parent_id];
-      er->set_parent_id_string(parent_id);
+    if (changed_ids.end() != changed_ids.find(parent_id_string)) {
+      parent_id_string = changed_ids[parent_id_string];
+      er->set_parent_id_string(parent_id_string);
     }
     if (entry.has_version() && 0 != entry.version()) {
-      er->set_id_string(id);  // Allows verification.
+      er->set_id_string(id_string);  // Allows verification.
     } else {
       string new_id = base::StringPrintf("mock_server:%d", next_new_id_++);
-      changed_ids[id] = new_id;
+      changed_ids[id_string] = new_id;
       er->set_id_string(new_id);
     }
   }
diff --git a/sync/test/engine/mock_connection_manager.h b/sync/test/engine/mock_connection_manager.h
index 0818b72..53b950bd 100644
--- a/sync/test/engine/mock_connection_manager.h
+++ b/sync/test/engine/mock_connection_manager.h
@@ -174,6 +174,8 @@ class MockConnectionManager : public ServerConnectionManager {
   void SetGUClientCommand(sync_pb::ClientCommand* command);
   void SetCommitClientCommand(sync_pb::ClientCommand* command);
 
+  void SetTransientErrorId(syncable::Id);
+
   const std::vector<syncable::Id>& committed_ids() const {
     return committed_ids_;
   }
@@ -284,6 +286,10 @@ class MockConnectionManager : public ServerConnectionManager {
   // Determine if one entry in a commit should be rejected with a conflict.
   bool ShouldConflictThisCommit();
 
+  // Determine if the given item's commit request should be refused with
+  // a TRANSIENT_ERROR response.
+  bool ShouldTransientErrorThisId(syncable::Id id);
+
   // Generate a numeric position_in_parent value.  We use a global counter
   // that only decreases; this simulates new objects always being added to the
   // front of the ordering.
@@ -315,6 +321,9 @@ class MockConnectionManager : public ServerConnectionManager {
   // All IDs that have been committed.
   std::vector<syncable::Id> committed_ids_;
 
+  // List of IDs which should return a transient error.
+  std::vector<syncable::Id> transient_error_ids_;
+
   // Control of when/if we return conflicts.
   bool conflict_all_commits_;
   int conflict_n_commits_;