Use correct URLLoaderFactory in an unload handler.

Context and behavior implemented + expected before and after the CL
===================================================================

A content::RenderFrameImpl stores |subresource_loader_factories| that
are used for all requests/XHRs/etc initiated by the frame.  We don't
currently swap the RenderFrame when committing another same-process
document (this future work is tracked by https://crbug.com/936696).

The URLLoaderFactory for handling http/https requests contains a
|request_initiator_site_lock| in network::URLLoaderFactoryParams.
This lock limits what network::ResourceRequest::request_initiator may be
used.  The lock is currently used in various, security-sensitive
features like CORB, CORP, Sec-Fetch-Site.  In the future the lock may be
consulted for all requests handled by the NetworkService (see
https://crbug.com/920634 and also https://crbug.com/961614).

When a cross-origin, same-process navigation commits, then:

- The commit IPC carries a bundle of |subresource_loader_factories|.
  The new |subresource_loader_factories| should be used by the newly
  committed document.

- Before committing the new document, the old document's unload
  handler needs to run.  The unload handler needs to use the old
  |subresource_loader_factories| (e.g. because otherwise
  |URLLoaderFactoryParams::request_initiator_site_lock| might
  not match the document origin).


The problematic behavior before this CL
=======================================

Before the CL, content::RenderFrameImpl::CommitNavigationWithParams
would start using the new |subresource_loader_factories| before running
the unload handler of the old document (i.e. before calling
blink::WebNavigationControl::CommitNavigation on |frame_|).

The CL adds WPT tests that fail when the old, incorrect behavior
happens.  The new test initiates a beacon request from a frame's unload
handler and verifies the Sec-Fetch-Site request header associated with
this request.  The header depends on the correct
request_initiator_site_lock / on using the correct URLLoaderFactory.


The fixes in this CL
====================

This CL introduces the |call_before_attaching_new_document| callback
that is taken by blink::WebNavigationControl::CommitNavigation and
called *after* finishing running the old document's unload handler
and *before* the new document can initiate any requests of its own.

This fix is a bit icky.  In the long-term the callback can be avoided if
the |subresource_loader_factories| are stored in a per-document object
(i.e. if we swap RenderFrame on every document change, or if we replace
RenderFrame with a RenderDocument - see https://crbug.com/936696).  The
CL adds a TODO to call this out.

The fix had to refactor BuildServiceWorkerNetworkProviderForNavigation
to make sure that it uses the |new_loader_factories| (even though they
have not yet been passes to SetLoaderFactoryBundle).

The fix also refactored GetLoaderFactoryBundleFromCreator to a separate
method, so that setting the creator-based factories can also be
postponed to |call_before_attaching_new_document|.


Bug: 986577
Change-Id: If0209df61b0305ec43b5179bfdc1b9f8668a88b5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1716084Reviewed-by: Wei Li <weili@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#685290}

components/plugins/renderer/webview_plugin.cc

@@ -6,6 +6,8 @@
 
 #include <stddef.h>
 
+#include <string>
+
 #include "base/auto_reset.h"
 #include "base/bind.h"
 #include "base/location.h"
@@ -383,7 +385,8 @@ void WebViewPlugin::OnZoomLevelChanged() {
 void WebViewPlugin::LoadHTML(const std::string& html_data, const GURL& url) {
   web_view_helper_.main_frame()->CommitNavigation(
       blink::WebNavigationParams::CreateWithHTMLString(html_data, url),
-      nullptr /* extra_data */);
+      nullptr /* extra_data */,
+      base::DoNothing::Once() /* call_before_attaching_new_document */);
 }
 
 void WebViewPlugin::UpdatePluginForNewGeometry(

components/printing/renderer/print_render_frame_helper.cc

@@ -940,7 +940,8 @@ void PrepareFrameAndViewForPrint::CopySelection(
   navigation_control_->CommitNavigation(
       blink::WebNavigationParams::CreateWithHTMLString(
           html, GURL(url::kAboutBlankURL)),
-      nullptr /* extra_data */);
+      nullptr /* extra_data */,
+      base::DoNothing::Once() /* call_before_attaching_new_document */);
 }
 
 blink::WebScreenInfo PrepareFrameAndViewForPrint::GetScreenInfo() {

content/renderer/render_frame_impl.h

@@ -1209,12 +1209,19 @@ class CONTENT_EXPORT RenderFrameImpl
   // is lazily cloned from the parent or opener's own bundle.
   ChildURLLoaderFactoryBundle* GetLoaderFactoryBundle();
 
-  void SetupLoaderFactoryBundle(
+  // Clones and returns the creator's (parent's or opener's)
+  // ChildURLLoaderFactoryBundle.
+  scoped_refptr<ChildURLLoaderFactoryBundle>
+  GetLoaderFactoryBundleFromCreator();
+
+  scoped_refptr<ChildURLLoaderFactoryBundle> CreateLoaderFactoryBundle(
       std::unique_ptr<blink::URLLoaderFactoryBundleInfo> info,
       base::Optional<std::vector<mojom::TransferrableURLLoaderPtr>>
           subresource_overrides,
       mojo::PendingRemote<network::mojom::URLLoaderFactory>
           prefetch_loader_factory);
+  void SetLoaderFactoryBundle(
+      scoped_refptr<ChildURLLoaderFactoryBundle> loader_factories);
 
   // Update current main frame's encoding and send it to browser window.
   // Since we want to let users see the right encoding info from menu
@@ -1436,14 +1443,6 @@ class CONTENT_EXPORT RenderFrameImpl
   // Whether url download should be throttled.
   bool ShouldThrottleDownload();
 
-  // Creates a service worker network provider using browser provided data,
-  // to be supplied to the loader.
-  std::unique_ptr<blink::WebServiceWorkerNetworkProvider>
-  BuildServiceWorkerNetworkProviderForNavigation(
-      blink::mojom::ControllerServiceWorkerInfoPtr
-          controller_service_worker_info,
-      blink::mojom::ServiceWorkerProviderInfoForClientPtr provider_info);
-
   // These functions avoid duplication between Commit*Navigation and
   // Commit*PerNavigationMojoInterfaceNavigation functions.
   void CommitNavigationInternal(
@@ -1735,6 +1734,10 @@ class CONTENT_EXPORT RenderFrameImpl
   // Depending on how the frame was created, |loader_factories_| could be:
   //   * |HostChildURLLoaderFactoryBundle| for standalone frames, or
   //   * |TrackedChildURLLoaderFactoryBundle| for frames opened by other frames.
+  //
+  // This must be updated only via SetLoaderFactoryBundle, which is called at a
+  // certain timing - right before the new document is committed during
+  // FrameLoader::CommitNavigation.
   scoped_refptr<ChildURLLoaderFactoryBundle> loader_factories_;
 
   scoped_refptr<FrameRequestBlocker> frame_request_blocker_;

content/test/test_render_frame.cc

@@ -366,8 +366,9 @@ void TestRenderFrame::BeginNavigation(
     auto navigation_params = blink::WebNavigationParams::CreateWithHTMLString(
         next_navigation_html_override_.value(), info->url_request.Url());
     next_navigation_html_override_ = base::nullopt;
-    frame_->CommitNavigation(std::move(navigation_params),
-                             nullptr /* extra_data */);
+    frame_->CommitNavigation(
+        std::move(navigation_params), nullptr /* extra_data */,
+        base::DoNothing::Once() /* call_before_attaching_new_document */);
     return;
   }
   if (info->navigation_policy == blink::kWebNavigationPolicyCurrentTab &&
@@ -391,8 +392,9 @@ void TestRenderFrame::BeginNavigation(
           navigation_params.get(), blink::WebString::FromUTF8(mime_type),
           blink::WebString::FromUTF8(charset), data);
     }
-    frame_->CommitNavigation(std::move(navigation_params),
-                             nullptr /* extra_data */);
+    frame_->CommitNavigation(
+        std::move(navigation_params), nullptr /* extra_data */,
+        base::DoNothing::Once() /* call_before_attaching_new_document */);
     return;
   }
   RenderFrameImpl::BeginNavigation(std::move(info));

third_party/blink/public/web/web_navigation_control.h

@@ -7,6 +7,7 @@
 
 #include <memory>
 
+#include "base/callback.h"
 #include "base/unguessable_token.h"
 #include "third_party/blink/public/web/web_document_loader.h"
 #include "third_party/blink/public/web/web_frame_load_type.h"
@@ -39,7 +40,8 @@ class WebNavigationControl : public WebLocalFrame {
   // TODO(dgozman): return mojom::CommitResult.
   virtual void CommitNavigation(
       std::unique_ptr<WebNavigationParams> navigation_params,
-      std::unique_ptr<WebDocumentLoader::ExtraData> extra_data) = 0;
+      std::unique_ptr<WebDocumentLoader::ExtraData> extra_data,
+      base::OnceClosure call_before_attaching_new_document) = 0;
 
   // Commits a same-document navigation in the frame. For history navigations, a
   // valid WebHistoryItem should be provided. Returns CommitResult::Ok if the

third_party/blink/renderer/bindings/core/v8/script_controller.cc

@@ -295,7 +295,8 @@ void ScriptController::ExecuteJavaScriptURL(
   String result = ToCoreString(v8::Local<v8::String>::Cast(v8_result));
   WebNavigationParams::FillStaticResponse(params.get(), "text/html", "UTF-8",
                                           StringUTF8Adaptor(result));
-  GetFrame()->Loader().CommitNavigation(std::move(params), nullptr, true);
+  GetFrame()->Loader().CommitNavigation(std::move(params), nullptr,
+                                        base::DoNothing::Once(), true);
 }
 
 void ScriptController::ExecuteScriptInMainWorld(

third_party/blink/renderer/core/frame/frame_test_helpers.cc

@@ -120,7 +120,9 @@ void LoadFrameDontWait(WebLocalFrame* frame, const WebURL& url) {
     params->navigation_timings.fetch_start = base::TimeTicks::Now();
     params->is_browser_initiated = true;
     FillNavigationParamsResponse(params.get());
-    impl->CommitNavigation(std::move(params), nullptr /* extra_data */);
+    impl->CommitNavigation(
+        std::move(params), nullptr /* extra_data */,
+        base::DoNothing::Once() /* call_before_attaching_new_document */);
   }
 }
 
@@ -137,8 +139,9 @@ void LoadHTMLString(WebLocalFrame* frame,
   std::unique_ptr<WebNavigationParams> navigation_params =
       WebNavigationParams::CreateWithHTMLString(html, base_url);
   navigation_params->tick_clock = clock;
-  impl->CommitNavigation(std::move(navigation_params),
-                         nullptr /* extra_data */);
+  impl->CommitNavigation(
+      std::move(navigation_params), nullptr /* extra_data */,
+      base::DoNothing::Once() /* call_before_attaching_new_document */);
   PumpPendingRequestsForFrameToLoad(frame);
 }
 
@@ -154,7 +157,9 @@ void LoadHistoryItem(WebLocalFrame* frame,
   params->navigation_timings.navigation_start = base::TimeTicks::Now();
   params->navigation_timings.fetch_start = base::TimeTicks::Now();
   FillNavigationParamsResponse(params.get());
-  impl->CommitNavigation(std::move(params), nullptr /* extra_data */);
+  impl->CommitNavigation(
+      std::move(params), nullptr /* extra_data */,
+      base::DoNothing::Once() /* call_before_attaching_new_document */);
   PumpPendingRequestsForFrameToLoad(frame);
 }
 
@@ -567,7 +572,9 @@ void TestWebFrameClient::CommitNavigation(
   auto params = WebNavigationParams::CreateFromInfo(*info);
   if (info->archive_status != WebNavigationInfo::ArchiveStatus::Present)
     FillNavigationParamsResponse(params.get());
-  frame_->CommitNavigation(std::move(params), nullptr /* extra_data */);
+  frame_->CommitNavigation(
+      std::move(params), nullptr /* extra_data */,
+      base::DoNothing::Once() /* call_before_attaching_new_document */);
 }
 
 WebEffectiveConnectionType TestWebFrameClient::GetEffectiveConnectionType() {

third_party/blink/renderer/core/frame/mhtml_loading_test.cc

@@ -76,7 +76,9 @@ class MHTMLLoadingTest : public testing::Test {
     body_loader->Write(*buffer);
     body_loader->Finish();
     params->body_loader = std::move(body_loader);
-    frame->CommitNavigation(std::move(params), nullptr /* extra_data */);
+    frame->CommitNavigation(
+        std::move(params), nullptr /* extra_data */,
+        base::DoNothing::Once() /* call_before_attaching_new_document */);
     frame_test_helpers::PumpPendingRequestsForFrameToLoad(frame);
   }
 

third_party/blink/renderer/core/frame/web_local_frame_impl.cc

@@ -2028,13 +2028,15 @@ bool WebLocalFrameImpl::DispatchBeforeUnloadEvent(bool is_reload) {
 
 void WebLocalFrameImpl::CommitNavigation(
     std::unique_ptr<WebNavigationParams> navigation_params,
-    std::unique_ptr<WebDocumentLoader::ExtraData> extra_data) {
+    std::unique_ptr<WebDocumentLoader::ExtraData> extra_data,
+    base::OnceClosure call_before_attaching_new_document) {
   DCHECK(GetFrame());
   DCHECK(!navigation_params->url.ProtocolIs("javascript"));
   if (GetTextFinder())
     GetTextFinder()->ClearActiveFindMatch();
-  GetFrame()->Loader().CommitNavigation(std::move(navigation_params),
-                                        std::move(extra_data));
+  GetFrame()->Loader().CommitNavigation(
+      std::move(navigation_params), std::move(extra_data),
+      std::move(call_before_attaching_new_document));
 }
 
 blink::mojom::CommitResult WebLocalFrameImpl::CommitSameDocumentNavigation(

third_party/blink/renderer/core/frame/web_local_frame_impl.h

@@ -33,6 +33,7 @@
 
 #include <memory>
 #include <set>
+#include <utility>
 
 #include "base/single_thread_task_runner.h"
 #include "mojo/public/cpp/bindings/pending_associated_receiver.h"
@@ -323,7 +324,8 @@ class CORE_EXPORT WebLocalFrameImpl final
   bool DispatchBeforeUnloadEvent(bool) override;
   void CommitNavigation(
       std::unique_ptr<WebNavigationParams> navigation_params,
-      std::unique_ptr<WebDocumentLoader::ExtraData> extra_data) override;
+      std::unique_ptr<WebDocumentLoader::ExtraData> extra_data,
+      base::OnceClosure call_before_attaching_new_document) override;
   blink::mojom::CommitResult CommitSameDocumentNavigation(
       const WebURL&,
       WebFrameLoadType,

third_party/blink/renderer/core/loader/frame_loader.cc

@@ -850,6 +850,7 @@ static bool ShouldNavigate(WebNavigationParams* params, LocalFrame* frame) {
 void FrameLoader::CommitNavigation(
     std::unique_ptr<WebNavigationParams> navigation_params,
     std::unique_ptr<WebDocumentLoader::ExtraData> extra_data,
+    base::OnceClosure call_before_attaching_new_document,
     bool is_javascript_url) {
   DCHECK(frame_->GetDocument());
   DCHECK(Client()->HasWebView());
@@ -940,6 +941,8 @@ void FrameLoader::CommitNavigation(
       return;
   }
 
+  std::move(call_before_attaching_new_document).Run();
+
   CommitDocumentLoader(provisional_document_loader_.Release());
 
   // Load the document if needed.

third_party/blink/renderer/core/loader/frame_loader.h

@@ -33,6 +33,8 @@
 #ifndef THIRD_PARTY_BLINK_RENDERER_CORE_LOADER_FRAME_LOADER_H_
 #define THIRD_PARTY_BLINK_RENDERER_CORE_LOADER_FRAME_LOADER_H_
 
+#include <memory>
+
 #include "base/macros.h"
 #include "third_party/blink/public/common/user_agent/user_agent_metadata.h"
 #include "third_party/blink/public/mojom/loader/request_context_frame_type.mojom-blink.h"
@@ -51,8 +53,6 @@
 #include "third_party/blink/renderer/platform/wtf/forward.h"
 #include "third_party/blink/renderer/platform/wtf/hash_set.h"
 
-#include <memory>
-
 namespace blink {
 
 class ContentSecurityPolicy;
@@ -101,6 +101,8 @@ class CORE_EXPORT FrameLoader final {
   void CommitNavigation(
       std::unique_ptr<WebNavigationParams> navigation_params,
       std::unique_ptr<WebDocumentLoader::ExtraData> extra_data,
+      base::OnceClosure call_before_attaching_new_document =
+          base::DoNothing::Once(),
       bool is_javascript_url = false);
 
   // Called before the browser process is asked to navigate this frame, to mark

third_party/blink/web_tests/external/wpt/fetch/sec-metadata/resources/unload-with-beacon.html

+<!DOCTYPE html>
+<script>
+  // When told, register an unload handler that will trigger a beacon to the
+  // URL given by the sender of the message.
+  window.addEventListener('message', e => {
+      var url = e.data;
+      window.addEventListener('unload', () => {
+          navigator.sendBeacon(url, 'blah');
+      });
+      window.parent.postMessage('navigate-away', '*');
+  });
+</script>

third_party/blink/web_tests/external/wpt/fetch/sec-metadata/unload.tentative.https.sub.html

+<!DOCTYPE html>
+<script src=/resources/testharness.js></script>
+<script src=/resources/testharnessreport.js></script>
+<script src=/resources/testdriver.js></script>
+<script src=/resources/testdriver-vendor.js></script>
+<script src=/fetch/sec-metadata/resources/helper.js></script>
+<script src=/common/utils.js></script>
+<body>
+<script>
+  // The test
+  // 1. Creates a same-origin iframe
+  // 2. Adds to the iframe an unload handler that will
+  //    trigger a request to <unload_request_url>/.../record-header.py...
+  // 3. Navigate the iframe to a cross-origin url (to data: url)
+  // 4. Waits until the request goes through
+  // 5. Verifies Sec-Fetch-Site request header of the request.
+  //
+  // This is a regression test for https://crbug.com/986577.
+  function create_test(unload_request_origin, expectations) {
+    async_test(t => {
+      // STEP 1: Create an iframe.
+      let nonce = token();
+      let key = "unload-test-" + nonce;
+      let url = unload_request_origin +
+          "/fetch/sec-metadata/resources/record-header.py?file=" + key;
+      let i = document.createElement('iframe');
+      i.src = 'resources/unload-with-beacon.html';
+      i.onload = () => {
+          // STEP 2: Ask the iframe to add an unload handler.
+          i.contentWindow.postMessage(url, '*');
+      };
+      window.addEventListener('message', e => {
+          // STEP 3: Navigate the iframe away
+          i.contentWindow.location = 'data:text/html,DONE';
+      });
+      document.body.appendChild(i);
+
+      // STEPS 4 and 5: Wait for the beacon to go through and verify
+      // the request headers.
+      function wait_and_verify() {
+          t.step_timeout(() => {
+              fetch("resources/record-header.py?retrieve=true&file=" + key)
+                  .then(response => response.text())
+                  .then(text => t.step(() => {
+                      if (text == 'No header has been recorded') {
+                        wait_and_verify();
+                        return;
+                      }
+                      assert_header_equals(text, expectations);
+                      t.done();
+                  }))
+          }, 200);
+      }
+      wait_and_verify();
+    }, "Fetch from an unload handler");
+  }
+
+  create_test("https://{{host}}:{{ports[https][0]}}", {
+    "dest": "empty",
+    "site": "same-origin",
+    "user": "",
+    "mode": "no-cors"
+  });
+</script>