WebApp: Fix DCHECK in WebAppInstallFinalizer::FinalizeFallbackInstallAfterSync

We have a logical race:
1) The bookmark app object arrives from the server. It doesn't
create any registry entries but only enqueues full web application
installation (loading url, manifest, icons etc). It gets queued 1st.
2) The web app object arrives from the server. It immediately
creates |is_in_sync_install| web app placeholder with all the sync
data committed to in-memory registry and leveldb database. Then
it enqueues full web application installation (loading url, manifest,
icons etc). It gets queued 2nd.
3) The bookmark app task finishes full install first with a success.
|FinalizeInstall| overwrites the |is_in_sync_install| web app placeholder
with real data from the web site. WebAppInstallManager proceeds
to next task in the queue.
4) The web app task fails to load the url. It tries to do a fallback
install: it does |FinalizeFallbackInstallAfterSync| instead of
|FinalizeInstall|. DCHECK fails because the web app entry was
overwritten at step 3.

This is tested in
SyncRace_InstallBookmarkAppFullThenWebAppFallback
unit test.

In next CLs: Fix other races and scenarios. Add unit tests.

Bug: 1084392
Change-Id: I7196e74e289bfa516a0ca4e455084a7ea6ca8932
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2208790
Commit-Queue: Alexey Baskakov <loyso@chromium.org>
Reviewed-by: Alan Cutter <alancutter@chromium.org>
Reviewed-by: Eric Willigers <ericwilligers@chromium.org>
Cr-Commit-Position: refs/heads/master@{#770067}

chrome/browser/web_applications/test/test_web_app_registry_controller.cc

@@ -6,10 +6,12 @@
 
 #include "base/run_loop.h"
 #include "base/test/bind_test_util.h"
+#include "chrome/browser/web_applications/components/web_app_helpers.h"
 #include "chrome/browser/web_applications/test/test_web_app_database_factory.h"
 #include "chrome/browser/web_applications/web_app.h"
 #include "chrome/browser/web_applications/web_app_registry_update.h"
 #include "chrome/browser/web_applications/web_app_sync_bridge.h"
+#include "third_party/skia/include/core/SkColor.h"
 
 namespace web_app {
 
@@ -52,6 +54,37 @@ void TestWebAppRegistryController::UnregisterAll() {
     update->DeleteApp(app_id);
 }
 
+void TestWebAppRegistryController::ApplySyncChanges_AddApps(
+    std::vector<GURL> apps_to_add) {
+  std::unique_ptr<syncer::MetadataChangeList> metadata_change_list =
+      sync_bridge().CreateMetadataChangeList();
+  syncer::EntityChangeList entity_changes;
+
+  for (const GURL& app_url : apps_to_add) {
+    const AppId app_id = GenerateAppIdFromURL(app_url);
+
+    auto web_app_server_data = std::make_unique<WebApp>(app_id);
+    web_app_server_data->SetName("WebApp name");
+    web_app_server_data->SetLaunchUrl(app_url);
+    web_app_server_data->SetUserDisplayMode(DisplayMode::kStandalone);
+
+    WebApp::SyncData sync_data;
+    sync_data.name = "WebApp sync data name";
+    sync_data.theme_color = SK_ColorWHITE;
+    web_app_server_data->SetSyncData(std::move(sync_data));
+
+    std::unique_ptr<syncer::EntityData> entity_data =
+        CreateSyncEntityData(*web_app_server_data);
+
+    auto entity_change =
+        syncer::EntityChange::CreateAdd(app_id, std::move(*entity_data));
+    entity_changes.push_back(std::move(entity_change));
+  }
+
+  sync_bridge().ApplySyncChanges(std::move(metadata_change_list),
+                                 std::move(entity_changes));
+}
+
 void TestWebAppRegistryController::ApplySyncChanges_DeleteApps(
     std::vector<AppId> app_ids_to_delete) {
   std::unique_ptr<syncer::MetadataChangeList> metadata_change_list =

chrome/browser/web_applications/test/test_web_app_registry_controller.h

@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_WEB_APPLICATIONS_TEST_TEST_WEB_APP_REGISTRY_CONTROLLER_H_
 
 #include <memory>
+#include <vector>
 
 #include "base/callback.h"
 #include "chrome/browser/web_applications/components/web_app_id.h"
@@ -14,6 +15,7 @@
 #include "components/sync/model/metadata_batch.h"
 #include "components/sync/model/mock_model_type_change_processor.h"
 #include "testing/gmock/include/gmock/gmock.h"
+#include "url/gurl.h"
 
 class Profile;
 
@@ -38,6 +40,7 @@ class TestWebAppRegistryController : public SyncInstallDelegate {
   void UnregisterApp(const AppId& app_id);
   void UnregisterAll();
 
+  void ApplySyncChanges_AddApps(std::vector<GURL> apps_to_add);
   void ApplySyncChanges_DeleteApps(std::vector<AppId> app_ids_to_delete);
 
   using InstallWebAppsAfterSyncDelegate =

chrome/browser/web_applications/test/test_web_app_url_loader.cc

@@ -27,7 +27,11 @@ void TestWebAppUrlLoader::ProcessLoadUrlRequests() {
     pending_requests_.pop();
 
     DCHECK(base::Contains(next_result_map_, url));
-    auto result = next_result_map_[url];
+
+    const UrlResponses& url_responses = next_result_map_[url];
+    DCHECK_EQ(1u, url_responses.results.size());
+
+    Result result = url_responses.results.front();
     next_result_map_.erase(url);
 
     std::move(callback).Run(result);
@@ -35,8 +39,17 @@ void TestWebAppUrlLoader::ProcessLoadUrlRequests() {
 }
 
 void TestWebAppUrlLoader::SetNextLoadUrlResult(const GURL& url, Result result) {
+  AddNextLoadUrlResults(url, {result});
+}
+
+void TestWebAppUrlLoader::AddNextLoadUrlResults(
+    const GURL& url,
+    const std::vector<Result>& results) {
   DCHECK(!base::Contains(next_result_map_, url)) << url;
-  next_result_map_[url] = result;
+  UrlResponses& responses = next_result_map_[url];
+
+  for (Result result : results)
+    responses.results.push(result);
 }
 
 void TestWebAppUrlLoader::LoadUrl(const GURL& url,
@@ -49,8 +62,14 @@ void TestWebAppUrlLoader::LoadUrl(const GURL& url,
   }
 
   DCHECK(base::Contains(next_result_map_, url)) << url;
-  auto result = next_result_map_[url];
-  next_result_map_.erase(url);
+  UrlResponses& responses = next_result_map_[url];
+  DCHECK(!responses.results.empty());
+
+  Result result = responses.results.front();
+  responses.results.pop();
+
+  if (responses.results.empty())
+    next_result_map_.erase(url);
 
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::BindOnce(std::move(callback), result));
@@ -61,4 +80,8 @@ void TestWebAppUrlLoader::SetAboutBlankResultLoaded() {
                        WebAppUrlLoader::Result::kUrlLoaded);
 }
 
+TestWebAppUrlLoader::UrlResponses::UrlResponses() = default;
+
+TestWebAppUrlLoader::UrlResponses::~UrlResponses() = default;
+
 }  // namespace web_app

chrome/browser/web_applications/test/test_web_app_url_loader.h

@@ -7,7 +7,9 @@
 
 #include <map>
 #include <queue>
+#include <vector>
 
+#include "base/containers/queue.h"
 #include "chrome/browser/web_applications/components/web_app_url_loader.h"
 #include "url/gurl.h"
 
@@ -27,8 +29,12 @@ class TestWebAppUrlLoader : public WebAppUrlLoader {
   // order they were issued.
   void ProcessLoadUrlRequests();
 
-  // Sets the result for the next loader that will be created.
+  // Sets the result for the next loader that will be created. Will load with
+  // the result just once.
   void SetNextLoadUrlResult(const GURL& url, Result result);
+  // Sets sequential results for next multiple loads of the given |url|.
+  void AddNextLoadUrlResults(const GURL& url,
+                             const std::vector<Result>& results);
 
   // WebAppUrlLoader
   void LoadUrl(const GURL& url,
@@ -42,7 +48,15 @@ class TestWebAppUrlLoader : public WebAppUrlLoader {
  private:
   bool should_save_requests_ = false;
 
-  std::map<GURL, Result> next_result_map_;
+  struct UrlResponses {
+    UrlResponses();
+    ~UrlResponses();
+
+    // Each LoadUrl() gets next result for a given url.
+    base::queue<Result> results;
+  };
+
+  std::map<GURL, UrlResponses> next_result_map_;
 
   std::queue<std::pair<GURL, ResultCallback>> pending_requests_;
 

chrome/browser/web_applications/web_app_install_finalizer.cc

@@ -193,7 +193,16 @@ void WebAppInstallFinalizer::FinalizeFallbackInstallAfterSync(
     InstallFinalizedCallback callback) {
   const WebApp* app_in_sync_install = GetWebAppRegistrar().GetAppById(app_id);
   DCHECK(app_in_sync_install);
-  DCHECK(app_in_sync_install->is_in_sync_install());
+
+  // This |is_in_sync_install| web app entry might be already overwritten by
+  // FinalizeInstall from a parallel bookmark app install. Do not overwrite
+  // the web app entry, ignore this fallback install to prefer bookmark app
+  // install data.
+  if (!app_in_sync_install->is_in_sync_install()) {
+    std::move(callback).Run(app_id,
+                            InstallResultCode::kSuccessAlreadyInstalled);
+    return;
+  }
 
   // Promote the app in sync install to a full user-visible app using the poor
   // data that we've got from sync. Prepare copy-on-write:

chrome/browser/web_applications/web_app_install_manager_unittest.cc

@@ -399,6 +399,7 @@ class WebAppInstallManagerTest : public WebAppTest {
 
     install_manager().SetDataRetrieverFactoryForTesting(
         base::BindLambdaForTesting([this]() {
+          DCHECK(data_retriever_);
           return std::unique_ptr<WebAppDataRetriever>(
               std::move(data_retriever_));
         }));
@@ -1216,4 +1217,125 @@ TEST_F(WebAppInstallManagerTest, InstallBookmarkAppFromSync_ExpectAppIdFailed) {
   EXPECT_TRUE(ContainsOneIconOfEachSize(icon_bitmaps));
 }
 
+TEST_F(WebAppInstallManagerTest,
+       SyncRace_InstallBookmarkAppFullThenWebAppFull) {
+  InitEmptyRegistrar();
+
+  const GURL url{"https://example.com/path"};
+  const AppId app_id = GenerateAppIdFromURL(url);
+
+  // The web site url will be loaded twice in a sequence.
+  url_loader().AddNextLoadUrlResults(url,
+                                     {WebAppUrlLoader::Result::kUrlLoaded,
+                                      WebAppUrlLoader::Result::kUrlLoaded});
+
+  // Prepare web site data for next enqueued full install (the bookmark app).
+  BuildDefaultDataRetriever(url);
+
+  auto server_bookmark_app_info = std::make_unique<WebApplicationInfo>();
+  server_bookmark_app_info->app_url = url;
+
+  bool bookmark_app_installed = false;
+  // The bookmark app object arrives first from the server, enqueue full
+  // install.
+  install_manager().InstallBookmarkAppFromSync(
+      app_id, std::move(server_bookmark_app_info),
+      base::BindLambdaForTesting(
+          [&](const AppId& installed_app_id, InstallResultCode code) {
+            EXPECT_EQ(app_id, installed_app_id);
+            EXPECT_EQ(InstallResultCode::kSuccessNewInstall, code);
+            bookmark_app_installed = true;
+          }));
+
+  base::RunLoop run_loop;
+  controller().SetInstallWebAppsAfterSyncDelegate(base::BindLambdaForTesting(
+      [&](std::vector<WebApp*> web_apps_installed,
+          SyncInstallDelegate::RepeatingInstallCallback callback) {
+        EXPECT_EQ(1u, web_apps_installed.size());
+        EXPECT_EQ(app_id, web_apps_installed[0]->app_id());
+        EXPECT_EQ(url, web_apps_installed[0]->launch_url());
+
+        // Prepare web site data for next enqueued full install (the web app).
+        BuildDefaultDataRetriever(url);
+
+        install_manager().InstallWebAppsAfterSync(
+            std::move(web_apps_installed),
+            base::BindLambdaForTesting(
+                [&](const AppId& installed_app_id, InstallResultCode code) {
+                  EXPECT_EQ(app_id, installed_app_id);
+                  EXPECT_EQ(InstallResultCode::kSuccessNewInstall, code);
+                  run_loop.Quit();
+                }));
+      }));
+
+  // The web app object arrives second from the server but it creates a registry
+  // entry immediately (with is_in_sync_install() flag set to true).
+  controller().ApplySyncChanges_AddApps({url});
+  run_loop.Run();
+
+  EXPECT_TRUE(bookmark_app_installed);
+}
+
+TEST_F(WebAppInstallManagerTest,
+       SyncRace_InstallBookmarkAppFullThenWebAppFallback) {
+  InitEmptyRegistrar();
+
+  const GURL url{"https://example.com/path"};
+  const AppId app_id = GenerateAppIdFromURL(url);
+
+  // The web site url will be loaded twice in a sequence. The second load fails
+  // (the web app).
+  url_loader().AddNextLoadUrlResults(
+      url, {WebAppUrlLoader::Result::kUrlLoaded,
+            WebAppUrlLoader::Result::kFailedPageTookTooLong});
+
+  // Prepare web site data for next enqueued full install (the bookmark app).
+  BuildDefaultDataRetriever(url);
+
+  auto server_bookmark_app_info = std::make_unique<WebApplicationInfo>();
+  server_bookmark_app_info->app_url = url;
+
+  bool bookmark_app_installed = false;
+  // The bookmark app object arrives first from the server, enqueue full
+  // install.
+  install_manager().InstallBookmarkAppFromSync(
+      app_id, std::move(server_bookmark_app_info),
+      base::BindLambdaForTesting(
+          [&](const AppId& installed_app_id, InstallResultCode code) {
+            EXPECT_EQ(app_id, installed_app_id);
+            EXPECT_EQ(InstallResultCode::kSuccessNewInstall, code);
+            bookmark_app_installed = true;
+          }));
+
+  base::RunLoop run_loop;
+  controller().SetInstallWebAppsAfterSyncDelegate(base::BindLambdaForTesting(
+      [&](std::vector<WebApp*> web_apps_installed,
+          SyncInstallDelegate::RepeatingInstallCallback callback) {
+        EXPECT_EQ(1u, web_apps_installed.size());
+        EXPECT_EQ(app_id, web_apps_installed[0]->app_id());
+        EXPECT_EQ(url, web_apps_installed[0]->launch_url());
+
+        // Prepare web site data for next enqueued full install (the web app).
+        BuildDefaultDataRetriever(url);
+
+        install_manager().InstallWebAppsAfterSync(
+            std::move(web_apps_installed),
+            base::BindLambdaForTesting(
+                [&](const AppId& installed_app_id, InstallResultCode code) {
+                  EXPECT_EQ(app_id, installed_app_id);
+                  // Full web app install fails, the web app fallback install
+                  // returns early.
+                  EXPECT_EQ(InstallResultCode::kSuccessAlreadyInstalled, code);
+                  run_loop.Quit();
+                }));
+      }));
+
+  // The web app object arrives second from the server but it creates a registry
+  // entry immediately (with is_in_sync_install() flag set to true).
+  controller().ApplySyncChanges_AddApps({url});
+  run_loop.Run();
+
+  EXPECT_TRUE(bookmark_app_installed);
+}
+
 }  // namespace web_app