Fix [D]CHECK failure in local sync

ModelTypeController assumes that the user must be signed in by the time
a datatype gets started. This however doesn't hold true for local sync,
so we relax the requirement.

Some datatypes have similar requirements in their bridge
implementations, so we disable those datatypes for local sync.

The patch introduces first browser tests for local sync, which would
have caught such a basic issue.

Bug: 887250
Change-Id: I1bca79478958ac64ee5502e4795f8619ee50e85c
Reviewed-on: https://chromium-review.googlesource.com/1235935
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592792}

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

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <memory>
+
+#include "base/command_line.h"
+#include "base/macros.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/sync/profile_sync_service_factory.h"
+#include "chrome/browser/sync/test/integration/single_client_status_change_checker.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "components/browser_sync/browser_sync_switches.h"
+#include "components/browser_sync/profile_sync_service.h"
+#include "components/sync/driver/sync_driver_switches.h"
+
+namespace {
+
+using browser_sync::ProfileSyncService;
+
+class SyncActiveChecker : public SingleClientStatusChangeChecker {
+ public:
+  explicit SyncActiveChecker(ProfileSyncService* service)
+      : SingleClientStatusChangeChecker(service) {}
+
+  bool IsExitConditionSatisfied() override {
+    return service()->GetTransportState() ==
+           syncer::SyncService::TransportState::ACTIVE;
+  }
+
+  std::string GetDebugMessage() const override { return "Sync Active"; }
+};
+
+// This test verifies some basic functionality of local sync, used for roaming
+// profiles (enterprise use-case).
+class LocalSyncTest : public InProcessBrowserTest {
+ protected:
+  LocalSyncTest() {}
+
+  ~LocalSyncTest() override {}
+
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    command_line->AppendSwitch(switches::kEnableLocalSyncBackend);
+    command_line->AppendSwitchASCII(
+        switches::kSyncDeferredStartupTimeoutSeconds, "1");
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(LocalSyncTest);
+};
+
+IN_PROC_BROWSER_TEST_F(LocalSyncTest, ShouldStart) {
+  ProfileSyncService* service =
+      ProfileSyncServiceFactory::GetForProfile(browser()->profile());
+
+  // Wait until the first sync cycle is completed.
+  ASSERT_TRUE(SyncActiveChecker(service).Wait());
+
+  EXPECT_TRUE(service->IsLocalSyncEnabled());
+}
+
+}  // namespace

chrome/test/BUILD.gn

@@ -5449,6 +5449,7 @@ if (!is_android && !is_fuchsia) {
     sources = [
       "../app/chrome_version.rc.version",
       "../browser/sync/test/integration/enable_disable_test.cc",
+      "../browser/sync/test/integration/local_sync_test.cc",
       "../browser/sync/test/integration/migration_test.cc",
       "../browser/sync/test/integration/single_client_app_list_sync_test.cc",
       "../browser/sync/test/integration/single_client_apps_sync_test.cc",

components/browser_sync/profile_sync_service.cc

@@ -994,6 +994,11 @@ void ProfileSyncService::OnEngineInitialized(
     bool success) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
+  // TODO(treib): Based on some crash reports, it seems like the user could have
+  // signed out already at this point, so many of the steps below, including
+  // datatype reconfiguration, should not be triggered.
+  DCHECK(IsEngineAllowedToStart());
+
   // The very first time the backend initializes is effectively the first time
   // we can say we successfully "synced".  LastSyncedTime will only be null in
   // this case, because the pref wasn't restored on StartUp.
@@ -1516,10 +1521,15 @@ bool ProfileSyncService::HasObserver(
 syncer::ModelTypeSet ProfileSyncService::GetPreferredDataTypes() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   const syncer::ModelTypeSet registered_types = GetRegisteredDataTypes();
-  const syncer::ModelTypeSet preferred_types =
+  syncer::ModelTypeSet preferred_types =
       Union(sync_prefs_.GetPreferredDataTypes(registered_types,
                                               user_events_separate_pref_group_),
             syncer::ControlTypes());
+  if (IsLocalSyncEnabled()) {
+    preferred_types.Remove(syncer::APP_LIST);
+    preferred_types.Remove(syncer::USER_CONSENTS);
+    preferred_types.Remove(syncer::USER_EVENTS);
+  }
   const syncer::ModelTypeSet enforced_types =
       Intersection(GetDataTypesFromPreferenceProviders(), registered_types);
   return Union(preferred_types, enforced_types);

components/sync/driver/model_type_controller.cc

@@ -96,8 +96,8 @@ void ModelTypeController::LoadModels(
   request.authenticated_account_id = configure_context.authenticated_account_id;
   request.cache_guid = configure_context.cache_guid;
 
-  CHECK(!request.authenticated_account_id.empty());
-  CHECK(!request.cache_guid.empty());
+  // Note that |request.authenticated_account_id| may be empty for local sync.
+  DCHECK(!request.cache_guid.empty());
 
   // Ask the delegate to actually start the datatype.
   delegate_->OnSyncStarting(