From 3638c168c6db3f12a8d72f1e70e2d66ced788528 Mon Sep 17 00:00:00 2001
From: Domenic Denicola <domenic@chromium.org>
Date: Wed, 26 Jan 2022 15:45:35 -0800
Subject: [PATCH] App history: tweak and test event/promise ordering

By adding new exhaustive tests under ordering/, it was revealed that the ordering between navigatesuccess/navigateerror and the committed/finished promises was not always consistent:

1. Simply adding a currentchange event handler would cause microtasks to run during commit, which changed some ordering.

2. Calling transitionWhile() would take us from the zero-promise case to the 1+-promise case in ScriptPromise::All(). As the new comment explains, both the spec and implementation have an observably-different fast path for the 0-promise case which caused changes in ordering.

In the course of fixing this, I found out that the did_finish_before_commit_ code in app_history_api_navigation.{h,cc} was actually not a mitigation for the case it stated, where promises passed to transitionWhile() would settle faster than the browser-process roundtrip for same-document traversals. That is in fact impossible, since we only fire the navigate event after the browser-process roundtrip has completed. Instead, they were a mitigation for (1).

This commit then ensures consistent ordering, tested with new rather-exhaustive tests, in the following manner:

* We move the firing of currentchange to before resolving the committed promise. This eliminates (1) and allows us to delete the did_finish_before_commit_ tracking.

* We always ensure we pass 1+ promises to ScriptPromise::All(). This eliminates (2).

A consequence of this is that we are now more likely to get rejected finished promises, in cases like

    await appHistory.navigate("#1").committed;
    await appHistory.navigate("#2").committed;

Before, the finished promise for the #1 navigation would go through the fast path per (2), and fulfill before the navigation to #2 canceled it. Now that does not happen, so code like the above will give an unhandled promise rejection for #1's finished promise.

To avoid this, we unconditionally mark finished promises as handled. This follows some web platform precedent, e.g. stream closed promises, where the promise is one of several information channels (in this case the developer might also find out via the AbortSignal or the navigateerror event). We do *not* do this for the committed promise though, as if a commit fails, that's probably something more deeply wrong, and cannot be ignored.

All of these changes will require spec updates.

For the tests, we introduce a new ordering/ directory which contains cross-cutting ordering tests, and we consolidate a few tests into the newly-introduced variant-driven exhaustive ones. A couple of other tests were affected by these changes too or fixed as a drive-by.

Change-Id: I8a50ca28d009e0a8a2c94331cd17f29b0a8dc463
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3405377
Reviewed-by: Nate Chapin <japhet@chromium.org>
Commit-Queue: Domenic Denicola <domenic@chromium.org>
Cr-Commit-Position: refs/heads/main@{#963772}
---
 ...nge-app-history-back-forward-same-doc.html |  2 +-
 .../transitionWhile-and-navigate.html         |  8 +--
 .../navigate-same-document-event-order.html   | 30 ---------
 ...te-transitionWhile-reject-event-order.html | 40 ------------
 app-history/ordering/README.md                | 24 +++++++
 app-history/ordering/back-same-document.html  | 63 +++++++++++++++++++
 .../currentchange-dispose-ordering.html       |  0
 .../navigate-cross-document-event-order.html  |  0
 ...-same-document-transitionWhile-reject.html | 57 +++++++++++++++++
 .../ordering/navigate-same-document.html      | 61 ++++++++++++++++++
 ...error-ordering-location-api-reentrant.html |  0
 .../navigateerror-ordering-location-api.html  |  0
 ...ror-ordering-navigate-cross-document.html} |  0
 ...or-ordering-transitionWhile-reentrant.html |  0
 ...avigateerror-ordering-transitionWhile.html |  0
 .../resources/notify-top-early.html           |  0
 16 files changed, 210 insertions(+), 75 deletions(-)
 delete mode 100644 app-history/navigate/navigate-same-document-event-order.html
 delete mode 100644 app-history/navigate/navigate-transitionWhile-reject-event-order.html
 create mode 100644 app-history/ordering/README.md
 create mode 100644 app-history/ordering/back-same-document.html
 rename app-history/{currentchange-event => ordering}/currentchange-dispose-ordering.html (100%)
 rename app-history/{navigate => ordering}/navigate-cross-document-event-order.html (100%)
 create mode 100644 app-history/ordering/navigate-same-document-transitionWhile-reject.html
 create mode 100644 app-history/ordering/navigate-same-document.html
 rename app-history/{navigate-event => ordering}/navigateerror-ordering-location-api-reentrant.html (100%)
 rename app-history/{navigate-event => ordering}/navigateerror-ordering-location-api.html (100%)
 rename app-history/{navigate-event/navigateerror-ordering-cross-document.html => ordering/navigateerror-ordering-navigate-cross-document.html} (100%)
 rename app-history/{navigate-event => ordering}/navigateerror-ordering-transitionWhile-reentrant.html (100%)
 rename app-history/{navigate-event => ordering}/navigateerror-ordering-transitionWhile.html (100%)
 rename app-history/{navigate => ordering}/resources/notify-top-early.html (100%)

diff --git a/app-history/currentchange-event/currentchange-app-history-back-forward-same-doc.html b/app-history/currentchange-event/currentchange-app-history-back-forward-same-doc.html
index 85a93a21bd0f4a..9b1b777614b9d2 100644
--- a/app-history/currentchange-event/currentchange-app-history-back-forward-same-doc.html
+++ b/app-history/currentchange-event/currentchange-app-history-back-forward-same-doc.html
@@ -6,7 +6,7 @@
   // Wait for after the load event so that the navigation doesn't get converted
   // into a replace navigation.
   await new Promise(resolve => window.onload = t.step_timeout(resolve, 0));
-  await appHistory.navigate("#foo");
+  await appHistory.navigate("#foo").committed;
   assert_equals(appHistory.entries().length, 2);
 
   let oncurrentchange_back_called = false;
diff --git a/app-history/navigate-event/transitionWhile-and-navigate.html b/app-history/navigate-event/transitionWhile-and-navigate.html
index 66a43b25ec2a9f..65e9d049278436 100644
--- a/app-history/navigate-event/transitionWhile-and-navigate.html
+++ b/app-history/navigate-event/transitionWhile-and-navigate.html
@@ -16,12 +16,12 @@
       assert_equals(appHistory.entries().length, 2);
       appHistory.navigate("#2");
     }
-  }
+  };
   let back_result = appHistory.back();
-  await promise_rejects_dom(t, "AbortError", back_result.committed);
-  await promise_rejects_dom(t, "AbortError", back_result.finished);
+  await back_result.committed;
   assert_equals(location.hash, "#2");
+  await promise_rejects_dom(t, "AbortError", back_result.finished);
   assert_equals(appHistory.current.index, 1);
   assert_equals(appHistory.entries().length, 2);
-}, "Using transitionWhile then navigate() in the ensuing currentchange should abort the transitionWhile");
+}, "Using transitionWhile() then navigate() in the ensuing currentchange should abort the finished promise (but not the committed promise)");
 </script>
diff --git a/app-history/navigate/navigate-same-document-event-order.html b/app-history/navigate/navigate-same-document-event-order.html
deleted file mode 100644
index 4be537fb7ae1c2..00000000000000
--- a/app-history/navigate/navigate-same-document-event-order.html
+++ /dev/null
@@ -1,30 +0,0 @@
-<!doctype html>
-<script src="/resources/testharness.js"></script>
-<script src="/resources/testharnessreport.js"></script>
-<script>
-async_test(t => {
-  let events = [];
-  function finish() {
-    // Until https://github.com/whatwg/html/issues/1792 (see also https://crbug.com/1254926) is
-    // resolved, ignore the ordering of hashchange and popstate (while still testing that they are
-    // the last two, in some order, and come in at most one task after "promisefulfilled").
-    // TODO(domenic): fix the spec, write non-app history tests, then reenable these checks.
-    assert_array_equals(
-      events.slice(0, -2),
-      ["onnavigate", "onnavigatesuccess", "promisefulfilled"/*, "onpopstate", "onhashchange"*/]
-    );
-    t.done();
-  }
-
-  appHistory.onnavigate = () => events.push("onnavigate");
-  appHistory.onnavigatesuccess = () => events.push("onnavigatesuccess");
-  appHistory.onnavigateerror = () => events.push("onnavigateerror");
-  window.onhashchange = t.step_func(() => {
-    events.push("onhashchange");
-    t.step_timeout(finish, 0);
-  });
-  window.onpopstate = () => events.push("onpopstate");
-  appHistory.navigate("#1").committed.then(
-      () => events.push("promisefulfilled"), () => events.push("promiserejected"));
-}, "navigate() event ordering for same-document navigation");
-</script>
diff --git a/app-history/navigate/navigate-transitionWhile-reject-event-order.html b/app-history/navigate/navigate-transitionWhile-reject-event-order.html
deleted file mode 100644
index 96e742766cccc8..00000000000000
--- a/app-history/navigate/navigate-transitionWhile-reject-event-order.html
+++ /dev/null
@@ -1,40 +0,0 @@
-<!doctype html>
-<script src="/resources/testharness.js"></script>
-<script src="/resources/testharnessreport.js"></script>
-<script>
-async_test(t => {
-  let events = [];
-  function finish() {
-    // Until https://github.com/whatwg/html/issues/1792 (see also https://crbug.com/1254926) is
-    // resolved, ignore the ordering of hashchange and popstate (while still testing that they are
-    // the last two, in some order, and come in at most one task after "finished promiserejected").
-    // TODO(domenic): fix the spec, write non-app history tests, then reenable these checks.
-    assert_array_equals(
-      events.slice(0, -2),
-      ["onnavigate", "committed promisefulfilled", "onnavigateerror", "finished promiserejected"/*, "onhashchange", "onpopstate"*/]
-    );
-    t.done();
-  }
-
-  appHistory.onnavigate = e => {
-    events.push("onnavigate");
-    e.transitionWhile(Promise.reject());
-  };
-  appHistory.onnavigatesuccess = () => events.push("onnavigatesuccess");
-  appHistory.onnavigateerror = () => events.push("onnavigateerror");
-  window.onhashchange = () => events.push("onhashchange");
-  window.onpopstate = () => events.push("onpopstate");
-  const result = appHistory.navigate("#1");
-
-  result.committed.then(
-      () => events.push("committed promisefulfilled"),
-      () => events.push("committed promiserejected"));
-
-  result.finished.then(
-      () => events.push("finished promisefulfilled"),
-      () => {
-        events.push("finished promiserejected");
-        t.step_timeout(finish, 0);
-      });
-}, "navigate() event ordering for same-document navigation");
-</script>
diff --git a/app-history/ordering/README.md b/app-history/ordering/README.md
new file mode 100644
index 00000000000000..9ae6c7b19ff77c
--- /dev/null
+++ b/app-history/ordering/README.md
@@ -0,0 +1,24 @@
+# App history ordering tests
+
+These are meant to test the ordering between various events and promises; they
+don't fit into any particular sibling directory.
+
+Some of them test simple cases rather-exhaustively, and others test tricky cases
+(e.g. reentrancy or navigations aborting previous navigations) in a more focused
+way.
+
+Note:
+
+* Variants specifically exist for `currentchange` because an event listener
+  existing for `currentchange` causes code to run, and thus microtasks to run,
+  at a very specific point in the navigation-commit lifecycle. We want to test
+  that it doesn't impact the ordering.
+* Similarly we test that `transitionWhile(Promise.resolve())` does not change
+  the ordering compared to no `transitionWhile()` call, for same-document
+  navigations.
+
+TODOs:
+
+* Also test `appHistory.transition.finished` when it is implemented.
+* Also test `popstate` and `hashchange` once
+  <https://github.com/whatwg/html/issues/1792> is fixed.
diff --git a/app-history/ordering/back-same-document.html b/app-history/ordering/back-same-document.html
new file mode 100644
index 00000000000000..f4a2c382b8b9fd
--- /dev/null
+++ b/app-history/ordering/back-same-document.html
@@ -0,0 +1,63 @@
+<!doctype html>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<meta name="variant" content="?">
+<meta name="variant" content="?transitionWhile">
+<meta name="variant" content="?currentchange">
+<meta name="variant" content="?transitionWhile&currentChange">
+
+<script>
+const variants = new Set((new URLSearchParams(location.search)).keys());
+
+promise_test(async t => {
+  // Wait for after the load event so that the navigation doesn't get converted
+  // into a replace navigation.
+  await new Promise(resolve => window.onload = () => t.step_timeout(resolve, 0));
+
+  await appHistory.navigate("#1").finished;
+
+  const events = [];
+  appHistory.addEventListener("navigatesuccess", () => events.push(`navigatesuccess ${location.hash}`));
+  appHistory.addEventListener("navigateerror", () => events.push(`navigateerror ${location.hash}`));
+  if (variants.has("currentchange")) {
+    appHistory.addEventListener("currentchange", () => events.push(`currentchange ${location.hash}`));
+  }
+
+  if (variants.has("transitionWhile")) {
+    appHistory.addEventListener("navigate", e => {
+      e.transitionWhile(Promise.resolve());
+    });
+  }
+
+  const { committed, finished } = appHistory.back();
+  committed.then(
+    () => events.push(`committed fulfilled ${location.hash}`),
+    () => events.push(`committed rejected ${location.hash}`)
+  );
+  finished.then(
+    () => events.push(`finished fulfilled ${location.hash}`),
+    () => events.push(`finished rejected ${location.hash}`)
+  );
+
+  Promise.resolve().then(() => events.push(`promise microtask ${location.hash}`));
+
+  await finished;
+
+  if (variants.has("currentchange")) {
+    assert_array_equals(events, [
+      "promise microtask #1",
+      "currentchange ",
+      "committed fulfilled ",
+      "navigatesuccess ",
+      "finished fulfilled "
+    ]);
+  } else {
+    assert_array_equals(events, [
+      "promise microtask #1",
+      "committed fulfilled ",
+      "navigatesuccess ",
+      "finished fulfilled "
+    ]);
+  }
+}, "event and promise ordering for same-document appHistory.back()");
+</script>
diff --git a/app-history/currentchange-event/currentchange-dispose-ordering.html b/app-history/ordering/currentchange-dispose-ordering.html
similarity index 100%
rename from app-history/currentchange-event/currentchange-dispose-ordering.html
rename to app-history/ordering/currentchange-dispose-ordering.html
diff --git a/app-history/navigate/navigate-cross-document-event-order.html b/app-history/ordering/navigate-cross-document-event-order.html
similarity index 100%
rename from app-history/navigate/navigate-cross-document-event-order.html
rename to app-history/ordering/navigate-cross-document-event-order.html
diff --git a/app-history/ordering/navigate-same-document-transitionWhile-reject.html b/app-history/ordering/navigate-same-document-transitionWhile-reject.html
new file mode 100644
index 00000000000000..84e95eea752595
--- /dev/null
+++ b/app-history/ordering/navigate-same-document-transitionWhile-reject.html
@@ -0,0 +1,57 @@
+<!doctype html>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<meta name="variant" content="?">
+<meta name="variant" content="?currentchange">
+
+<script>
+const variants = new Set((new URLSearchParams(location.search)).keys());
+
+promise_test(async t => {
+  // Wait for after the load event so that the navigation doesn't get converted
+  // into a replace navigation.
+  await new Promise(resolve => window.onload = () => t.step_timeout(resolve, 0));
+
+  const events = [];
+  appHistory.addEventListener("navigatesuccess", () => events.push(`navigatesuccess ${location.hash}`));
+  appHistory.addEventListener("navigateerror", () => events.push(`navigateerror ${location.hash}`));
+  if (variants.has("currentchange")) {
+    appHistory.addEventListener("currentchange", () => events.push(`currentchange ${location.hash}`));
+  }
+
+  appHistory.addEventListener("navigate", e => {
+    e.transitionWhile(Promise.reject());
+  });
+
+  const { committed, finished } = appHistory.navigate("#1");
+  committed.then(
+    () => events.push(`committed fulfilled ${location.hash}`),
+    () => events.push(`committed rejected ${location.hash}`)
+  );
+  finished.then(
+    () => events.push(`finished fulfilled ${location.hash}`),
+    () => events.push(`finished rejected ${location.hash}`)
+  );
+
+  Promise.resolve().then(() => events.push(`promise microtask ${location.hash}`));
+
+  await finished.catch(() => {});
+
+  if (variants.has("currentchange")) {
+    assert_array_equals(events, [
+      "currentchange #1",
+      "committed fulfilled #1",
+      "promise microtask #1",
+      "navigateerror #1",
+      "finished rejected #1"
+    ]);
+  } else {
+    assert_array_equals(events, [
+      "committed fulfilled #1",
+      "promise microtask #1",
+      "navigateerror #1",
+      "finished rejected #1"
+    ]);
+  }
+}, "event and promise ordering for appHistory.navigate() with a rejected promise passed to transitionWhile()");
+</script>
diff --git a/app-history/ordering/navigate-same-document.html b/app-history/ordering/navigate-same-document.html
new file mode 100644
index 00000000000000..0d30eeddfb67da
--- /dev/null
+++ b/app-history/ordering/navigate-same-document.html
@@ -0,0 +1,61 @@
+<!doctype html>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<meta name="variant" content="?">
+<meta name="variant" content="?transitionWhile">
+<meta name="variant" content="?currentchange">
+<meta name="variant" content="?transitionWhile&currentChange">
+
+<script>
+const variants = new Set((new URLSearchParams(location.search)).keys());
+
+promise_test(async t => {
+  // Wait for after the load event so that the navigation doesn't get converted
+  // into a replace navigation.
+  await new Promise(resolve => window.onload = () => t.step_timeout(resolve, 0));
+
+  const events = [];
+  appHistory.addEventListener("navigatesuccess", () => events.push(`navigatesuccess ${location.hash}`));
+  appHistory.addEventListener("navigateerror", () => events.push(`navigateerror ${location.hash}`));
+  if (variants.has("currentchange")) {
+    appHistory.addEventListener("currentchange", () => events.push(`currentchange ${location.hash}`));
+  }
+
+  if (variants.has("transitionWhile")) {
+    appHistory.addEventListener("navigate", e => {
+      e.transitionWhile(Promise.resolve());
+    });
+  }
+
+  const { committed, finished } = appHistory.navigate("#1");
+  committed.then(
+    () => events.push(`committed fulfilled ${location.hash}`),
+    () => events.push(`committed rejected ${location.hash}`)
+  );
+  finished.then(
+    () => events.push(`finished fulfilled ${location.hash}`),
+    () => events.push(`finished rejected ${location.hash}`)
+  );
+
+  Promise.resolve().then(() => events.push(`promise microtask ${location.hash}`));
+
+  await finished;
+
+  if (variants.has("currentchange")) {
+    assert_array_equals(events, [
+      "currentchange #1",
+      "committed fulfilled #1",
+      "promise microtask #1",
+      "navigatesuccess #1",
+      "finished fulfilled #1"
+    ]);
+  } else {
+    assert_array_equals(events, [
+      "committed fulfilled #1",
+      "promise microtask #1",
+      "navigatesuccess #1",
+      "finished fulfilled #1"
+    ]);
+  }
+}, "event and promise ordering for same-document appHistory.navigate()");
+</script>
diff --git a/app-history/navigate-event/navigateerror-ordering-location-api-reentrant.html b/app-history/ordering/navigateerror-ordering-location-api-reentrant.html
similarity index 100%
rename from app-history/navigate-event/navigateerror-ordering-location-api-reentrant.html
rename to app-history/ordering/navigateerror-ordering-location-api-reentrant.html
diff --git a/app-history/navigate-event/navigateerror-ordering-location-api.html b/app-history/ordering/navigateerror-ordering-location-api.html
similarity index 100%
rename from app-history/navigate-event/navigateerror-ordering-location-api.html
rename to app-history/ordering/navigateerror-ordering-location-api.html
diff --git a/app-history/navigate-event/navigateerror-ordering-cross-document.html b/app-history/ordering/navigateerror-ordering-navigate-cross-document.html
similarity index 100%
rename from app-history/navigate-event/navigateerror-ordering-cross-document.html
rename to app-history/ordering/navigateerror-ordering-navigate-cross-document.html
diff --git a/app-history/navigate-event/navigateerror-ordering-transitionWhile-reentrant.html b/app-history/ordering/navigateerror-ordering-transitionWhile-reentrant.html
similarity index 100%
rename from app-history/navigate-event/navigateerror-ordering-transitionWhile-reentrant.html
rename to app-history/ordering/navigateerror-ordering-transitionWhile-reentrant.html
diff --git a/app-history/navigate-event/navigateerror-ordering-transitionWhile.html b/app-history/ordering/navigateerror-ordering-transitionWhile.html
similarity index 100%
rename from app-history/navigate-event/navigateerror-ordering-transitionWhile.html
rename to app-history/ordering/navigateerror-ordering-transitionWhile.html
diff --git a/app-history/navigate/resources/notify-top-early.html b/app-history/ordering/resources/notify-top-early.html
similarity index 100%
rename from app-history/navigate/resources/notify-top-early.html
rename to app-history/ordering/resources/notify-top-early.html