Don't clear debug info until after it has been sent to the server.

Replaced DebugInfoGetter::GetAndClearDebugInfo with GetDebugInfo and ClearDebugInfo so we can clear the debug info only after we have successfully sent it to the server.

Moved MockDebugInfoGetter into its own file so it can be used by other tests.

BUG=319937

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

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

4 files changed, 86 insertions, 62 deletions

diff --git a/sync/engine/download.cc b/sync/engine/download.cc
index 53d36f5..7d00beb 100644
--- a/sync/engine/download.cc
+++ b/sync/engine/download.cc
@@ -94,11 +94,6 @@ void InitDownloadUpdatesContext(
   // (e.g. Bookmark URLs but not their containing folders).
   get_updates->set_fetch_folders(true);
 
-  sync_pb::DebugInfo* debug_info = message->mutable_debug_info();
-  AppendClientDebugInfoIfNeeded(session->context()->debug_info_getter(),
-                                session->mutable_status_controller(),
-                                debug_info);
-
   get_updates->set_create_mobile_bookmarks_folder(
       create_mobile_bookmarks_folder);
   bool need_encryption_key = ShouldRequestEncryptionKey(session->context());
@@ -344,6 +339,11 @@ SyncerError ExecuteDownloadUpdates(
   StatusController* status = session->mutable_status_controller();
   bool need_encryption_key = ShouldRequestEncryptionKey(session->context());
 
+  if (session->context()->debug_info_getter()) {
+    sync_pb::DebugInfo* debug_info = msg->mutable_debug_info();
+    CopyClientDebugInfo(session->context()->debug_info_getter(), debug_info);
+  }
+
   SyncerError result = SyncerProtoUtil::PostClientToServerMessage(
       msg,
       &update_response,
@@ -366,6 +366,12 @@ SyncerError ExecuteDownloadUpdates(
            << update_response.get_updates().changes_remaining()
            << " updates left on server.";
 
+  if (session->context()->debug_info_getter()) {
+    // Clear debug info now that we have successfully sent it to the server.
+    DVLOG(1) << "Clearing client debug info.";
+    session->context()->debug_info_getter()->ClearDebugInfo();
+  }
+
   if (need_encryption_key ||
       update_response.get_updates().encryption_keys_size() > 0) {
     syncable::Directory* dir = session->context()->directory();
@@ -392,20 +398,11 @@ SyncerError ExecuteDownloadUpdates(
   }
 }
 
-void AppendClientDebugInfoIfNeeded(
+void CopyClientDebugInfo(
     sessions::DebugInfoGetter* debug_info_getter,
-    StatusController* status,
     sync_pb::DebugInfo* debug_info) {
-  // We want to send the debug info only once per sync cycle. Check if it has
-  // already been sent.
-  if (!status->debug_info_sent()) {
-    DVLOG(1) << "Sending client debug info ...";
-    // Could be null in some unit tests.
-    if (debug_info_getter) {
-      debug_info_getter->GetAndClearDebugInfo(debug_info);
-    }
-    status->set_debug_info_sent();
-  }
+  DVLOG(1) << "Copying client debug info to send.";
+  debug_info_getter->GetDebugInfo(debug_info);
 }
 
 }  // namespace download
diff --git a/sync/engine/download.h b/sync/engine/download.h
index ec085c9..952823f 100644
--- a/sync/engine/download.h
+++ b/sync/engine/download.h
@@ -82,11 +82,10 @@ SYNC_EXPORT_PRIVATE SyncerError
                            sessions::SyncSession* session,
                            sync_pb::ClientToServerMessage* msg);
 
-// Helper function to append client debug info to the message, but only do so
-// once per sync cycle.  Defined here for testing.
-void SYNC_EXPORT_PRIVATE AppendClientDebugInfoIfNeeded(
+// Helper function to copy client debug info from debug_info_getter to
+// debug_info.  Defined here for testing.
+void SYNC_EXPORT_PRIVATE CopyClientDebugInfo(
     sessions::DebugInfoGetter* debug_info_getter,
-    sessions::StatusController* status,
     sync_pb::DebugInfo* debug_info);
 
 }  // namespace download
diff --git a/sync/engine/download_unittest.cc b/sync/engine/download_unittest.cc
index f38ab2b..9336893 100644
--- a/sync/engine/download_unittest.cc
+++ b/sync/engine/download_unittest.cc
@@ -14,10 +14,13 @@
 #include "sync/sessions/status_controller.h"
 #include "sync/syncable/directory.h"
 #include "sync/test/engine/test_directory_setter_upper.h"
+#include "sync/test/sessions/mock_debug_info_getter.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace syncer {
 
+using sessions::MockDebugInfoGetter;
+
 // A test fixture for tests exercising download updates functions.
 class DownloadUpdatesTest : public ::testing::Test {
  protected:
@@ -201,24 +204,6 @@ TEST_F(DownloadUpdatesTest, PollTest) {
   EXPECT_TRUE(proto_request_types().Equals(progress_types));
 }
 
-class MockDebugInfoGetter : public sessions::DebugInfoGetter {
- public:
-  MockDebugInfoGetter() {}
-  virtual ~MockDebugInfoGetter() {}
-
-  virtual void GetAndClearDebugInfo(sync_pb::DebugInfo* debug_info) OVERRIDE {
-    debug_info->CopyFrom(debug_info_);
-    debug_info_.Clear();
-  }
-
-  void AddDebugEvent() {
-    debug_info_.add_events();
-  }
-
- private:
-  sync_pb::DebugInfo debug_info_;
-};
-
 class DownloadUpdatesDebugInfoTest : public ::testing::Test {
  public:
   DownloadUpdatesDebugInfoTest() {}
@@ -242,33 +227,20 @@ class DownloadUpdatesDebugInfoTest : public ::testing::Test {
 };
 
 
-// Verify AppendDebugInfo when there are no events to upload.
-TEST_F(DownloadUpdatesDebugInfoTest, VerifyAppendDebugInfo_Empty) {
+// Verify CopyClientDebugInfo when there are no events to upload.
+TEST_F(DownloadUpdatesDebugInfoTest, VerifyCopyClientDebugInfo_Empty) {
   sync_pb::DebugInfo debug_info;
-  download::AppendClientDebugInfoIfNeeded(debug_info_getter(),
-                                          status(),
-                                          &debug_info);
+  download::CopyClientDebugInfo(debug_info_getter(), &debug_info);
   EXPECT_EQ(0, debug_info.events_size());
 }
 
-// We should upload debug info only once per sync cycle.
-TEST_F(DownloadUpdatesDebugInfoTest, TryDoubleAppend) {
-  sync_pb::DebugInfo debug_info1;
-
-  AddDebugEvent();
-  download::AppendClientDebugInfoIfNeeded(debug_info_getter(),
-                                          status(),
-                                          &debug_info1);
-  EXPECT_EQ(1, debug_info1.events_size());
-
-
-  // Repeated invocations should not send up more events.
+TEST_F(DownloadUpdatesDebugInfoTest, VerifyCopyOverwrites) {
+  sync_pb::DebugInfo debug_info;
   AddDebugEvent();
-  sync_pb::DebugInfo debug_info2;
-  download::AppendClientDebugInfoIfNeeded(debug_info_getter(),
-                                          status(),
-                                          &debug_info2);
-  EXPECT_EQ(0, debug_info2.events_size());
+  download::CopyClientDebugInfo(debug_info_getter(), &debug_info);
+  EXPECT_EQ(1, debug_info.events_size());
+  download::CopyClientDebugInfo(debug_info_getter(), &debug_info);
+  EXPECT_EQ(1, debug_info.events_size());
 }
 
 }  // namespace syncer
diff --git a/sync/engine/syncer_unittest.cc b/sync/engine/syncer_unittest.cc
index e31a08e..e4755b6 100644
--- a/sync/engine/syncer_unittest.cc
+++ b/sync/engine/syncer_unittest.cc
@@ -50,6 +50,7 @@
 #include "sync/test/engine/test_syncable_utils.h"
 #include "sync/test/fake_encryptor.h"
 #include "sync/test/fake_sync_encryption_handler.h"
+#include "sync/test/sessions/mock_debug_info_getter.h"
 #include "sync/util/cryptographer.h"
 #include "sync/util/extensions_activity.h"
 #include "sync/util/time.h"
@@ -104,6 +105,7 @@ using syncable::SPECIFICS;
 using syncable::SYNCING;
 using syncable::UNITTEST;
 
+using sessions::MockDebugInfoGetter;
 using sessions::StatusController;
 using sessions::SyncSessionContext;
 using sessions::SyncSession;
@@ -204,6 +206,7 @@ class SyncerTest : public testing::Test,
     dir_maker_.SetUp();
     mock_server_.reset(new MockConnectionManager(directory(),
                                                  &cancelation_signal_));
+    debug_info_getter_.reset(new MockDebugInfoGetter);
     EnableDatatype(BOOKMARKS);
     EnableDatatype(NIGORI);
     EnableDatatype(PREFERENCES);
@@ -222,7 +225,7 @@ class SyncerTest : public testing::Test,
         new SyncSessionContext(
             mock_server_.get(), directory(), workers,
             extensions_activity_,
-            listeners, NULL, &traffic_recorder_,
+            listeners, debug_info_getter_.get(), &traffic_recorder_,
             true,  // enable keystore encryption
             false,  // force enable pre-commit GU avoidance experiment
             "fake_invalidator_client_id"));
@@ -490,6 +493,7 @@ class SyncerTest : public testing::Test,
   ModelTypeSet enabled_datatypes_;
   TrafficRecorder traffic_recorder_;
   sessions::NudgeTracker nudge_tracker_;
+  scoped_ptr<MockDebugInfoGetter> debug_info_getter_;
 
   DISALLOW_COPY_AND_ASSIGN(SyncerTest);
 };
@@ -2515,6 +2519,58 @@ TEST_F(SyncerTest, CommitManyItemsInOneGo_CommitConflict) {
             directory()->unsynced_entity_count());
 }
 
+// Tests that sending debug info events works.
+TEST_F(SyncerTest, SendDebugInfoEvents_HappyCase) {
+  debug_info_getter_->AddDebugEvent();
+  debug_info_getter_->AddDebugEvent();
+
+  SyncShareNudge();
+
+  // Verify we received one request with two debug info events.
+  EXPECT_EQ(1U, mock_server_->requests().size());
+  EXPECT_EQ(2, mock_server_->requests().back().debug_info().events_size());
+
+  SyncShareNudge();
+
+  // See that we received another request, but that it contains no debug info
+  // events.
+  EXPECT_EQ(2U, mock_server_->requests().size());
+  EXPECT_EQ(0, mock_server_->requests().back().debug_info().events_size());
+
+  debug_info_getter_->AddDebugEvent();
+
+  SyncShareNudge();
+
+  // See that we received another request and it contains one debug info event.
+  EXPECT_EQ(3U, mock_server_->requests().size());
+  EXPECT_EQ(1, mock_server_->requests().back().debug_info().events_size());
+}
+
+// Tests that debug info events are dropped on server error.
+TEST_F(SyncerTest, SendDebugInfoEvents_PostFailsDontDrop) {
+  debug_info_getter_->AddDebugEvent();
+  debug_info_getter_->AddDebugEvent();
+
+  mock_server_->FailNextPostBufferToPathCall();
+  SyncShareNudge();
+
+  // Verify we attempted to send one request with two debug info events.
+  EXPECT_EQ(1U, mock_server_->requests().size());
+  EXPECT_EQ(2, mock_server_->requests().back().debug_info().events_size());
+
+  SyncShareNudge();
+
+  // See that the client resent the two debug info events.
+  EXPECT_EQ(2U, mock_server_->requests().size());
+  EXPECT_EQ(2, mock_server_->requests().back().debug_info().events_size());
+
+  // The previous send was successful so this next one shouldn't generate any
+  // debug info events.
+  SyncShareNudge();
+  EXPECT_EQ(3U, mock_server_->requests().size());
+  EXPECT_EQ(0, mock_server_->requests().back().debug_info().events_size());
+}
+
 TEST_F(SyncerTest, HugeConflict) {
   int item_count = 300;  // We should be able to do 300 or 3000 w/o issue.