ServiceWorkerImportedScriptUpdateCheck uses throttles on the browser

Previously it doesn't care about the throttles, but it means the update requests will be missed from the embedder. This CL is to use ThrottlingURLLoader in ServiceWorkerSingleScriptUpdateChecker, and also this makes ServiceWorkerNewScriptLoader be able to inherit the throttling loader if it's resuming a cache writer. Now throttles work only on the UI thread, so this CL introduces a thin wrapper to invoke method calls on the UI thread. After this CL lands, I'm going to add code to send the paused state to the renderer as a resource override so that the renderer doesn't create ThrottlingURLLoader for the request which is already using ThrottlingURLLoader in the browser process. Bug: 988957 Change-Id: I32e19d3cd7071dddc056ffdab2bfad36679d5b0f Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1728889 Auto-Submit: Makoto Shimazu <shimazu@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Josh Karlin <jkarlin@chromium.org> Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org> Commit-Queue: Makoto Shimazu <shimazu@chromium.org> Cr-Commit-Position: refs/heads/master@{#686419}
chromium · Aug 13, 2019 · f47a94c · f47a94c
1 parent 439397d
commit f47a94c
Show file tree

Hide file tree

Showing 11 changed files with 316 additions and 74 deletions.
diff --git a/chrome/browser/net/variations_http_headers_browsertest.cc b/chrome/browser/net/variations_http_headers_browsertest.cc
@@ -40,6 +40,7 @@
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "services/network/public/cpp/simple_url_loader.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/blink/public/common/service_worker/service_worker_utils.h"
 #include "url/gurl.h"
 
 namespace {
@@ -135,6 +136,8 @@ class VariationsHttpHeadersBrowserTest : public InProcessBrowserTest {
     return it->second.find(header) != it->second.end();
   }
 
+  void ClearReceivedHeaders() { received_headers_.clear(); }
+
   bool FetchResource(const GURL& url) {
     if (!url.is_valid())
       return false;
@@ -164,11 +167,9 @@ class VariationsHttpHeadersBrowserTest : public InProcessBrowserTest {
     GURL url =
         GetGoogleUrlWithPath("/service_worker/create_service_worker.html");
     EXPECT_TRUE(ui_test_utils::NavigateToURL(browser(), url));
-
-    const std::string scope = "/";
-    EXPECT_EQ("DONE",
-              EvalJs(GetWebContents(), base::StrCat({"register('", worker_path,
-                                                     "', '", scope, "');"})));
+    EXPECT_EQ("DONE", EvalJs(GetWebContents(),
+                             base::StringPrintf("register('%s', '/');",
+                                                worker_path.c_str())));
   }
 
   // Registers the given service worker for google.com then tests navigation and
@@ -426,7 +427,7 @@ IN_PROC_BROWSER_TEST_F(VariationsHttpHeadersBrowserTest,
 }
 
 // Verify in an integration test that the variations header (X-Client-Data) is
-// attached to requests for service worker scripts.
+// attached to requests for service worker scripts when installing and updating.
 IN_PROC_BROWSER_TEST_F(VariationsHttpHeadersBrowserTest, ServiceWorkerScript) {
   // Register a service worker that imports scripts.
   GURL absolute_import = GetExampleUrlWithPath("/service_worker/empty.js");
@@ -445,6 +446,24 @@ IN_PROC_BROWSER_TEST_F(VariationsHttpHeadersBrowserTest, ServiceWorkerScript) {
 
   // But not on requests not to Google.
   EXPECT_FALSE(HasReceivedHeader(absolute_import, "X-Client-Data"));
+
+  // Prepare for the update case.
+  ClearReceivedHeaders();
+
+  // Tries to update the service worker.
+  EXPECT_EQ("DONE", EvalJs(GetWebContents(), "update();"));
+
+  // Test that the header is present on the main script request.
+  EXPECT_TRUE(
+      HasReceivedHeader(GetGoogleUrlWithPath(worker_path), "X-Client-Data"));
+
+  if (blink::ServiceWorkerUtils::IsImportedScriptUpdateCheckEnabled()) {
+    // And on import script requests to Google.
+    EXPECT_TRUE(HasReceivedHeader(
+        GetGoogleUrlWithPath("/service_worker/empty.js"), "X-Client-Data"));
+    // But not on requests not to Google.
+    EXPECT_FALSE(HasReceivedHeader(absolute_import, "X-Client-Data"));
+  }
 }
 
 // Verify in an integration test that the variations header (X-Client-Data) is

diff --git a/content/browser/service_worker/service_worker_single_script_update_checker.cc b/content/browser/service_worker/service_worker_single_script_update_checker.cc
@@ -14,12 +14,17 @@
 #include "content/browser/service_worker/service_worker_consts.h"
 #include "content/browser/service_worker/service_worker_loader_helpers.h"
 #include "content/common/service_worker/service_worker_utils.h"
+#include "content/common/throttling_url_loader.h"
+#include "content/public/browser/content_browser_client.h"
+#include "content/public/browser/render_frame_host.h"
+#include "content/public/common/content_client.h"
 #include "content/public/common/resource_type.h"
 #include "mojo/public/cpp/system/simple_watcher.h"
 #include "net/base/ip_endpoint.h"
 #include "net/base/load_flags.h"
 #include "services/network/public/cpp/net_adapters.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
+#include "third_party/blink/public/common/loader/url_loader_throttle.h"
 #include "third_party/blink/public/mojom/fetch/fetch_api_request.mojom.h"
 
 // TODO(momohatt): Add UMA to capture the result of the update checking.
@@ -94,6 +99,8 @@ ServiceWorkerSingleScriptUpdateChecker::ServiceWorkerSingleScriptUpdateChecker(
     blink::mojom::ServiceWorkerUpdateViaCache update_via_cache,
     base::TimeDelta time_since_last_check,
     const net::HttpRequestHeaders& default_headers,
+    ServiceWorkerUpdatedScriptLoader::BrowserContextGetter
+        browser_context_getter,
     scoped_refptr<network::SharedURLLoaderFactory> loader_factory,
     std::unique_ptr<ServiceWorkerResponseReader> compare_reader,
     std::unique_ptr<ServiceWorkerResponseReader> copy_reader,
@@ -193,20 +200,20 @@ ServiceWorkerSingleScriptUpdateChecker::ServiceWorkerSingleScriptUpdateChecker(
 
   cache_writer_ = ServiceWorkerCacheWriter::CreateForComparison(
       std::move(compare_reader), std::move(copy_reader), std::move(writer),
-      true /* pause_when_not_identical */);
+      /*pause_when_not_identical=*/true);
 
-  network::mojom::URLLoaderClientPtr network_client;
+  network::mojom::URLLoaderClientPtrInfo network_client;
   network_client_binding_.Bind(mojo::MakeRequest(&network_client));
 
   // Use NavigationURLLoaderImpl to get a unique request id across
   // browser-initiated navigations and worker script fetch.
   const int request_id =
       NavigationURLLoaderImpl::MakeGlobalRequestID().request_id;
-  loader_factory->CreateLoaderAndStart(
-      mojo::MakeRequest(&network_loader_), MSG_ROUTING_NONE, request_id,
-      network::mojom::kURLLoadOptionNone, resource_request,
-      std::move(network_client),
-      net::MutableNetworkTrafficAnnotationTag(kUpdateCheckTrafficAnnotation));
+  network_loader_ = ServiceWorkerUpdatedScriptLoader::
+      ThrottlingURLLoaderIOWrapper::CreateLoaderAndStart(
+          loader_factory->Clone(), browser_context_getter, MSG_ROUTING_NONE,
+          request_id, network::mojom::kURLLoadOptionNone, resource_request,
+          std::move(network_client), kUpdateCheckTrafficAnnotation);
   DCHECK_EQ(network_loader_state_,
             ServiceWorkerUpdatedScriptLoader::LoaderState::kNotStarted);
   network_loader_state_ =
@@ -577,7 +584,9 @@ void ServiceWorkerSingleScriptUpdateChecker::Finish(
 
 ServiceWorkerSingleScriptUpdateChecker::PausedState::PausedState(
     std::unique_ptr<ServiceWorkerCacheWriter> cache_writer,
-    network::mojom::URLLoaderPtr network_loader,
+    std::unique_ptr<
+        ServiceWorkerUpdatedScriptLoader::ThrottlingURLLoaderIOWrapper>
+        network_loader,
     network::mojom::URLLoaderClientRequest network_client_request,
     mojo::ScopedDataPipeConsumerHandle network_consumer,
     ServiceWorkerUpdatedScriptLoader::LoaderState network_loader_state,

diff --git a/content/browser/service_worker/service_worker_single_script_update_checker.h b/content/browser/service_worker/service_worker_single_script_update_checker.h
@@ -58,7 +58,9 @@ class CONTENT_EXPORT ServiceWorkerSingleScriptUpdateChecker
   struct CONTENT_EXPORT PausedState {
     PausedState(
         std::unique_ptr<ServiceWorkerCacheWriter> cache_writer,
-        network::mojom::URLLoaderPtr network_loader,
+        std::unique_ptr<
+            ServiceWorkerUpdatedScriptLoader::ThrottlingURLLoaderIOWrapper>
+            network_loader,
         network::mojom::URLLoaderClientRequest network_client_request,
         mojo::ScopedDataPipeConsumerHandle network_consumer,
         ServiceWorkerUpdatedScriptLoader::LoaderState network_loader_state,
@@ -68,7 +70,9 @@ class CONTENT_EXPORT ServiceWorkerSingleScriptUpdateChecker
     ~PausedState();
 
     std::unique_ptr<ServiceWorkerCacheWriter> cache_writer;
-    network::mojom::URLLoaderPtr network_loader;
+    std::unique_ptr<
+        ServiceWorkerUpdatedScriptLoader::ThrottlingURLLoaderIOWrapper>
+        network_loader;
     network::mojom::URLLoaderClientRequest network_client_request;
     mojo::ScopedDataPipeConsumerHandle network_consumer;
     ServiceWorkerUpdatedScriptLoader::LoaderState network_loader_state;
@@ -100,6 +104,8 @@ class CONTENT_EXPORT ServiceWorkerSingleScriptUpdateChecker
       blink::mojom::ServiceWorkerUpdateViaCache update_via_cache,
       base::TimeDelta time_since_last_check,
       const net::HttpRequestHeaders& default_headers,
+      ServiceWorkerUpdatedScriptLoader::BrowserContextGetter
+          browser_context_getter,
       scoped_refptr<network::SharedURLLoaderFactory> loader_factory,
       std::unique_ptr<ServiceWorkerResponseReader> compare_reader,
       std::unique_ptr<ServiceWorkerResponseReader> copy_reader,
@@ -159,7 +165,9 @@ class CONTENT_EXPORT ServiceWorkerSingleScriptUpdateChecker
   const base::TimeDelta time_since_last_check_;
   bool network_accessed_ = false;
 
-  network::mojom::URLLoaderPtr network_loader_;
+  std::unique_ptr<
+      ServiceWorkerUpdatedScriptLoader::ThrottlingURLLoaderIOWrapper>
+      network_loader_;
   mojo::Binding<network::mojom::URLLoaderClient> network_client_binding_;
   mojo::ScopedDataPipeConsumerHandle network_consumer_;
   mojo::SimpleWatcher network_watcher_;

diff --git a/content/browser/service_worker/service_worker_single_script_update_checker_unittest.cc b/content/browser/service_worker/service_worker_single_script_update_checker_unittest.cc
@@ -13,6 +13,7 @@
 #include "content/browser/service_worker/service_worker_context_core.h"
 #include "content/browser/service_worker/service_worker_storage.h"
 #include "content/browser/service_worker/service_worker_test_utils.h"
+#include "content/public/test/test_browser_context.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "net/base/load_flags.h"
 #include "net/http/http_util.h"
@@ -60,7 +61,11 @@ class ServiceWorkerSingleScriptUpdateCheckerTest : public testing::Test {
   };
 
   ServiceWorkerSingleScriptUpdateCheckerTest()
-      : thread_bundle_(TestBrowserThreadBundle::IO_MAINLOOP) {}
+      : thread_bundle_(TestBrowserThreadBundle::IO_MAINLOOP),
+        browser_context_(std::make_unique<TestBrowserContext>()) {
+    BrowserContext::EnsureResourceContextInitialized(browser_context_.get());
+    base::RunLoop().RunUntilIdle();
+  }
   ~ServiceWorkerSingleScriptUpdateCheckerTest() override = default;
 
   ServiceWorkerStorage* storage() { return helper_->context()->storage(); }
@@ -115,6 +120,8 @@ class ServiceWorkerSingleScriptUpdateCheckerTest : public testing::Test {
         GURL(url), url == main_script_url, GURL(main_script_url), scope,
         force_bypass_cache, update_via_cache, time_since_last_check,
         net::HttpRequestHeaders(),
+        base::BindRepeating([](BrowserContext* context) { return context; },
+                            browser_context_.get()),
         helper_->context()->GetLoaderFactoryBundleForUpdateCheck(),
         std::move(compare_reader), std::move(copy_reader), std::move(writer),
         base::BindOnce(
@@ -154,6 +161,7 @@ class ServiceWorkerSingleScriptUpdateCheckerTest : public testing::Test {
   TestBrowserThreadBundle thread_bundle_;
   std::unique_ptr<EmbeddedWorkerTestHelper> helper_;
   base::test::ScopedFeatureList feature_list_;
+  std::unique_ptr<TestBrowserContext> browser_context_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(ServiceWorkerSingleScriptUpdateCheckerTest);

diff --git a/content/browser/service_worker/service_worker_test_utils.cc b/content/browser/service_worker/service_worker_test_utils.cc
@@ -22,6 +22,7 @@
 #include "content/common/frame.mojom.h"
 #include "content/common/frame_messages.h"
 #include "content/common/frame_messages.mojom.h"
+#include "content/common/throttling_url_loader.h"
 #include "content/public/common/child_process_host.h"
 #include "content/public/common/transferrable_url_loader.mojom.h"
 #include "mojo/public/cpp/bindings/strong_binding.h"
@@ -624,13 +625,11 @@ ServiceWorkerUpdateCheckTestUtils::CreateUpdateCheckerPausedState(
     ServiceWorkerUpdatedScriptLoader::LoaderState network_loader_state,
     ServiceWorkerUpdatedScriptLoader::WriterState body_writer_state,
     mojo::ScopedDataPipeConsumerHandle network_consumer) {
-  network::mojom::URLLoaderPtr network_loader;
   network::mojom::URLLoaderClientPtr network_loader_client;
-  mojo::MakeRequest(&network_loader);
   network::mojom::URLLoaderClientRequest network_loader_client_request =
       mojo::MakeRequest(&network_loader_client);
   return std::make_unique<ServiceWorkerSingleScriptUpdateChecker::PausedState>(
-      std::move(cache_writer), std::move(network_loader),
+      std::move(cache_writer), /*network_loader=*/nullptr,
       std::move(network_loader_client_request), std::move(network_consumer),
       network_loader_state, body_writer_state);
 }

diff --git a/content/browser/service_worker/service_worker_update_checker.cc b/content/browser/service_worker/service_worker_update_checker.cc
@@ -26,13 +26,17 @@ namespace content {
 
 namespace {
 
-base::Optional<net::HttpRequestHeaders> GetDefaultHeadersOnUI(
-    base::WeakPtr<ServiceWorkerProcessManager> process_manager) {
+void SetUpOnUI(
+    base::WeakPtr<ServiceWorkerProcessManager> process_manager,
+    base::OnceCallback<void(
+        net::HttpRequestHeaders,
+        ServiceWorkerUpdatedScriptLoader::BrowserContextGetter)> callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   if (!process_manager) {
     // If no process manager is found, maybe it's being shut down.
-    // ServiceWorkerUpdateChecker is destroyed after posting this task.
-    return base::nullopt;
+    // ServiceWorkerUpdateChecker is going to be destroyed after this task, so
+    // just do nothing here.
+    return;
   }
 
   net::HttpRequestHeaders headers;
@@ -49,7 +53,20 @@ base::Optional<net::HttpRequestHeaders> GetDefaultHeadersOnUI(
       &headers, browser_context, /*should_update_existing_headers=*/false,
       renderer_preferences);
 
-  return headers;
+  ServiceWorkerUpdatedScriptLoader::BrowserContextGetter
+      browser_context_getter = base::BindRepeating(
+          [](base::WeakPtr<ServiceWorkerProcessManager> process_manager)
+              -> BrowserContext* {
+            DCHECK_CURRENTLY_ON(BrowserThread::UI);
+            if (process_manager)
+              return process_manager->browser_context();
+            return nullptr;
+          },
+          process_manager);
+
+  base::PostTask(FROM_HERE, {BrowserThread::IO},
+                 base::BindOnce(std::move(callback), std::move(headers),
+                                browser_context_getter));
 }
 
 }  // namespace
@@ -80,25 +97,19 @@ void ServiceWorkerUpdateChecker::Start(UpdateStatusCallback callback) {
   DCHECK(!scripts_to_compare_.empty());
   callback_ = std::move(callback);
 
-  // TODO(shimazu): Add UMA to capture the number of update.
-
-  base::PostTaskAndReplyWithResult(
+  base::PostTask(
       FROM_HERE, {BrowserThread::UI},
-      base::BindOnce(&GetDefaultHeadersOnUI,
-                     context_->process_manager()->AsWeakPtr()),
-      base::BindOnce(&ServiceWorkerUpdateChecker::OnGetDefaultHeaders,
-                     weak_factory_.GetWeakPtr()));
+      base::BindOnce(&SetUpOnUI, context_->process_manager()->AsWeakPtr(),
+                     base::BindOnce(&ServiceWorkerUpdateChecker::DidSetUpOnUI,
+                                    weak_factory_.GetWeakPtr())));
 }
 
-void ServiceWorkerUpdateChecker::OnGetDefaultHeaders(
-    base::Optional<net::HttpRequestHeaders> header) {
-  // |header| is always valid because it could be base::nullopt when the process
-  // manager is destroyed, but it means that the ServiceWorkerContextCore is
-  // also destroyed. In that case, ServiceWorkerUpdateChecker is destroyed
-  // because it's owned by ServiceWorkerContextCore through
-  // ServiceWorkerJobCoordinator and ServiceWorkerRegisterJob.
-  DCHECK(header);
-  default_headers_ = std::move(header.value());
+void ServiceWorkerUpdateChecker::DidSetUpOnUI(
+    net::HttpRequestHeaders header,
+    ServiceWorkerUpdatedScriptLoader::BrowserContextGetter
+        browser_context_getter) {
+  default_headers_ = std::move(header);
+  browser_context_getter_ = std::move(browser_context_getter);
   CheckOneScript(main_script_url_, main_script_resource_id_);
 }
 
@@ -198,8 +209,8 @@ void ServiceWorkerUpdateChecker::CheckOneScript(const GURL& url,
   running_checker_ = std::make_unique<ServiceWorkerSingleScriptUpdateChecker>(
       url, is_main_script, main_script_url_, version_to_update_->scope(),
       force_bypass_cache_, update_via_cache_, time_since_last_check_,
-      default_headers_, loader_factory_, std::move(compare_reader),
-      std::move(copy_reader), std::move(writer),
+      default_headers_, browser_context_getter_, loader_factory_,
+      std::move(compare_reader), std::move(copy_reader), std::move(writer),
       base::BindOnce(&ServiceWorkerUpdateChecker::OnOneUpdateCheckFinished,
                      weak_factory_.GetWeakPtr(), resource_id));
 }

diff --git a/content/browser/service_worker/service_worker_update_checker.h b/content/browser/service_worker/service_worker_update_checker.h
@@ -8,6 +8,7 @@
 #include "base/callback.h"
 #include "content/browser/service_worker/service_worker_database.h"
 #include "content/browser/service_worker/service_worker_single_script_update_checker.h"
+#include "content/browser/service_worker/service_worker_updated_script_loader.h"
 
 namespace network {
 class SharedURLLoaderFactory;
@@ -106,7 +107,9 @@ class CONTENT_EXPORT ServiceWorkerUpdateChecker {
 
  private:
   void CheckOneScript(const GURL& url, const int64_t resource_id);
-  void OnGetDefaultHeaders(base::Optional<net::HttpRequestHeaders> header);
+  void DidSetUpOnUI(net::HttpRequestHeaders header,
+                    ServiceWorkerUpdatedScriptLoader::BrowserContextGetter
+                        browser_context_getter);
 
   std::vector<ServiceWorkerDatabase::ResourceRecord> scripts_to_compare_;
   size_t next_script_index_to_compare_ = 0;
@@ -130,6 +133,9 @@ class CONTENT_EXPORT ServiceWorkerUpdateChecker {
   // Headers that need to be added to network requests for update checking.
   net::HttpRequestHeaders default_headers_;
 
+  ServiceWorkerUpdatedScriptLoader::BrowserContextGetter
+      browser_context_getter_;
+
   // True if any at least one of the scripts is fetched by network.
   bool network_accessed_ = false;