GitLab

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}