[ServiceWorker] Refactor ServiceWorkerNetworkProviderForFrame::Create()

Make ServiceWorkerNetworkProviderForFrame::Create() be called only in
case of creation for a valid instance.

Have those callsites intending to create an empty instance just call
ServiceWorkerNetworkProviderForFrame::CreateInvalidInstance() directly.

BUG=931092

Change-Id: Icec8a537b96bc420c8dbc0b79a09d6ff3bd51598
Reviewed-on: https://chromium-review.googlesource.com/c/1477513Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Commit-Queue: Leon Han <leon.han@intel.com>
Cr-Commit-Position: refs/heads/master@{#635438}

content/renderer/render_frame_impl.cc

@@ -2891,7 +2891,7 @@ void RenderFrameImpl::LoadNavigationErrorPage(
   if (replace_current_item)
     navigation_params->frame_load_type = WebFrameLoadType::kReplaceCurrentItem;
   navigation_params->service_worker_network_provider =
-      BuildServiceWorkerNetworkProviderForNavigation(nullptr, nullptr);
+      ServiceWorkerNetworkProviderForFrame::CreateInvalidInstance();
 
   // The load of the error page can result in this frame being removed.
   frame_->CommitNavigation(std::move(navigation_params),
@@ -3835,6 +3835,9 @@ void RenderFrameImpl::MarkInitiatorAsRequiringSeparateURLLoaderFactory(
     network::mojom::URLLoaderFactoryPtr url_loader_factory) {
   DCHECK(base::FeatureList::IsEnabled(network::features::kNetworkService));
 
+  // Set up |loader_factories_| to be updated by the
+  // UpdateSubresourceLoaderFactories() below.
+  GetLoaderFactoryBundle();
   auto factory_bundle = std::make_unique<blink::URLLoaderFactoryBundleInfo>();
   factory_bundle->initiator_specific_factory_infos()[initiator_origin] =
       url_loader_factory.PassInterface();
@@ -4466,8 +4469,7 @@ void RenderFrameImpl::DidCreateDocumentLoader(
     // document.
     document_loader->SetExtraData(BuildDocumentState());
     document_loader->SetServiceWorkerNetworkProvider(
-        BuildServiceWorkerNetworkProviderForNavigation(
-            nullptr /* commit_params */, nullptr /* controller_info */));
+        ServiceWorkerNetworkProviderForFrame::CreateInvalidInstance());
   }
 }
 
@@ -6388,9 +6390,7 @@ void RenderFrameImpl::CommitSyncNavigation(
   // We need the provider to be non-null, otherwise Blink crashes, even
   // though the provider should not be used for any actual networking.
   navigation_params->service_worker_network_provider =
-      BuildServiceWorkerNetworkProviderForNavigation(
-          nullptr /* commit_params */,
-          nullptr /* controller_service_worker_info */);
+      ServiceWorkerNetworkProviderForFrame::CreateInvalidInstance();
   frame_->CommitNavigation(std::move(navigation_params), BuildDocumentState());
 }
 
@@ -6713,7 +6713,8 @@ void RenderFrameImpl::SetupLoaderFactoryBundle(
     // 2) With NetworkService, but only for a placeholder document or an
     // initial empty document cases.
     DCHECK(!base::FeatureList::IsEnabled(network::features::kNetworkService) ||
-           !frame_->GetDocumentLoader());
+           GetLoadingUrl().is_empty() ||
+           GetLoadingUrl().spec() == url::kAboutBlankURL);
     loader_factories_->Update(render_thread->blink_platform_impl()
                                   ->CreateDefaultURLLoaderFactoryBundle()
                                   ->PassInterface());
@@ -7444,12 +7445,17 @@ RenderFrameImpl::BuildServiceWorkerNetworkProviderForNavigation(
     const CommitNavigationParams* commit_params,
     blink::mojom::ControllerServiceWorkerInfoPtr
         controller_service_worker_info) {
+  // An empty provider will always be created since it is expected in a certain
+  // number of places.
+  if (!commit_params->should_create_service_worker) {
+    return ServiceWorkerNetworkProviderForFrame::CreateInvalidInstance();
+  }
   scoped_refptr<network::SharedURLLoaderFactory> fallback_factory =
       network::SharedURLLoaderFactory::Create(
           GetLoaderFactoryBundle()->CloneWithoutAppCacheFactory());
   return ServiceWorkerNetworkProviderForFrame::Create(
-      this, commit_params, std::move(controller_service_worker_info),
-      std::move(fallback_factory));
+      this, commit_params->service_worker_provider_id,
+      std::move(controller_service_worker_info), std::move(fallback_factory));
 }
 
 }  // namespace content

content/renderer/service_worker/service_worker_network_provider_for_frame.cc

@@ -6,7 +6,6 @@
 
 #include <utility>
 
-#include "content/common/navigation_params.h"
 #include "content/common/service_worker/service_worker_utils.h"
 #include "content/public/common/origin_util.h"
 #include "content/public/renderer/render_frame_observer.h"
@@ -74,18 +73,9 @@ class ServiceWorkerNetworkProviderForFrame::NewDocumentObserver
 std::unique_ptr<ServiceWorkerNetworkProviderForFrame>
 ServiceWorkerNetworkProviderForFrame::Create(
     RenderFrameImpl* frame,
-    const CommitNavigationParams* commit_params,
+    int provider_id,
     blink::mojom::ControllerServiceWorkerInfoPtr controller_info,
     scoped_refptr<network::SharedURLLoaderFactory> fallback_loader_factory) {
-  // Determine if a provider should be created and properly initialized for the
-  // navigation. A default provider will always be created since it is expected
-  // in a certain number of places, however it will have an invalid id.
-  if (!commit_params || !commit_params->should_create_service_worker) {
-    return CreateInvalidInstance();
-  }
-
-  // Otherwise, create the provider.
-
   // Ideally Document::IsSecureContext would be called here, but the document is
   // not created yet, and due to redirects the URL may change. So pass
   // is_parent_frame_secure to the browser process, so it can determine the
@@ -94,7 +84,6 @@ ServiceWorkerNetworkProviderForFrame::Create(
   const bool is_parent_frame_secure =
       IsFrameSecure(frame->GetWebFrame()->Parent());
 
-  int provider_id = commit_params->service_worker_provider_id;
   // If the browser process did not assign a provider id already, assign one
   // now (see class comments for content::ServiceWorkerProviderHost).
   DCHECK(ServiceWorkerUtils::IsBrowserAssignedProviderId(provider_id) ||

content/renderer/service_worker/service_worker_network_provider_for_frame.h

@@ -15,7 +15,6 @@
 #include "third_party/blink/public/platform/modules/service_worker/web_service_worker_network_provider.h"
 
 namespace content {
-struct CommitNavigationParams;
 class RenderFrameImpl;
 
 // The WebServiceWorkerNetworkProvider implementation used for frames.
@@ -24,16 +23,6 @@ class CONTENT_EXPORT ServiceWorkerNetworkProviderForFrame final
  public:
   // Creates a network provider for |frame|.
   //
-  // |commit_params| are navigation parameters that were transmitted to the
-  // renderer by the browser on a navigation commit. It is null if we have not
-  // yet heard from the browser (currently only during the time it takes from
-  // having the renderer initiate a navigation until the browser commits it).
-  // Note: in particular, provisional load failure do not provide
-  // |commit_params|.
-  // TODO(ahemery): Update this comment when do not create placeholder document
-  // loaders for renderer-initiated navigations. In this case, this should never
-  // be null.
-  //
   // For S13nServiceWorker:
   // |controller_info| contains the endpoint and object info that is needed to
   // set up the controller service worker for the client.
@@ -43,7 +32,7 @@ class CONTENT_EXPORT ServiceWorkerNetworkProviderForFrame final
   // the loading context, e.g. a frame, provides it.
   static std::unique_ptr<ServiceWorkerNetworkProviderForFrame> Create(
       RenderFrameImpl* frame,
-      const CommitNavigationParams* commit_params,
+      int provider_id,
       blink::mojom::ControllerServiceWorkerInfoPtr controller_info,
       scoped_refptr<network::SharedURLLoaderFactory> fallback_loader_factory);