Correct comments for the |request_initiator| parameter in WillCreateURLLoaderFactory.

This CL corrects comments for the |request_initiator| parameter in
WillCreateURLLoaderFactory. The current comments say that navigation requests
are not associated with an initiator which is incorrect.

Also, add a check for a null |render_frame_host| to
ProxyingURLLoaderFactory::MaybeProxyRequest to prevent a null pointer dereference
and fix associated crashes.

BUG=996041

Change-Id: I4a468ded6006c19f0a321d685fa40aa7f6ad1331
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1761607Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#689202}

chrome/browser/signin/chrome_signin_proxying_url_loader_factory.cc

@@ -432,6 +432,9 @@ bool ProxyingURLLoaderFactory::MaybeProxyRequest(
   if (is_navigation)
     return false;
 
+  if (!render_frame_host)
+    return false;
+
   // This proxy should only be installed for subresource requests from a frame
   // that is rendering the GAIA signon realm.
   if (!gaia::IsGaiaSignonRealm(request_initiator.GetURL()))

content/browser/loader/navigation_url_loader_impl.cc

@@ -1268,10 +1268,6 @@ NavigationURLLoaderImpl::NavigationURLLoaderImpl(
         ->RegisterNonNetworkNavigationURLLoaderFactories(
             frame_tree_node_id, &non_network_url_loader_factories_);
 
-    // Navigation requests are not associated with any particular
-    // |network::ResourceRequest::request_initiator| origin - using an opaque
-    // origin instead.
-    url::Origin navigation_request_initiator = url::Origin();
     // The embedder may want to proxy all network-bound URLLoaderFactory
     // requests that it can. If it elects to do so, we'll pass its proxy
     // endpoints off to the URLLoaderRequestController where wthey will be
@@ -1281,9 +1277,8 @@ NavigationURLLoaderImpl::NavigationURLLoaderImpl(
     bool use_proxy = GetContentClient()->browser()->WillCreateURLLoaderFactory(
         partition->browser_context(), frame_tree_node->current_frame_host(),
         frame_tree_node->current_frame_host()->GetProcess()->GetID(),
-        ContentBrowserClient::URLLoaderFactoryType::kNavigation,
-        navigation_request_initiator, &factory_receiver, &header_client,
-        &bypass_redirect_checks);
+        ContentBrowserClient::URLLoaderFactoryType::kNavigation, url::Origin(),
+        &factory_receiver, &header_client, &bypass_redirect_checks);
     if (devtools_instrumentation::WillCreateURLLoaderFactory(
             frame_tree_node->current_frame_host(), true /* is_navigation */,
             false /* is_download */, &factory_receiver)) {
@@ -1475,20 +1470,15 @@ void NavigationURLLoaderImpl::BindNonNetworkURLLoaderFactoryRequest(
     return;
   }
 
-  // Navigation requests are not associated with any particular
-  // |network::ResourceRequest::request_initiator| origin - using an opaque
-  // origin instead.
-  url::Origin navigation_request_initiator = url::Origin();
-
   FrameTreeNode* frame_tree_node =
       FrameTreeNode::GloballyFindByID(frame_tree_node_id);
   auto* frame = frame_tree_node->current_frame_host();
   GetContentClient()->browser()->WillCreateURLLoaderFactory(
       frame->GetSiteInstance()->GetBrowserContext(), frame,
       frame->GetProcess()->GetID(),
-      ContentBrowserClient::URLLoaderFactoryType::kNavigation,
-      navigation_request_initiator, &factory_receiver,
-      nullptr /* header_client */, nullptr /* bypass_redirect_checks */);
+      ContentBrowserClient::URLLoaderFactoryType::kNavigation, url::Origin(),
+      &factory_receiver, nullptr /* header_client */,
+      nullptr /* bypass_redirect_checks */);
   it->second->Clone(std::move(factory_receiver));
 }
 

content/browser/service_worker/service_worker_fetch_dispatcher.cc

@@ -300,13 +300,6 @@ void CreateNetworkFactoryForNavigationPreloadOnUI(
     return;
   }
 
-  // Follow what NavigationURLLoaderImpl does for the initiator passed to
-  // WillCreateURLLoaderFactory():
-  // Navigation requests are not associated with any particular
-  // |network::ResourceRequest::request_initiator| origin - using an opaque
-  // origin instead.
-  url::Origin initiator = url::Origin();
-
   // We ignore the value of |bypass_redirect_checks_unused| since a redirect is
   // just relayed to the service worker where preloadResponse is resolved as
   // redirect.
@@ -317,7 +310,7 @@ void CreateNetworkFactoryForNavigationPreloadOnUI(
   GetContentClient()->browser()->WillCreateURLLoaderFactory(
       partition->browser_context(), frame_tree_node->current_frame_host(),
       frame_tree_node->current_frame_host()->GetProcess()->GetID(),
-      ContentBrowserClient::URLLoaderFactoryType::kNavigation, initiator,
+      ContentBrowserClient::URLLoaderFactoryType::kNavigation, url::Origin(),
       &receiver, &header_client, &bypass_redirect_checks_unused);
 
   // Make the network factory.

content/public/browser/content_browser_client.h

@@ -1182,14 +1182,21 @@ class CONTENT_EXPORT ContentBrowserClient {
   // URLLoaderFactory will be used.
   //
   // |request_initiator| indicates which origin will be the initiator of
-  // requests that will use the URLLoaderFactory (see also
-  // |network::ResourceRequest::requests|). It's set when this factory is
+  // requests that will use the URLLoaderFactory. It's set when this factory is
   // created a) for a renderer to use to fetch subresources
   // (kDocumentSubResource, kWorkerSubResource, kServiceWorkerSubResource), or
-  // b) for the browser to use to fetch a worker (kWorkerMainResource). It's not
-  // set when creating a factory for navigation requests, because navigation
-  // requests are made on behalf of the browser, rather than on behalf of any
-  // particular origin.
+  // b) for the browser to use to fetch a worker (kWorkerMainResource). An
+  // opaque origin is passed currently for navigation (kNavigation) and
+  // download (kDownload) factories even though requests from these factories
+  // can have a valid |network::ResourceRequest::request_initiator|.
+  // Note: For the kDocumentSubResource case, the |request_initiator| may be
+  // incorrect in some cases:
+  // - Sandboxing flags of the frame are not taken into account, which may mean
+  //   that |request_initiator| might need to be opaque but isn't.
+  // - For about:blank frames, the |request_initiator| might be opaque or might
+  //   be the process lock.
+  // TODO(lukasza): https://crbug.com/888079: Ensure that |request_initiator| is
+  // always accurate.
   //
   // |*factory_request| is always valid upon entry and MUST be valid upon
   // return. The embedder may swap out the value of |*factory_request| for its