Reland "Fix sync protocol violation about birthday requirements"

This is a reland of 956715c7

Original change's description:
> 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/+/1599456
> Reviewed-by: Marc Treib <treib@chromium.org>
> Commit-Queue: Mikel Astiz <mastiz@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#658540}

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

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.