Navigation API: ensure that 204/205/downloads never settle

See WICG/navigation-api#137. This requires distinguishing between "dropped" navigations and other canceled navigations at the FrameLoader/navigation API interface. This also makes cross-document ongoing navigations generally abortable, as can be seen by the revamped navigate-cross-document-double.html test. Previously, we would clean up the ongoing navigation for any cross-document nav, but per spec we need to keep it around so that it can be aborted if someone interrupts it. (This applies both to "normal" cross-document navigations, and ones to 204s/etc.) Bug: 1183545 Change-Id: I6f8f1771816ddf454d651f7147d3179fba88979b
web-platform-tests · Mar 22, 2022 · b2aa133 · b2aa133
1 parent aaa06bb
commit b2aa133
Show file tree

Hide file tree

Showing 6 changed files with 245 additions and 28 deletions.
diff --git a/navigation-api/navigation-methods/return-value/back-204-205-download.html b/navigation-api/navigation-methods/return-value/back-204-205-download.html
@@ -0,0 +1,52 @@
+<!doctype html>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="resources/helpers.js"></script>
+<script src="/common/utils.js"></script>
+
+<body>
+<script>
+const tests = [
+  ["204s", "204"],
+  ["205s", "205"],
+  ["Content-Disposition: attachment responses", "download"]
+];
+
+for (const [description, action] of tests) {
+  promise_test(async t => {
+    const id = token();
+
+    const i = document.createElement("iframe");
+    i.src = `resources/204-205-download-on-second-visit.py?id=${id}`;
+    document.body.append(i);
+    await new Promise(r => i.onload = r);
+
+    // Configure it to return a 204 on the next visit
+    await fetch(i.src + `&action=${action}`, { method: "POST" });
+
+    // Now navigate elsewhere
+    i.contentWindow.location.href = "/common/blank.html";
+    await new Promise(r => i.onload = r);
+
+    // Now try going back. It should do nothing (and not tell us about the result).
+
+    const indexBefore = i.contentWindow.navigation.currentEntry.index;
+
+    // One might be surprised that navigate does not fire. (It does fire for the
+    // corresponding tests of navigation.navigate(), i.e., this is
+    // traversal-specific behavior.) See https://github.com/WICG/navigation-api/issues/207
+    // for some discussion.
+    i.contentWindow.navigation.onnavigate = t.unreached_func("onnavigate should not be called");
+    i.contentWindow.navigation.onnavigatesuccess = t.unreached_func("onnavigatesuccess should not be called");
+    i.contentWindow.navigation.onnavigateerror = t.unreached_func("onnavigateerror should not be called");
+
+    const result = i.contentWindow.navigation.back();
+
+    assertNeverSettles(t, result, i.contentWindow);
+
+    await new Promise(resolve => t.step_timeout(resolve, 50));
+    assert_equals(i.contentWindow.navigation.currentEntry.index, indexBefore);
+    assert_equals(i.contentWindow.navigation.transition, null);
+  }, `back() promises to ${description} never settle`);
+}
+</script>
diff --git a/navigation-api/navigation-methods/return-value/navigate-204-205-download.html b/navigation-api/navigation-methods/return-value/navigate-204-205-download.html
@@ -0,0 +1,42 @@
+<!doctype html>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="resources/helpers.js"></script>
+
+<body>
+<script>
+const tests = [
+  ["204s", "/common/blank.html?pipe=status(204)"],
+  ["205s", "/common/blank.html?pipe=status(205)"],
+  ["Content-Disposition: attachment responses", "/common/blank.html?pipe=header(Content-Disposition,attachment)"]
+];
+
+for (const [description, url] of tests) {
+  promise_test(async t => {
+    const i = document.createElement("iframe");
+    i.src = "/common/blank.html";
+    document.body.append(i);
+    await new Promise(resolve => i.onload = resolve);
+
+    // This seems to be important? Without it the (outer) window load event
+    // doesn't fire, and the test harness hangs forever. This is probably a
+    // Chromium bug, but maybe a testharness bug, especially since explicit_done
+    // doesn't seem to help?
+    t.add_cleanup(() => i.remove());
+
+    let navigateCount = 0;
+    i.contentWindow.navigation.onnavigate = () => { ++navigateCount; };
+    i.contentWindow.navigation.onnavigatesuccess = t.unreached_func("onnavigatesuccess should not be called");
+    i.contentWindow.navigation.onnavigateerror = t.unreached_func("onnavigateerror should not be called");
+
+    const result = i.contentWindow.navigation.navigate(url);
+
+    assert_equals(navigateCount, 1);
+    assertNeverSettles(t, result, i.contentWindow);
+
+    await new Promise(resolve => t.step_timeout(resolve, 50));
+    assert_equals(i.contentWindow.location.href, i.src);
+    assert_equals(i.contentWindow.navigation.transition, null);
+  }, `navigate() promises to ${description} never settle`);
+}
+</script>
diff --git a/navigation-api/navigation-methods/return-value/resources/204-205-download-on-second-visit.py b/navigation-api/navigation-methods/return-value/resources/204-205-download-on-second-visit.py
@@ -0,0 +1,24 @@
+def main(request, response):
+    key = request.GET[b"id"]
+
+    # If hit with a POST with ?action=X, store X in the stash
+    if request.method == "POST":
+      action = request.GET[b"action"]
+      request.server.stash.put(key, action)
+
+      return (204, [], "")
+
+    # If hit with a GET, either return a normal initial page, or the abnormal requested response
+    elif request.method == "GET":
+      action = request.server.stash.take(key)
+
+      if action is None:
+          return (200, [("Content-Type", "text/html"), ("Cache-Control", "no-store")], "initial page")
+      if action == b"204":
+          return (204, [], "")
+      if action == b"205":
+          return (205, [], "")
+      if action == b"download":
+          return (200, [("Content-Type", "text/plain"), ("Content-Disposition", "attachment")], "some text to download")
+
+    return (400, [], "")
diff --git a/navigation-api/ordering-and-transition/navigate-204-205-download-then-same-document.html b/navigation-api/ordering-and-transition/navigate-204-205-download-then-same-document.html
@@ -0,0 +1,66 @@
+<!doctype html>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+
+<script type="module">
+import { Recorder } from "./resources/helpers.mjs";
+
+const tests = [
+  ["204s", "/common/blank.html?pipe=status(204)"],
+  ["205s", "/common/blank.html?pipe=status(205)"],
+  ["Content-Disposition: attachment responses", "/common/blank.html?pipe=header(Content-Disposition,attachment)"]
+];
+
+for (const [description, url] of tests) {
+  promise_test(async t => {
+    const i = document.createElement("iframe");
+    i.src = "/common/blank.html";
+    document.body.append(i);
+    await new Promise(resolve => i.onload = () => t.step_timeout(resolve, 0));
+
+    const fromStart = i.contentWindow.navigation.currentEntry;
+
+    const recorder = new Recorder({
+      window: i.contentWindow,
+      finalExpectedEvent: "finished fulfilled 2"
+    });
+
+    recorder.setUpNavigationAPIListeners();
+
+    const result1 = i.contentWindow.navigation.navigate(url);
+    recorder.setUpResultListeners(result1, " 1");
+
+    // Give the server time to send the response. This is not strictly
+    // necessary (the expectations are the same either way) but it's better
+    // coverage if the server is done responding by this time; it guarantees
+    // we're hitting the code path for "got a 204/etc. and ignored it" instead
+    // of "didn't get a response yet".
+    await new Promise(resolve => t.step_timeout(resolve, 50));
+
+    const result2 = i.contentWindow.navigation.navigate("#1");
+    recorder.setUpResultListeners(result2, " 2");
+
+    Promise.resolve().then(() => recorder.record("promise microtask"));
+
+    await recorder.readyToAssert;
+
+    recorder.assert([
+      /* event name, location.hash value, navigation.transition properties */
+      ["navigate", "", null],
+      ["AbortSignal abort", "", null],
+      ["navigateerror", "", null],
+
+      ["navigate", "", null],
+      ["currententrychange", "#1", null],
+      ["committed rejected 1", "#1", null],
+      ["finished rejected 1", "#1", null],
+      ["committed fulfilled 2", "#1", null],
+      ["promise microtask", "#1", null],
+      ["navigatesuccess", "#1", null],
+      ["finished fulfilled 2", "#1", null]
+    ]);
+
+    recorder.assertErrorsAreAbortErrors();
+  }, `event and promise ordering when navigate() is to a ${description} and then to a same-document navigation`);
+}
+</script>
diff --git a/navigation-api/ordering-and-transition/navigate-cross-document-double.html b/navigation-api/ordering-and-transition/navigate-cross-document-double.html
@@ -2,22 +2,48 @@
 <script src="/resources/testharness.js"></script>
 <script src="/resources/testharnessreport.js"></script>
 <iframe id="i" src="/common/blank.html"></iframe>
-<script>
+
+
+<script type="module">
+import { Recorder } from "./resources/helpers.mjs";
+
 promise_test(async t => {
-  await new Promise(resolve => window.onload = resolve);
-  let start_href = i.contentWindow.location.href;
-  let events = [];
-  i.contentWindow.navigation.onnavigateerror = () => events.push("navigateerror");
-  i.contentWindow.navigation.onnavigate = t.step_func(e => {
-    assert_equals(i.contentWindow.location.href, start_href);
-    events.push("navigate");
-    e.signal.onabort = () => events.push("abort");
+  await new Promise(resolve => i.onload = () => t.step_timeout(resolve, 0));
+
+  const fromStart = i.contentWindow.navigation.currentEntry;
+
+  const recorder = new Recorder({
+    window: i.contentWindow,
+    finalExpectedEvent: "promise microtask"
   });
-  i.contentWindow.navigation.navigate("?1").committed.finally(t.unreached_func("navigate to ?1"));
-  i.contentWindow.navigation.navigate("?2").committed.finally(t.unreached_func("navigate to ?2"));
-  assert_equals(i.contentWindow.location.search, "");
-  await new Promise(resolve => i.onload = resolve);
-  assert_equals(i.contentWindow.location.search, "?2");
-  assert_array_equals(events, ["navigate", "abort", "navigateerror", "navigate"]);
-}, "navigateerror ordering when navigate() is called repeatedly (cross-document)");
+
+  recorder.setUpNavigationAPIListeners();
+
+  // Use https://web-platform-tests.org/writing-tests/server-pipes.html to make
+  // sure the response doesn't come back quickly, since once the response comes
+  // back the page would be unloaded and that would break our test.
+  const result1 = i.contentWindow.navigation.navigate("?pipe=trickle(d100)");
+  recorder.setUpResultListeners(result1, " 1");
+
+  const result2 = i.contentWindow.navigation.navigate("?2");
+  recorder.setUpResultListeners(result2, " 2");
+
+  Promise.resolve().then(() => recorder.record("promise microtask"));
+
+  await recorder.readyToAssert;
+
+  recorder.assert([
+    /* event name, location.hash value, navigation.transition properties */
+    ["navigate", "", null],
+    ["AbortSignal abort", "", null],
+    ["navigateerror", "", null],
+
+    ["navigate", "", null],
+    ["committed rejected 1", "", null],
+    ["finished rejected 1", "", null],
+    ["promise microtask", "", null]
+  ]);
+
+  recorder.assertErrorsAreAbortErrors();
+}, "event and promise ordering when navigate() is called to a cross-document destination, interrupting another navigate() to a cross-document destination");
 </script>
diff --git a/navigation-api/ordering-and-transition/resources/helpers.mjs b/navigation-api/ordering-and-transition/resources/helpers.mjs
@@ -7,6 +7,9 @@ export function hasVariant(name) {
 export class Recorder {
   #events = [];
   #errors = [];
+  #navigationAPI;
+  #domExceptionConstructor;
+  #location;
   #skipCurrentChange;
   #finalExpectedEvent;
   #finalExpectedEventCount;
@@ -15,43 +18,47 @@ export class Recorder {
   #readyToAssertResolve;
   #readyToAssertPromise = new Promise(resolve => { this.#readyToAssertResolve = resolve; });
 
-  constructor({ skipCurrentChange = false, finalExpectedEvent, finalExpectedEventCount = 1 }) {
+  constructor({ window = self, skipCurrentChange = false, finalExpectedEvent, finalExpectedEventCount = 1 }) {
     assert_equals(typeof finalExpectedEvent, "string", "Must pass a string for finalExpectedEvent");
 
+    this.#navigationAPI = window.navigation;
+    this.#domExceptionConstructor = window.DOMException;
+    this.#location = window.location;
+
     this.#skipCurrentChange = skipCurrentChange;
     this.#finalExpectedEvent = finalExpectedEvent;
     this.#finalExpectedEventCount = finalExpectedEventCount;
   }
 
   setUpNavigationAPIListeners() {
-    navigation.addEventListener("navigate", e => {
+    this.#navigationAPI.addEventListener("navigate", e => {
       this.record("navigate");
 
       e.signal.addEventListener("abort", () => {
         this.recordWithError("AbortSignal abort", e.signal.reason);
       });
     });
 
-    navigation.addEventListener("navigateerror", e => {
+    this.#navigationAPI.addEventListener("navigateerror", e => {
       this.recordWithError("navigateerror", e.error);
 
-      navigation.transition?.finished.then(
+      this.#navigationAPI.transition?.finished.then(
         () => this.record("transition.finished fulfilled"),
         err => this.recordWithError("transition.finished rejected", err)
       );
     });
 
-    navigation.addEventListener("navigatesuccess", () => {
+    this.#navigationAPI.addEventListener("navigatesuccess", () => {
       this.record("navigatesuccess");
 
-      navigation.transition?.finished.then(
+      this.#navigationAPI.transition?.finished.then(
         () => this.record("transition.finished fulfilled"),
         err => this.recordWithError("transition.finished rejected", err)
       );
     });
 
     if (!this.#skipCurrentChange) {
-      navigation.addEventListener("currententrychange", () => this.record("currententrychange"));
+      this.#navigationAPI.addEventListener("currententrychange", () => this.record("currententrychange"));
     }
   }
 
@@ -68,12 +75,12 @@ export class Recorder {
   }
 
   record(name) {
-    const transitionProps = navigation.transition === null ? null : {
-      from: navigation.transition.from,
-      navigationType: navigation.transition.navigationType
+    const transitionProps = this.#navigationAPI.transition === null ? null : {
+      from: this.#navigationAPI.transition.from,
+      navigationType: this.#navigationAPI.transition.navigationType
     };
 
-    this.#events.push({ name, location: location.hash, transitionProps });
+    this.#events.push({ name, location: this.#location.hash, transitionProps });
 
     if (name === this.#finalExpectedEvent && ++this.#currentFinalEventCount === this.#finalExpectedEventCount) {
       this.#readyToAssertResolve();
@@ -169,7 +176,7 @@ export class Recorder {
 
     // Assume assert() has been called so all error objects are the same.
     const { errorObject } = this.#errors[0];
-    assert_throws_dom("AbortError", () => { throw errorObject; });
+    assert_throws_dom("AbortError", this.#domExceptionConstructor, () => { throw errorObject; });
   }
 
   assertErrorsAre(expectedErrorObject) {