Don't block InstallableManager::GetData calls when waiting for a service worker.

chrome/browser/installable/installable_manager_browsertest.cc

 // Copyright 2016 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 "chrome/browser/installable/installable_manager.h"
 
 #include "base/command_line.h"
 #include "base/memory/ptr_util.h"
 #include "base/run_loop.h"
 #include "base/threading/thread_task_runner_handle.h"
@@ skipping 690 matching lines @@
                        CheckLazyServiceWorkerPassesWhenWaiting) {
   base::RunLoop tester_run_loop, sw_run_loop;
   std::unique_ptr<CallbackTester> tester(
       new CallbackTester(tester_run_loop.QuitClosure()));
 
   content::WebContents* web_contents =
       browser()->tab_strip_model()->GetActiveWebContents();
   auto manager = base::MakeUnique<LazyWorkerInstallableManager>(
       web_contents, sw_run_loop.QuitClosure());
 
-  // Load a URL with no service worker.
-  GURL test_url = embedded_test_server()->GetURL(
-      "/banners/manifest_no_service_worker.html");
-  ui_test_utils::NavigateToURL(browser(), test_url);
+  {
+    // Load a URL with no service worker.
+    GURL test_url = embedded_test_server()->GetURL(
+        "/banners/manifest_no_service_worker.html");
+    ui_test_utils::NavigateToURL(browser(), test_url);
 
-  // Kick off fetching the data. This should block on waiting for a worker.
-  manager->GetData(GetWebAppParams(),
-                   base::Bind(&CallbackTester::OnDidFinishInstallableCheck,
-                              base::Unretained(tester.get())));
-  sw_run_loop.Run();
+    // Kick off fetching the data. This should block on waiting for a worker.
+    manager->GetData(GetWebAppParams(),
+                     base::Bind(&CallbackTester::OnDidFinishInstallableCheck,
+                                base::Unretained(tester.get())));
+    sw_run_loop.Run();

[benwells, 2017/06/27 07:20:59]

Unrelated to this change - do you need so many run loops? Can you just have the
one and reset it when required?

[dominickn, 2017/06/27 07:32:49]

base::RunLoop has no reset method, and it can only be run once (and its
QuitClosure can only be dispatched once).

[benwells, 2017/06/27 08:31:01]

Right, the pattern I've seen is to have a unique_ptr<> and call reset on that
with a new one after you've called Run and need to do it again.

+  }
 
   // We should now be waiting for the service worker.
+  EXPECT_TRUE(tester->manifest().IsEmpty());
   EXPECT_FALSE(manager->manifest().IsEmpty());
   EXPECT_FALSE(manager->manifest_url().is_empty());
   EXPECT_FALSE(manager->is_installable());
   EXPECT_EQ(1u, manager->icons_.size());
   EXPECT_FALSE((manager->icon_url(kPrimaryIconParams).is_empty()));
   EXPECT_NE(nullptr, (manager->icon(kPrimaryIconParams)));
   EXPECT_EQ(NO_ERROR_DETECTED, manager->manifest_error());
   EXPECT_EQ(NO_ERROR_DETECTED, manager->valid_manifest_error());
   EXPECT_EQ(NO_ERROR_DETECTED, manager->worker_error());
   EXPECT_EQ(NO_ERROR_DETECTED, (manager->icon_error(kPrimaryIconParams)));
-  EXPECT_TRUE(manager->worker_waiting());
-  EXPECT_FALSE(manager->tasks_.empty());
+  EXPECT_TRUE(manager->tasks_.empty());
+  EXPECT_FALSE(manager->paused_tasks_.empty());
+
+  {
+    // Fetching just the manifest and icons should not hang while the other call
+    // is waiting for a worker.
+    base::RunLoop run_loop;
+    std::unique_ptr<CallbackTester> nested_tester(
+        new CallbackTester(run_loop.QuitClosure()));
+    manager->GetData(GetPrimaryIconParams(),
+                     base::Bind(&CallbackTester::OnDidFinishInstallableCheck,
+                                base::Unretained(nested_tester.get())));
+    run_loop.Run();
+
+    EXPECT_FALSE(nested_tester->manifest().IsEmpty());
+    EXPECT_FALSE(nested_tester->manifest_url().is_empty());
+    EXPECT_FALSE(nested_tester->primary_icon_url().is_empty());
+    EXPECT_NE(nullptr, nested_tester->primary_icon());
+    EXPECT_FALSE(nested_tester->is_installable());
+    EXPECT_TRUE(nested_tester->badge_icon_url().is_empty());
+    EXPECT_EQ(nullptr, nested_tester->badge_icon());
+    EXPECT_EQ(NO_ERROR_DETECTED, nested_tester->error_code());
+  }
 
   // Load the service worker.
   EXPECT_TRUE(content::ExecuteScript(
       web_contents, "navigator.serviceWorker.register('service_worker.js');"));
   tester_run_loop.Run();
 
   // We should have passed now.
   EXPECT_FALSE(tester->manifest().IsEmpty());
   EXPECT_FALSE(tester->manifest_url().is_empty());
   EXPECT_FALSE(tester->primary_icon_url().is_empty());
   EXPECT_NE(nullptr, tester->primary_icon());
   EXPECT_TRUE(tester->is_installable());
   EXPECT_TRUE(tester->badge_icon_url().is_empty());
   EXPECT_EQ(nullptr, tester->badge_icon());
   EXPECT_EQ(NO_ERROR_DETECTED, tester->error_code());
   EXPECT_EQ(manager->page_status_,
             InstallabilityCheckStatus::COMPLETE_PROGRESSIVE_WEB_APP);
 
   // Verify internal state.
   EXPECT_FALSE(manager->manifest().IsEmpty());
   EXPECT_FALSE(manager->manifest_url().is_empty());
   EXPECT_TRUE(manager->is_installable());
   EXPECT_EQ(1u, manager->icons_.size());
   EXPECT_FALSE((manager->icon_url(kPrimaryIconParams).is_empty()));
   EXPECT_NE(nullptr, (manager->icon(kPrimaryIconParams)));
   EXPECT_EQ(NO_ERROR_DETECTED, manager->manifest_error());
   EXPECT_EQ(NO_ERROR_DETECTED, manager->valid_manifest_error());
   EXPECT_EQ(NO_ERROR_DETECTED, manager->worker_error());
   EXPECT_EQ(NO_ERROR_DETECTED, (manager->icon_error(kPrimaryIconParams)));
-  EXPECT_FALSE(manager->worker_waiting());
   EXPECT_TRUE(manager->tasks_.empty());
+  EXPECT_TRUE(manager->paused_tasks_.empty());
 }
 
 IN_PROC_BROWSER_TEST_F(InstallableManagerBrowserTest,
                        CheckLazyServiceWorkerNoFetchHandlerFails) {
   base::RunLoop tester_run_loop, sw_run_loop;
   std::unique_ptr<CallbackTester> tester(
       new CallbackTester(tester_run_loop.QuitClosure()));
 
   content::WebContents* web_contents =
       browser()->tab_strip_model()->GetActiveWebContents();
   auto manager = base::MakeUnique<LazyWorkerInstallableManager>(
       web_contents, sw_run_loop.QuitClosure());
 
   // Load a URL with no service worker.
   GURL test_url = embedded_test_server()->GetURL(
       "/banners/manifest_no_service_worker.html");
   ui_test_utils::NavigateToURL(browser(), test_url);
 
   // Kick off fetching the data. This should block on waiting for a worker.
   manager->GetData(GetWebAppParams(),
                    base::Bind(&CallbackTester::OnDidFinishInstallableCheck,
                               base::Unretained(tester.get())));
   sw_run_loop.Run();
 
   // We should now be waiting for the service worker.
-  EXPECT_TRUE(manager->worker_waiting());
-  EXPECT_FALSE(manager->tasks_.empty());
+  EXPECT_TRUE(manager->tasks_.empty());
+  EXPECT_FALSE(manager->paused_tasks_.empty());
 
   // Load the service worker with no fetch handler.
   EXPECT_TRUE(content::ExecuteScript(web_contents,
                                      "navigator.serviceWorker.register('"
                                      "service_worker_no_fetch_handler.js');"));
   tester_run_loop.Run();
 
   // We should fail the check.
   EXPECT_FALSE(tester->manifest().IsEmpty());
   EXPECT_FALSE(tester->manifest_url().is_empty());
   EXPECT_FALSE(tester->primary_icon_url().is_empty());
   EXPECT_NE(nullptr, tester->primary_icon());
   EXPECT_FALSE(tester->is_installable());
   EXPECT_TRUE(tester->badge_icon_url().is_empty());
   EXPECT_EQ(nullptr, tester->badge_icon());
   EXPECT_EQ(NOT_OFFLINE_CAPABLE, tester->error_code());
-  EXPECT_FALSE(manager->worker_waiting());
   EXPECT_EQ(manager->page_status_,
             InstallabilityCheckStatus::COMPLETE_NON_PROGRESSIVE_WEB_APP);
 }
 
 IN_PROC_BROWSER_TEST_F(InstallableManagerBrowserTest,
                        CheckServiceWorkerErrorIsNotCached) {
   content::WebContents* web_contents =
       browser()->tab_strip_model()->GetActiveWebContents();
   base::RunLoop sw_run_loop;
   auto manager = base::MakeUnique<LazyWorkerInstallableManager>(
@@ skipping 217 matching lines @@
                        CheckNestedCallsToGetData) {
   // Verify that we can call GetData while in a callback from GetData.
   base::RunLoop run_loop;
   InstallableParams params = GetWebAppParams();
   std::unique_ptr<NestedCallbackTester> tester(
       new NestedCallbackTester(GetManager(), params, run_loop.QuitClosure()));
 
   tester->Run();
   run_loop.Run();
 }