Fix for negative routing info problem. We were replacing tokens after

failure multiple times.

BUG=39214
TEST=included new fancy unit test

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

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

5 files changed, 125 insertions, 16 deletions

diff --git a/chrome/browser/sync/engine/build_commit_command.cc b/chrome/browser/sync/engine/build_commit_command.cc
index cf6b4f53..d0dc64b 100644
--- a/chrome/browser/sync/engine/build_commit_command.cc
+++ b/chrome/browser/sync/engine/build_commit_command.cc
@@ -37,9 +37,13 @@ void BuildCommitCommand::AddExtensionsActivityToMessage(
     SyncSession* session, CommitMessage* message) {
   // We only send ExtensionsActivity to the server if bookmarks are being
   // committed.
-  if (!session->status_controller()->HasBookmarkCommitActivity())
+  ExtensionsActivityMonitor* monitor = session->context()->extensions_monitor();
+  if (!session->status_controller()->HasBookmarkCommitActivity()) {
+    // Return the records to the activity monitor.
+    monitor->PutRecords(session->extensions_activity());
+    session->mutable_extensions_activity()->clear();
     return;
-
+  }
   const ExtensionsActivityMonitor::Records& records =
       session->extensions_activity();
   for (ExtensionsActivityMonitor::Records::const_iterator it = records.begin();
diff --git a/chrome/browser/sync/engine/process_commit_response_command.cc b/chrome/browser/sync/engine/process_commit_response_command.cc
index 0db5def..3abe92f 100644
--- a/chrome/browser/sync/engine/process_commit_response_command.cc
+++ b/chrome/browser/sync/engine/process_commit_response_command.cc
@@ -100,6 +100,7 @@ void ProcessCommitResponseCommand::ModelChangingExecuteImpl(
       session->status_controller()->syncer_status()
           .num_successful_bookmark_commits == 0) {
     monitor->PutRecords(session->extensions_activity());
+    session->mutable_extensions_activity()->clear();
   }
 }
 
@@ -196,9 +197,9 @@ void ProcessCommitResponseCommand::ProcessCommitResponse(
 
 void LogServerError(const CommitResponse_EntryResponse& res) {
   if (res.has_error_message())
-    LOG(ERROR) << "  " << res.error_message();
+    LOG(WARNING) << "  " << res.error_message();
   else
-    LOG(ERROR) << "  No detailed error message returned from server";
+    LOG(WARNING) << "  No detailed error message returned from server";
 }
 
 CommitResponse::ResponseType
diff --git a/chrome/browser/sync/engine/process_commit_response_command_unittest.cc b/chrome/browser/sync/engine/process_commit_response_command_unittest.cc
index 70c6732..f313a41 100644
--- a/chrome/browser/sync/engine/process_commit_response_command_unittest.cc
+++ b/chrome/browser/sync/engine/process_commit_response_command_unittest.cc
@@ -34,7 +34,9 @@ using syncable::UNITTEST;
 using syncable::WriteTransaction;
 
 // A test fixture for tests exercising ProcessCommitResponseCommand.
-class ProcessCommitResponseCommandTest : public SyncerCommandTest {
+template<typename T>
+class ProcessCommitResponseCommandTestWithParam
+    : public SyncerCommandTestWithParam<T> {
  public:
   virtual void SetUp() {
     workers()->clear();
@@ -47,11 +49,18 @@ class ProcessCommitResponseCommandTest : public SyncerCommandTest {
     (*mutable_routing_info())[syncable::AUTOFILL] = GROUP_PASSIVE;
 
     commit_set_.reset(new sessions::OrderedCommitSet(routing_info()));
-    SyncerCommandTest::SetUp();
+    SyncerCommandTestWithParam<T>::SetUp();
   }
 
  protected:
-  ProcessCommitResponseCommandTest()
+  using SyncerCommandTestWithParam<T>::context;
+  using SyncerCommandTestWithParam<T>::mutable_routing_info;
+  using SyncerCommandTestWithParam<T>::routing_info;
+  using SyncerCommandTestWithParam<T>::session;
+  using SyncerCommandTestWithParam<T>::syncdb;
+  using SyncerCommandTestWithParam<T>::workers;
+
+  ProcessCommitResponseCommandTestWithParam()
       : next_old_revision_(1),
         next_new_revision_(4000),
         next_server_position_(10000) {
@@ -172,6 +181,16 @@ class ProcessCommitResponseCommandTest : public SyncerCommandTest {
     }
   }
 
+  void SetLastErrorCode(CommitResponse::ResponseType error_code) {
+    sessions::StatusController* sync_state = session()->status_controller();
+    sync_pb::ClientToServerResponse* response =
+        sync_state->mutable_commit_response();
+    sync_pb::CommitResponse_EntryResponse* entry_response =
+        response->mutable_commit()->mutable_entryresponse(
+            response->mutable_commit()->entryresponse_size() - 1);
+    entry_response->set_response_type(error_code);
+  }
+
   ProcessCommitResponseCommand command_;
   TestIdFactory id_factory_;
   scoped_ptr<sessions::OrderedCommitSet> commit_set_;
@@ -179,9 +198,12 @@ class ProcessCommitResponseCommandTest : public SyncerCommandTest {
   int64 next_old_revision_;
   int64 next_new_revision_;
   int64 next_server_position_;
-  DISALLOW_COPY_AND_ASSIGN(ProcessCommitResponseCommandTest);
+  DISALLOW_COPY_AND_ASSIGN(ProcessCommitResponseCommandTestWithParam);
 };
 
+class ProcessCommitResponseCommandTest
+    : public ProcessCommitResponseCommandTestWithParam<void*> {};
+
 TEST_F(ProcessCommitResponseCommandTest, MultipleCommitIdProjections) {
   Id bookmark_folder_id = id_factory_.NewLocalId();
   Id bookmark_id1 = id_factory_.NewLocalId();
@@ -344,4 +366,71 @@ 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 ProcessCommitResponseCommandTestWithParam<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) {
+  EXPECT_NE(routing_info().find(syncable::BOOKMARKS)->second,
+            routing_info().find(syncable::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", syncable::BOOKMARKS);
+  if (ShouldFailBookmarkCommit())
+    SetLastErrorCode(CommitResponse::TRANSIENT_ERROR);
+  // Autofill item setup.
+  CreateUnprocessedCommitResult(id_factory_.NewServerId(),
+      id_factory_.root(), "Some autofill", syncable::AUTOFILL);
+  if (ShouldFailAutofillCommit())
+    SetLastErrorCode(CommitResponse::TRANSIENT_ERROR);
+
+  // 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;
+  }
+  command_.ExecuteImpl(session());
+
+  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 browser_sync
diff --git a/chrome/browser/sync/sessions/status_controller.h b/chrome/browser/sync/sessions/status_controller.h
index 87d30df..ac7cbba 100644
--- a/chrome/browser/sync/sessions/status_controller.h
+++ b/chrome/browser/sync/sessions/status_controller.h
@@ -151,9 +151,10 @@ class StatusController {
   int64 CountUpdates() const;
 
   // Returns true iff any of the commit ids added during this session are
-  // bookmark related.
+  // bookmark related, and the bookmark group restriction is in effect.
   bool HasBookmarkCommitActivity() const {
-    return shared_.commit_set.HasBookmarkCommitId();
+    return ActiveGroupRestrictionIncludesModel(syncable::BOOKMARKS) &&
+        shared_.commit_set.HasBookmarkCommitId();
   }
 
   // Returns true if the last download_updates_command received a valid
@@ -178,6 +179,17 @@ class StatusController {
     return group_restriction_;
   }
 
+  // Check whether a particular model is included by the active group
+  // restriction.
+  bool ActiveGroupRestrictionIncludesModel(syncable::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;
+  }
+
   // A toolbelt full of methods for updating counters and flags.
   void increment_num_conflicting_commits_by(int value);
   void reset_num_conflicting_commits();
diff --git a/chrome/test/sync/engine/syncer_command_test.h b/chrome/test/sync/engine/syncer_command_test.h
index a8814ac..92175ed 100644
--- a/chrome/test/sync/engine/syncer_command_test.h
+++ b/chrome/test/sync/engine/syncer_command_test.h
@@ -18,9 +18,10 @@ namespace browser_sync {
 // A test fixture that simplifies writing unit tests for individual
 // SyncerCommands, providing convenient access to a test directory
 // and a syncer session.
-class SyncerCommandTest : public testing::Test,
-                          public sessions::SyncSession::Delegate,
-                          public ModelSafeWorkerRegistrar {
+template<typename T>
+class SyncerCommandTestWithParam : public testing::TestWithParam<T>,
+                                   public sessions::SyncSession::Delegate,
+                                   public ModelSafeWorkerRegistrar {
  public:
   // SyncSession::Delegate implementation.
   virtual void OnSilencedUntil(const base::TimeTicks& silenced_until) {
@@ -51,8 +52,8 @@ class SyncerCommandTest : public testing::Test,
   }
 
  protected:
-  SyncerCommandTest() {}
-  virtual ~SyncerCommandTest() {}
+  SyncerCommandTestWithParam() {}
+  virtual ~SyncerCommandTestWithParam() {}
   virtual void SetUp() {
     syncdb_.SetUp();
     context_.reset(new sessions::SyncSessionContext(NULL, NULL,
@@ -90,9 +91,11 @@ class SyncerCommandTest : public testing::Test,
   scoped_ptr<sessions::SyncSession> session_;
   std::vector<scoped_refptr<ModelSafeWorker> > workers_;
   ModelSafeRoutingInfo routing_info_;
-  DISALLOW_COPY_AND_ASSIGN(SyncerCommandTest);
+  DISALLOW_COPY_AND_ASSIGN(SyncerCommandTestWithParam);
 };
 
+class SyncerCommandTest : public SyncerCommandTestWithParam<void*> {};
+
 }  // namespace browser_sync
 
 #endif  // CHROME_TEST_SYNC_ENGINE_SYNCER_COMMAND_TEST_H_