web-apps: flush WebContents state between PendingAppManagerImpl tasks

We should flush WebContents state between PendingAppManagerImpl tasks.
This fix is inspired by crbug.com/1086778, crrev.com/c/2237542 and
crrev.com/c/2246044.

Before each Install task and registration task, navigate WebContents
to about:blank to reset it. This hopefully fix InstallURLRedirected
errors spike on System Web Apps since BMO launch.

Bug: 1093707
Change-Id: Ia72f39ecc099c486014342c01ece5cbb4f0362f3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2251418
Commit-Queue: Jiewei Qian  <qjw@chromium.org>
Reviewed-by: Alan Cutter <alancutter@chromium.org>
Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Reviewed-by: Alexey Baskakov <loyso@chromium.org>
Cr-Commit-Position: refs/heads/master@{#781201}

chrome/browser/web_applications/components/web_app_url_loader.cc

@@ -176,4 +176,10 @@ void WebAppUrlLoader::LoadUrl(const GURL& url,
           std::move(callback), std::move(loader_task)));
 }
 
+void WebAppUrlLoader::PrepareForLoad(content::WebContents* web_contents,
+                                     ResultCallback callback) {
+  LoadUrl(GURL(url::kAboutBlankURL), web_contents, UrlComparison::kExact,
+          std::move(callback));
+}
+
 }  // namespace web_app

chrome/browser/web_applications/components/web_app_url_loader.h

@@ -44,6 +44,24 @@ class WebAppUrlLoader {
   WebAppUrlLoader();
   virtual ~WebAppUrlLoader();
 
+  // Navigates |web_contents| to about:blank to prepare for the next LoadUrl()
+  // call.
+  //
+  // We've observed some races when using LoadUrl() on previously navigated
+  // WebContents. Sometimes events from the last navigation are triggered after
+  // we start the new navigation, causing us to incorrectly run the callback
+  // with a redirect error.
+  //
+  // Clients of LoadUrl() should always call PrepareForLoad() before calling
+  // LoadUrl(). PrepareForLoad() will start a new navigation to about:blank and
+  // ignore all navigation events until we've successfully navigated to
+  // about:blank or timed out.
+  //
+  // Clients should check |callback| result and handle failure scenarios
+  // appropriately.
+  virtual void PrepareForLoad(content::WebContents* web_contents,
+                              ResultCallback callback);
+
   // Navigates |web_contents| to |url|, compares the resolved URL with
   // |url_comparison|, and runs callback with the result code.
   virtual void LoadUrl(const GURL& url,

chrome/browser/web_applications/components/web_app_url_loader_browsertest.cc

@@ -258,4 +258,76 @@ IN_PROC_BROWSER_TEST_F(WebAppUrlLoaderTest, NetworkError) {
       LoadUrlAndWait(UrlComparison::kIgnoreQueryParamsAndRef, "/close-socket"));
 }
 
+IN_PROC_BROWSER_TEST_F(WebAppUrlLoaderTest,
+                       PrepareForLoad_AfterNavigationComplete) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  WebAppUrlLoader loader;
+
+  // Load a URL, and wait for its completion.
+  LoadUrlAndWait(UrlComparison::kExact, "/title1.html");
+
+  // Prepare for next load.
+  base::RunLoop run_loop;
+  loader.PrepareForLoad(web_contents(),
+                        base::BindLambdaForTesting([&](Result result) {
+                          EXPECT_EQ(Result::kUrlLoaded, result);
+                          run_loop.Quit();
+                        }));
+  run_loop.Run();
+
+  // Load the next URL.
+  LoadUrlAndWait(UrlComparison::kExact, "/title2.html");
+}
+
+namespace {
+class WebContentsLoadingObserver : public content::WebContentsObserver {
+ public:
+  explicit WebContentsLoadingObserver(content::WebContents* contents)
+      : WebContentsObserver(contents) {}
+  WebContentsLoadingObserver(const WebContentsLoadingObserver&) = delete;
+  WebContentsLoadingObserver& operator=(const WebContentsLoadingObserver&) =
+      delete;
+  ~WebContentsLoadingObserver() override = default;
+
+  void Wait() { run_loop_.Run(); }
+
+  // content::WebContentsObserver:
+  void DidStartLoading() override { run_loop_.Quit(); }
+
+ private:
+  base::RunLoop run_loop_;
+};
+}  // namespace
+
+IN_PROC_BROWSER_TEST_F(WebAppUrlLoaderTest,
+                       PrepareForLoad_BeforeNavigationComplete) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  WebAppUrlLoader loader;
+
+  // Load a URL that takes a long time to load. Use /hung-after-headers here
+  // because it starts the HTTP response, but never returns a HTML document.
+  // We intentionally don't wait for load completion.
+  {
+    WebContentsLoadingObserver observer(web_contents());
+    loader.LoadUrl(embedded_test_server()->GetURL("/hung-after-headers"),
+                   web_contents(), UrlComparison::kExact, base::DoNothing());
+    observer.Wait();
+  }
+
+  // Prepare for next load.
+  {
+    EXPECT_TRUE(web_contents()->IsLoading());
+    base::RunLoop run_loop;
+    loader.PrepareForLoad(web_contents(),
+                          base::BindLambdaForTesting([&](Result result) {
+                            EXPECT_EQ(Result::kUrlLoaded, result);
+                            run_loop.Quit();
+                          }));
+    run_loop.Run();
+  }
+
+  // Load the next URL.
+  LoadUrlAndWait(UrlComparison::kExact, "/title2.html");
+}
+
 }  // namespace web_app

chrome/browser/web_applications/pending_app_manager_impl.cc

@@ -220,6 +220,15 @@ void PendingAppManagerImpl::StartInstallationTask(
 
   CreateWebContentsIfNecessary();
 
+  url_loader_->PrepareForLoad(
+      web_contents_.get(),
+      base::BindOnce(&PendingAppManagerImpl::OnWebContentsReady,
+                     weak_ptr_factory_.GetWeakPtr()));
+}
+
+void PendingAppManagerImpl::OnWebContentsReady(WebAppUrlLoader::Result) {
+  // TODO(crbug.com/1098139): Handle the scenario where WebAppUrlLoader fails to
+  // load about:blank and flush WebContents states.
   url_loader_->LoadUrl(current_install_->task->install_options().url,
                        web_contents_.get(),
                        WebAppUrlLoader::UrlComparison::kSameOrigin,

chrome/browser/web_applications/pending_app_manager_impl.h

@@ -78,6 +78,8 @@ class PendingAppManagerImpl : public PendingAppManager {
 
   void CreateWebContentsIfNecessary();
 
+  void OnWebContentsReady(WebAppUrlLoader::Result result);
+
   void OnUrlLoaded(WebAppUrlLoader::Result result);
 
   void OnInstalled(PendingAppInstallTask::Result result);

chrome/browser/web_applications/pending_app_registration_task.cc

@@ -12,6 +12,7 @@
 #include "content/public/browser/service_worker_context.h"
 #include "content/public/browser/storage_partition.h"
 #include "content/public/browser/web_contents.h"
+#include "url/url_constants.h"
 
 namespace web_app {
 
@@ -85,6 +86,17 @@ void PendingAppRegistrationTask::OnDidCheckHasServiceWorker(
     return;
   }
 
+  url_loader_->PrepareForLoad(
+      web_contents_,
+      base::BindOnce(&PendingAppRegistrationTask::OnWebContentsReady,
+                     weak_ptr_factory_.GetWeakPtr()));
+}
+
+void PendingAppRegistrationTask::OnWebContentsReady(
+    WebAppUrlLoader::Result result) {
+  // TODO(crbug.com/1098139): Handle the scenario where WebAppUrlLoader fails to
+  // load about:blank and flush WebContents states.
+
   // No action is needed when the URL loads.
   // We wait for OnRegistrationCompleted (or registration timeout).
   url_loader_->LoadUrl(launch_url(), web_contents_,

chrome/browser/web_applications/pending_app_registration_task.h

@@ -10,6 +10,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list_types.h"
 #include "base/timer/timer.h"
+#include "chrome/browser/web_applications/components/web_app_url_loader.h"
 #include "content/public/browser/service_worker_context_observer.h"
 
 class GURL;
@@ -58,6 +59,8 @@ class PendingAppRegistrationTask : public PendingAppRegistrationTaskBase {
  private:
   void OnDidCheckHasServiceWorker(content::ServiceWorkerCapability capability);
 
+  void OnWebContentsReady(WebAppUrlLoader::Result result);
+
   void OnRegistrationTimeout();
 
   WebAppUrlLoader* const url_loader_;

chrome/browser/web_applications/test/test_web_app_url_loader.cc

@@ -75,14 +75,13 @@ void TestWebAppUrlLoader::LoadUrl(const GURL& url,
       FROM_HERE, base::BindOnce(std::move(callback), result));
 }
 
-void TestWebAppUrlLoader::SetAboutBlankResultLoaded() {
-  SetNextLoadUrlResult(GURL("about:blank"),
-                       WebAppUrlLoader::Result::kUrlLoaded);
+void TestWebAppUrlLoader::SetPrepareForLoadResultLoaded() {
+  AddPrepareForLoadResults({WebAppUrlLoader::Result::kUrlLoaded});
 }
 
-void TestWebAppUrlLoader::AddAboutBlankResults(
+void TestWebAppUrlLoader::AddPrepareForLoadResults(
     const std::vector<Result>& results) {
-  AddNextLoadUrlResults(GURL("about:blank"), results);
+  AddNextLoadUrlResults(GURL(url::kAboutBlankURL), results);
 }
 
 TestWebAppUrlLoader::UrlResponses::UrlResponses() = default;

chrome/browser/web_applications/test/test_web_app_url_loader.h

@@ -42,9 +42,9 @@ class TestWebAppUrlLoader : public WebAppUrlLoader {
                UrlComparison url_comparison,
                ResultCallback callback) override;
 
-  // Sets the result for the next about:blank load to be ok.
-  void SetAboutBlankResultLoaded();
-  void AddAboutBlankResults(const std::vector<Result>& results);
+  // Sets the result for PrepareForLoad() to be ok.
+  void SetPrepareForLoadResultLoaded();
+  void AddPrepareForLoadResults(const std::vector<Result>& results);
 
  private:
   bool should_save_requests_ = false;

chrome/browser/web_applications/web_app_install_manager_unittest.cc

@@ -440,8 +440,8 @@ class WebAppInstallManagerTest : public WebAppTest {
 
 TEST_F(WebAppInstallManagerTest,
        InstallWebAppsAfterSync_TwoConcurrentInstallsAreRunInOrder) {
-  url_loader().AddAboutBlankResults({WebAppUrlLoader::Result::kUrlLoaded,
-                                     WebAppUrlLoader::Result::kUrlLoaded});
+  url_loader().AddPrepareForLoadResults({WebAppUrlLoader::Result::kUrlLoaded,
+                                         WebAppUrlLoader::Result::kUrlLoaded});
 
   const GURL url1{"https://example.com/path"};
   const AppId app1_id = GenerateAppIdFromURL(url1);
@@ -625,7 +625,7 @@ TEST_F(WebAppInstallManagerTest,
 
   WebApp* web_app = controller().mutable_registrar().GetAppByIdMutable(app_id);
 
-  url_loader().AddAboutBlankResults({WebAppUrlLoader::Result::kUrlLoaded});
+  url_loader().AddPrepareForLoadResults({WebAppUrlLoader::Result::kUrlLoaded});
   url_loader().SetNextLoadUrlResult(launch_url,
                                     WebAppUrlLoader::Result::kUrlLoaded);
 
@@ -695,7 +695,7 @@ TEST_F(WebAppInstallManagerTest, InstallWebAppsAfterSync_Success) {
   WebApp* app = controller().mutable_registrar().GetAppByIdMutable(
       expected_app->app_id());
 
-  url_loader().AddAboutBlankResults({WebAppUrlLoader::Result::kUrlLoaded});
+  url_loader().AddPrepareForLoadResults({WebAppUrlLoader::Result::kUrlLoaded});
   url_loader().SetNextLoadUrlResult(url, WebAppUrlLoader::Result::kUrlLoaded);
 
   install_manager().SetDataRetrieverFactoryForTesting(
@@ -767,7 +767,7 @@ TEST_F(WebAppInstallManagerTest, InstallWebAppsAfterSync_Fallback) {
   // Simulate if the web app publisher's website is down.
   url_loader().SetNextLoadUrlResult(
       url, WebAppUrlLoader::Result::kFailedPageTookTooLong);
-  url_loader().AddAboutBlankResults({WebAppUrlLoader::Result::kUrlLoaded});
+  url_loader().AddPrepareForLoadResults({WebAppUrlLoader::Result::kUrlLoaded});
 
   install_manager().SetDataRetrieverFactoryForTesting(
       base::BindLambdaForTesting([]() {
@@ -971,8 +971,8 @@ TEST_F(WebAppInstallManagerTest, InstallBookmarkAppFromSync_LoadSuccess) {
   const auto url1 = GURL("https://example.com/");
   const auto url2 = GURL("https://example.org/");
 
-  url_loader().AddAboutBlankResults({WebAppUrlLoader::Result::kUrlLoaded,
-                                     WebAppUrlLoader::Result::kUrlLoaded});
+  url_loader().AddPrepareForLoadResults({WebAppUrlLoader::Result::kUrlLoaded,
+                                         WebAppUrlLoader::Result::kUrlLoaded});
   url_loader().SetNextLoadUrlResult(url1, WebAppUrlLoader::Result::kUrlLoaded);
 
   install_manager().SetDataRetrieverFactoryForTesting(
@@ -1025,8 +1025,8 @@ TEST_F(WebAppInstallManagerTest, InstallBookmarkAppFromSync_LoadFailed) {
   const auto url1 = GURL("https://example.com/");
   const auto url2 = GURL("https://example.org/");
 
-  url_loader().AddAboutBlankResults({WebAppUrlLoader::Result::kUrlLoaded,
-                                     WebAppUrlLoader::Result::kUrlLoaded});
+  url_loader().AddPrepareForLoadResults({WebAppUrlLoader::Result::kUrlLoaded,
+                                         WebAppUrlLoader::Result::kUrlLoaded});
 
   // Induce a load failure:
   url_loader().SetNextLoadUrlResult(
@@ -1061,7 +1061,7 @@ TEST_F(WebAppInstallManagerTest, InstallBookmarkAppFromSync_TwoIcons_Success) {
   const GURL icon1_url{"https://example.com/path/icon1.png"};
   const GURL icon2_url{"https://example.com/path/icon2.png"};
 
-  url_loader().AddAboutBlankResults({WebAppUrlLoader::Result::kUrlLoaded});
+  url_loader().AddPrepareForLoadResults({WebAppUrlLoader::Result::kUrlLoaded});
   url_loader().SetNextLoadUrlResult(url, WebAppUrlLoader::Result::kUrlLoaded);
 
   const AppId app_id = GenerateAppIdFromURL(url);
@@ -1135,7 +1135,7 @@ TEST_F(WebAppInstallManagerTest, InstallBookmarkAppFromSync_TwoIcons_Fallback) {
   const GURL icon1_url{"https://example.com/path/icon1.png"};
   const GURL icon2_url{"https://example.com/path/icon2.png"};
 
-  url_loader().AddAboutBlankResults({WebAppUrlLoader::Result::kUrlLoaded});
+  url_loader().AddPrepareForLoadResults({WebAppUrlLoader::Result::kUrlLoaded});
   // Induce a load failure:
   url_loader().SetNextLoadUrlResult(
       url, WebAppUrlLoader::Result::kRedirectedUrlLoaded);
@@ -1193,7 +1193,7 @@ TEST_F(WebAppInstallManagerTest, InstallBookmarkAppFromSync_NoIcons) {
 
   const GURL url{"https://example.com/path"};
 
-  url_loader().AddAboutBlankResults({WebAppUrlLoader::Result::kUrlLoaded});
+  url_loader().AddPrepareForLoadResults({WebAppUrlLoader::Result::kUrlLoaded});
   // Induce a load failure:
   url_loader().SetNextLoadUrlResult(
       url, WebAppUrlLoader::Result::kRedirectedUrlLoaded);
@@ -1227,7 +1227,7 @@ TEST_F(WebAppInstallManagerTest, InstallBookmarkAppFromSync_ExpectAppIdFailed) {
 
   const GURL old_url{"https://example.com/path"};
 
-  url_loader().AddAboutBlankResults({WebAppUrlLoader::Result::kUrlLoaded});
+  url_loader().AddPrepareForLoadResults({WebAppUrlLoader::Result::kUrlLoaded});
   url_loader().SetNextLoadUrlResult(old_url,
                                     WebAppUrlLoader::Result::kUrlLoaded);
 
@@ -1265,7 +1265,7 @@ TEST_F(WebAppInstallManagerTest, InstallBookmarkAppFromSync_QueueNewInstall) {
 
   const GURL url{"https://example.com/path"};
 
-  url_loader().AddAboutBlankResults({WebAppUrlLoader::Result::kUrlLoaded});
+  url_loader().AddPrepareForLoadResults({WebAppUrlLoader::Result::kUrlLoaded});
   url_loader().SetNextLoadUrlResult(url, WebAppUrlLoader::Result::kUrlLoaded);
 
   UseDefaultDataRetriever(url);
@@ -1355,7 +1355,7 @@ TEST_F(WebAppInstallManagerTest, SyncRace_InstallWebAppFull_ThenBookmarkApp) {
   const AppId app_id = GenerateAppIdFromURL(url);
 
   // The web site url must be loaded only once.
-  url_loader().AddAboutBlankResults({WebAppUrlLoader::Result::kUrlLoaded});
+  url_loader().AddPrepareForLoadResults({WebAppUrlLoader::Result::kUrlLoaded});
   url_loader().AddNextLoadUrlResults(url,
                                      {WebAppUrlLoader::Result::kUrlLoaded});
 
@@ -1414,7 +1414,7 @@ TEST_F(WebAppInstallManagerTest, SyncRace_InstallBookmarkAppFull_ThenWebApp) {
   const AppId app_id = GenerateAppIdFromURL(url);
 
   // The web site url must be loaded only once.
-  url_loader().AddAboutBlankResults({WebAppUrlLoader::Result::kUrlLoaded});
+  url_loader().AddPrepareForLoadResults({WebAppUrlLoader::Result::kUrlLoaded});
   url_loader().AddNextLoadUrlResults(url,
                                      {WebAppUrlLoader::Result::kUrlLoaded});
 
@@ -1476,7 +1476,7 @@ TEST_F(WebAppInstallManagerTest,
   const AppId app_id = GenerateAppIdFromURL(url);
 
   // We will try to load the web site url only once.
-  url_loader().AddAboutBlankResults({WebAppUrlLoader::Result::kUrlLoaded});
+  url_loader().AddPrepareForLoadResults({WebAppUrlLoader::Result::kUrlLoaded});
   // The web site url will fail.
   url_loader().AddNextLoadUrlResults(
       url, {WebAppUrlLoader::Result::kFailedPageTookTooLong});
@@ -1543,7 +1543,7 @@ TEST_F(WebAppInstallManagerTest, TaskQueueWebContentsReadyRace) {
 
   // Enqueue task A and await it to be started.
   base::RunLoop run_loop_a_start;
-  url_loader().SetAboutBlankResultLoaded();
+  url_loader().SetPrepareForLoadResultLoaded();
   install_manager().EnsureWebContentsCreated();
   install_manager().EnqueueTask(std::move(task_a),
                                 run_loop_a_start.QuitClosure());
@@ -1556,7 +1556,7 @@ TEST_F(WebAppInstallManagerTest, TaskQueueWebContentsReadyRace) {
       base::BindLambdaForTesting([&]() { task_b_started = true; }));
 
   // Finish task A.
-  url_loader().SetAboutBlankResultLoaded();
+  url_loader().SetPrepareForLoadResultLoaded();
   install_manager().OnQueuedTaskCompleted(
       task_a_ptr, base::DoNothing(), AppId(),
       InstallResultCode::kSuccessNewInstall);