Use correct URLLoaderFactory in an unload handler.

anforowicz · chromium-wpt-export-bot · commit c0b0a0d72066 · 2019-08-08T15:33:34.000-04:00
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/+/1716084 Reviewed-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}
diff --git a/fetch/sec-metadata/resources/unload-with-beacon.html b/fetch/sec-metadata/resources/unload-with-beacon.html
@@ -0,0 +1,12 @@
+<!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>
diff --git a/fetch/sec-metadata/unload.tentative.https.sub.html b/fetch/sec-metadata/unload.tentative.https.sub.html
@@ -0,0 +1,64 @@
+<!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>
