Fix sync protocol violation about birthday requirements

The sync (store) birthday can only be empty during the very initial
interactions with the server, when control datatypes are downloaded,
after enabling sync (or the user signing in, for sync-the-transport
mode).

If, during startup, the birthday is empty, we should throw away all
sync metadata and redownload everything. One way to do so is to
regenerate the cache GUID, which datatypes detect as mismatch and
leads to all sync metadata being cleared.

As can seen in newly added tests, the scenario seems hard to
reproduce, but we have server-side evidence that some users run into
this issue (and would even DCHECK-fail for DCHECK-enabled builds).

Bug: 923285
Change-Id: If9e4807a086001931fd65f3e12a1ab53c4a33c9d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1599456Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#658540}

chrome/browser/sync/test/integration/enable_disable_test.cc

@@ -22,6 +22,7 @@
 #include "components/sync/driver/sync_user_settings_impl.h"
 #include "components/sync/test/fake_server/bookmark_entity_builder.h"
 #include "components/sync/test/fake_server/entity_builder_factory.h"
+#include "components/sync/test/fake_server/fake_server_http_post_provider.h"
 
 namespace {
 
@@ -442,6 +443,39 @@ IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest,
   EXPECT_EQ(cache_guid, prefs.GetCacheGuid());
 }
 
+IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest,
+                       PRE_RestartDuringInitialSync) {
+  SetupSyncNoWaitingForCompletion();
+
+  // The intended scenario being tested involves an ongoing initial sync. This
+  // means a cache GUID has been generated, but the initial birthday hasn't been
+  // received yet (which is received from the server as part of the initial
+  // GetUpdates request).
+  SyncPrefs prefs(GetProfile(0)->GetPrefs());
+  ASSERT_NE("", prefs.GetCacheGuid());
+  ASSERT_EQ("", prefs.GetBirthday());
+
+  // Mimic a non-responding server (slow connection) during shutdown, to make
+  // sure test teardown logic doesn't fetch the birthday. This is asserted in
+  // the next test and would otherwise fail.
+  fake_server::FakeServerHttpPostProvider::SetNetworkDelay(
+      base::TimeDelta::Max());
+}
+
+IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest,
+                       RestartDuringInitialSync) {
+  ASSERT_TRUE(SetupClients());
+
+  SyncPrefs prefs(GetProfile(0)->GetPrefs());
+  ASSERT_NE("", prefs.GetCacheGuid());
+  ASSERT_EQ("", prefs.GetBirthday());
+
+  EXPECT_TRUE(GetClient(0)->AwaitSyncSetupCompletion());
+
+  ASSERT_NE("", prefs.GetCacheGuid());
+  EXPECT_NE("", prefs.GetBirthday());
+}
+
 class EnableDisableSingleClientSelfNotifyTest
     : public EnableDisableSingleClientTest {
  public:

components/sync/driver/profile_sync_service.cc

@@ -484,17 +484,25 @@ void ProfileSyncService::StartUpSlowEngineComponents() {
   }
   params.sync_manager_factory =
       std::make_unique<SyncManagerFactory>(network_connection_tracker_);
-  // The first time we start up the engine we want to ensure we have a clean
-  // directory, so delete any old one that might be there.
-  params.delete_sync_data_folder = !user_settings_->IsFirstSetupComplete();
-  if (params.delete_sync_data_folder) {
-    // This looks questionable here but it mimics the old behavior of deleting
-    // the directory via Directory::DeleteDirectoryFiles(). One consecuence is
-    // that, for sync the transport users (without sync-the-feature enabled),
-    // the cache GUID and other fields are reset on every restart.
-    // TODO(crbug.com/923285): Reconsider the lifetime of the cache GUID and
-    // its persistence depending on StorageOption.
+  params.birthday = sync_prefs_.GetBirthday();
+  params.cache_guid = sync_prefs_.GetCacheGuid();
+  // If either the cache GUID or the birthday are unitialized, it means we
+  // haven't completed a first sync cycle. We regenerate the cache GUID either
+  // way, even if just the birthday is missing, because fetching updates
+  // requires that the request either has a birthday, or there should be no
+  // progress marker.
+  // TODO(crbug.com/923285): This looks questionable here for
+  // |!IsFirstSetupComplete()| but it mimics the old behavior of deleting the
+  // directory via Directory::DeleteDirectoryFiles(). One consecuence is that,
+  // for sync the transport users (without sync-the-feature enabled), the cache
+  // GUID and other fields are reset on every restart.
+  if (params.cache_guid.empty() || params.birthday.empty() ||
+      !user_settings_->IsFirstSetupComplete()) {
     sync_prefs_.ClearDirectoryConsistencyPreferences();
+    params.cache_guid = GenerateCacheGUID();
+    params.birthday.clear();
+    params.delete_sync_data_folder = true;
+    sync_prefs_.SetCacheGuid(params.cache_guid);
   }
   params.enable_local_sync_backend = sync_prefs_.IsLocalSyncEnabled();
   params.local_sync_backend_folder = sync_client_->GetLocalSyncBackendFolder();
@@ -502,12 +510,7 @@ void ProfileSyncService::StartUpSlowEngineComponents() {
       sync_prefs_.GetEncryptionBootstrapToken();
   params.restored_keystore_key_for_bootstrapping =
       sync_prefs_.GetKeystoreEncryptionBootstrapToken();
-  params.cache_guid = sync_prefs_.GetCacheGuid();
-  if (params.cache_guid.empty()) {
-    params.cache_guid = GenerateCacheGUID();
-    sync_prefs_.SetCacheGuid(params.cache_guid);
-  }
-  params.birthday = sync_prefs_.GetBirthday();
+
   params.bag_of_chips = sync_prefs_.GetBagOfChips();
   params.engine_components_factory =
       std::make_unique<EngineComponentsFactoryImpl>(
@@ -859,6 +862,10 @@ void ProfileSyncService::OnEngineInitialized(
     return;
   }
 
+  // TODO(crbug.com/923285): The store birthday should be available at this
+  // point, so we should ideally save to prefs, since OnSyncCycleCompleted()
+  // does not get called for the first sync cycle for control types.
+
   sync_js_controller_.AttachJsBackend(js_backend);
 
   if (protocol_event_observers_.might_have_observers()) {

components/sync/engine_impl/loopback_server/loopback_server.cc

@@ -344,9 +344,9 @@ net::HttpStatusCode LoopbackServer::HandleCommand(const string& request,
     std::vector<ModelType> datatypes_to_migrate;
     switch (message.message_contents()) {
       case sync_pb::ClientToServerMessage::GET_UPDATES:
-        success = HandleGetUpdatesRequest(message.get_updates(),
-                                          response_proto.mutable_get_updates(),
-                                          &datatypes_to_migrate);
+        success = HandleGetUpdatesRequest(
+            message.get_updates(), message.store_birthday(),
+            response_proto.mutable_get_updates(), &datatypes_to_migrate);
         break;
       case sync_pb::ClientToServerMessage::COMMIT:
         success = HandleCommitRequest(message.commit(),
@@ -394,10 +394,25 @@ void LoopbackServer::EnableStrongConsistencyWithConflictDetectionModel() {
 
 bool LoopbackServer::HandleGetUpdatesRequest(
     const sync_pb::GetUpdatesMessage& get_updates,
+    const std::string& store_birthday,
     sync_pb::GetUpdatesResponse* response,
     std::vector<ModelType>* datatypes_to_migrate) {
   response->set_changes_remaining(0);
 
+  // It's a protocol-level contract that the birthday should only be empty
+  // during the initial sync cycle, which requires all progress markers to be
+  // empty. This is also DCHECK-ed on the client, inside syncer_proto_util.cc,
+  // but we guard against client-side code changes here.
+  if (store_birthday.empty()) {
+    for (const sync_pb::DataTypeProgressMarker& marker :
+         get_updates.from_progress_marker()) {
+      if (!marker.token().empty()) {
+        DLOG(WARNING) << "Non-empty progress marker without birthday";
+        return false;
+      }
+    }
+  }
+
   auto sieve = std::make_unique<UpdateSieve>(get_updates, migration_versions_);
 
   if (sieve->ShouldTriggerMigration(migration_versions_,

components/sync/engine_impl/loopback_server/loopback_server.h

@@ -89,6 +89,7 @@ class LoopbackServer {
 
   // Processes a GetUpdates call.
   bool HandleGetUpdatesRequest(const sync_pb::GetUpdatesMessage& get_updates,
+                               const std::string& store_birthday,
                                sync_pb::GetUpdatesResponse* response,
                                std::vector<ModelType>* datatypes_to_migrate);
 

components/sync/test/fake_server/fake_server_http_post_provider.cc

@@ -8,7 +8,6 @@
 
 #include "base/bind.h"
 #include "base/location.h"
-#include "base/time/time.h"
 #include "components/sync/test/fake_server/fake_server.h"
 #include "net/base/net_errors.h"
 
@@ -17,6 +16,10 @@ namespace fake_server {
 // static
 std::atomic_bool FakeServerHttpPostProvider::network_enabled_(true);
 
+// static
+std::atomic<base::TimeDelta> FakeServerHttpPostProvider::network_delay_ = {
+    base::TimeDelta()};
+
 FakeServerHttpPostProviderFactory::FakeServerHttpPostProviderFactory(
     const base::WeakPtr<FakeServer>& fake_server,
     scoped_refptr<base::SequencedTaskRunner> fake_server_task_runner)
@@ -89,6 +92,17 @@ bool FakeServerHttpPostProvider::MakeSynchronousPost(int* net_error_code,
   synchronous_post_completion_.Reset();
   aborted_ = false;
 
+  const base::TimeDelta network_delay = network_delay_.load();
+  if (!network_delay.is_zero()) {
+    // Block the specified time unless Abort() is called meanwhile.
+    synchronous_post_completion_.TimedWait(network_delay);
+    if (aborted_) {
+      *net_error_code = net::ERR_ABORTED;
+      return false;
+    }
+    DCHECK(!synchronous_post_completion_.IsSignaled());
+  }
+
   // It is assumed that a POST is being made to /command.
   int post_status_code = -1;
   std::string post_response;
@@ -160,6 +174,12 @@ void FakeServerHttpPostProvider::EnableNetwork() {
   network_enabled_ = true;
 }
 
+// static
+void FakeServerHttpPostProvider::SetNetworkDelay(base::TimeDelta delay) {
+  // Note: This may be called on any thread.
+  network_delay_ = delay;
+}
+
 void FakeServerHttpPostProvider::HandleCommandOnFakeServerThread(
     int* http_status_code,
     std::string* response) {

components/sync/test/fake_server/fake_server_http_post_provider.h

@@ -15,6 +15,7 @@
 #include "base/sequence_checker.h"
 #include "base/sequenced_task_runner.h"
 #include "base/synchronization/waitable_event.h"
+#include "base/time/time.h"
 #include "components/sync/engine/net/http_post_provider_factory.h"
 #include "components/sync/engine/net/http_post_provider_interface.h"
 
@@ -50,6 +51,9 @@ class FakeServerHttpPostProvider
   // Undoes the effects of DisableNetwork.
   static void EnableNetwork();
 
+  // Mimics a slow network: each request will be blocked for the specified time.
+  static void SetNetworkDelay(base::TimeDelta delay);
+
  protected:
   ~FakeServerHttpPostProvider() override;
 
@@ -60,6 +64,7 @@ class FakeServerHttpPostProvider
                                        std::string* response);
 
   static std::atomic_bool network_enabled_;
+  static std::atomic<base::TimeDelta> network_delay_;
 
   // |fake_server_| should only be dereferenced on the same thread as
   // |fake_server_task_runner_| runs on.