[Sync] Fix a crash in StopSyncHelperOnModelThread()

This is a new crash with the DestroyProfileOnBrowserClose experiment
(go/destroy-profile-on-browser-close). The BookmarkSyncService uses a
ProxyModelTypeControllerDelegate, with the UI thread as the model
thread. This causes issues, where posted tasks run _after_ the
BookmarkSyncService gets deleted.

This patch switches it to a ForwardingModelTypeControllerDelegate,
so the tasks run synchronously instead of posting to the UI thread. This
fixes teardown order.

Bug: 1140232, 88586
Change-Id: I0802334ce3da56326c167f7951d512fe045ddece
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2487222
Commit-Queue: Nicolas Ouellet-Payeur <nicolaso@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#820229}

chrome/browser/sync/profile_sync_service_factory_unittest.cc

@@ -12,6 +12,8 @@
 #include "base/feature_list.h"
 #include "base/task/thread_pool/thread_pool_instance.h"
 #include "build/build_config.h"
+#include "chrome/browser/favicon/favicon_service_factory.h"
+#include "chrome/browser/history/history_service_factory.h"
 #include "chrome/common/buildflags.h"
 #include "chrome/test/base/testing_profile.h"
 #include "components/browser_sync/browser_sync_switches.h"
@@ -42,7 +44,12 @@ class ProfileSyncServiceFactoryTest : public testing::Test {
 #if defined(OS_CHROMEOS)
     app_list::AppListSyncableServiceFactory::SetUseInTesting(true);
 #endif  // defined(OS_CHROMEOS)
-    profile_ = std::make_unique<TestingProfile>();
+    TestingProfile::Builder builder;
+    builder.AddTestingFactory(FaviconServiceFactory::GetInstance(),
+                              FaviconServiceFactory::GetDefaultFactory());
+    builder.AddTestingFactory(HistoryServiceFactory::GetInstance(),
+                              HistoryServiceFactory::GetDefaultFactory());
+    profile_ = builder.Build();
     // Some services will only be created if there is a WebDataService.
     profile_->CreateWebDataService();
   }

components/browser_sync/profile_sync_components_factory_impl.cc

@@ -244,14 +244,17 @@ ProfileSyncComponentsFactoryImpl::CreateCommonDataTypeControllers(
   // Bookmark sync is enabled by default.  Register unless explicitly
   // disabled.
   if (!disabled_types.Has(syncer::BOOKMARKS)) {
+    favicon::FaviconService* favicon_service =
+        sync_client_->GetFaviconService();
+    // Services can be null in tests.
+    if (bookmark_sync_service_ && favicon_service) {
       controllers.push_back(std::make_unique<ModelTypeController>(
           syncer::BOOKMARKS,
-          std::make_unique<syncer::ProxyModelTypeControllerDelegate>(
-              ui_thread_,
-              base::BindRepeating(&sync_bookmarks::BookmarkSyncService::
-                                      GetBookmarkSyncControllerDelegate,
-                                  base::Unretained(bookmark_sync_service_),
-                                  sync_client_->GetFaviconService()))));
+          std::make_unique<syncer::ForwardingModelTypeControllerDelegate>(
+              bookmark_sync_service_
+                  ->GetBookmarkSyncControllerDelegate(favicon_service)
+                  .get())));
+    }
   }
 
   // These features are enabled only if history is not disabled.

ios/chrome/browser/sync/BUILD.gn

@@ -143,6 +143,7 @@ source_set("unit_tests") {
     "//components/sync",
     "//ios/chrome/browser",
     "//ios/chrome/browser/browser_state:test_support",
+    "//ios/chrome/browser/favicon",
     "//ios/web/public/test",
     "//testing/gtest",
     "//ui/base",

ios/chrome/browser/sync/profile_sync_service_factory_unittest.cc

@@ -17,6 +17,7 @@
 #include "components/sync/driver/profile_sync_service.h"
 #include "components/sync/driver/sync_driver_switches.h"
 #include "ios/chrome/browser/browser_state/test_chrome_browser_state.h"
+#include "ios/chrome/browser/favicon/favicon_service_factory.h"
 #include "ios/web/public/test/web_task_environment.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "testing/platform_test.h"
@@ -27,7 +28,12 @@ class ProfileSyncServiceFactoryTest : public PlatformTest {
  public:
   ProfileSyncServiceFactoryTest() {
     TestChromeBrowserState::Builder browser_state_builder;
+    // BOOKMARKS requires the FaviconService, which requires the HistoryService.
+    browser_state_builder.AddTestingFactory(
+        ios::FaviconServiceFactory::GetInstance(),
+        ios::FaviconServiceFactory::GetDefaultFactory());
     chrome_browser_state_ = browser_state_builder.Build();
+    CHECK(chrome_browser_state_->CreateHistoryService());
   }
 
   void SetUp() override {