Use fetch controller full timing info for navigation timing

[noamr]

Extracts the full timing info from the fetch controller for network navigations.

Adds a dedicated fallback timing info branch for non-fetch navigations (about:srcdoc, javascript:, error pages), ensuring they receive a coarsened navigation start time and a valid PerformanceNavigationTiming entry.

Passing response's cache state as cacheMode.

Fixes the previously unbound navigationTimingType to use navigationParams's navigation timing type.

Closes #12887.

(See WHATWG Working Mode: Changes for more details.)

[annevk]

Oh oops, we created two navigation timing entries before?

[annevk]

This is wrong, no? Shouldn't this be the https://w3c.github.io/hr-time/#dfn-coarsened-shared-current-time given some parameters?

[noamr]

Right. Fixed

[annevk]

AI suggests there is no duplication here and this would impact srcdoc documents and the like.

Claude

  Summary

  Fixes two genuinely broken references (response's timing info no longer exists) plus a never-bound navigationTimingType variable and a duplicate create the navigation timing entry call that has been in the spec since 0a97a81da (2022-10-31). But the
  deduplication silently drops navigation timing entry creation on every fetch-controller-null path, and the new fallback timestamp reads state that doesn't exist yet. Roughly 2 blockers, 6 suggestions, 2 nits. (annevk's inline comment about using
  coarsened shared current time is already addressed by af9744f5e; not repeated below.)
  
  Blockers

  - source:113416-113422 (removal of the old unconditional step) — dropping the unconditional Create the navigation timing entry leaves queue the navigation timing entry with nothing to queue. Navigation Timing defines a Document's navigation timing 
  entry as "initially unset", and queue the navigation timing entry is just "queue document's navigation timing entry". source:149088 runs that unconditionally in the load event steps. With entry creation now confined to the fetch controller is
  non-null branch, nothing is ever created for:
    - srcdoc documents (source:110324)
    - javascript: URL documents (source:108903)
    - UA-generated inline pages, e.g. error pages (source:113878)
    - navigate supplied with a response — object/embed, and every multipart/x-mixed-replace part after the first (source:108476)

  So source:149088 operates on an unset entry, and performance.getEntriesByType("navigation") is empty in those documents. That's normative and observable, not the "duplicated steps" cleanup the PR description implies. Fix: add an else-branch that
  creates the entry from a synthetic fetch timing info whose start time is navigationStartTime (with redirectCount 0), or make the drop explicit and add WPT coverage — there is currently none (navigation-timing/ has no srcdoc/about:blank test).
  - source:113257-113260 — window's relevant settings object's cross-origin isolated capability is read before window has an associated Document. That capability is defined at source:98134-98147 as "realm's agent cluster's cross-origin-isolation mode
  is concrete" and "window's associated Document is allowed to use the cross-origin-isolated feature". window's associated Document is only set at source:113340, 83 steps later. So in the new-Window branch it is unset, and in the initial-about:blank
  reuse branch (source:113176-113187) it is still the old Document — meaning the coarsening decision is made from the initial about:blank document's permissions policy rather than permissionsPolicy (computed at step 2 from the response). Fix:
  capture unsafeNavigationStartTime as the unsafe shared current time at the top of the algorithm, and coarsen it into document's load timing info's navigation start time in a step after source:113341. Nothing reads loadTimingInfo in between, and
  time origin is a getter, so this is safe. (There is precedent for the wart at source:106341, but it doesn't have the reuse-branch problem this one does.)

  Suggestions

  - source:113267-113269 and source:113385-113387 — extract full timing info is now invoked twice on the same controller, bound to two names (fetchTimingInfo, fullTimingInfo) for the same struct. Hoist a single nullable binding and reuse it:
  Let fetchTimingInfo be null.
  If navigationParams's fetch controller is non-null, then set fetchTimingInfo to the result of
  extracting the full timing info from navigationParams's fetch controller.
  Let navigationStartTime be fetchTimingInfo's start time, if fetchTimingInfo is non-null;
  otherwise the coarsened shared current time given ...
  - then change source:113381 to If fetchTimingInfo is non-null: and pass fetchTimingInfo at source:113417. That also removes the need for the two-step Let/Set dance.
  - source:113416-113422 — the argument list still doesn't match Navigation Timing's signature. Create the navigation timing entry takes seven arguments: fetchTiming, redirectCount, navigationType, serviceWorkerTiming, cacheMode, criticalCHRestart,
  bodyInfo. HTML passes five, so bodyInfo lands in cacheMode's position and criticalCHRestart is missing entirely. Pre-existing, but this PR is rewriting exactly this call, so it's the moment to fix it or file it.
  - Cross-spec (Fetch) — extract full timing info asserts non-null, but Fetch only populates full timing info when destination is exactly "document" (Fetch §4.1: "If fetchParams's request's destination is "document", then set fetchParams's
  controller's full timing info to …"). HTML sets the navigation request's destination to the container's local name for nested navigables (source:110501-110503), i.e. "iframe"/"frame". So the assert fails for every nested-navigable navigation, at
  both call sites. Needs a Fetch fix (widen to mode is "navigate", or the document-ish destinations).
  - source:113257 — the fallback timestamp is captured mid-algorithm, after obtaining the browsing context, building the permissions policy, obtaining an agent, creating a realm, and setting up the settings object. Compare create a new browsing 
  context and document, which deliberately captures unsafeContextCreationTime as its second step (source:106276-106277) so the time origin reflects when the work started. Folding this into the Blocker-2 fix above handles both.
  - source:113264 vs source:113382 — "is non-null" vs "is not null" for the identical condition on the same field, two steps apart in the same algorithm. Pick one (both appear in this algorithm already, so it's cosmetic, but not next to each other).
  - PR description — worth noting that this also fixes the never-bound navigationTimingType (used at origin/main:source:113401, never introduced by a Let), and spelling out the normative consequences: srcdoc/javascript:/UA-page documents go from a
  null dereference to a real navigationStart/timeOrigin. Since that's observable, the template checklist and a WPT link should be filled in rather than deleted.

  Nits

  - source:113272 — missing full stop. data-x="fetch-timing-info-start-time">start time</span></p></li> → … start time</span>.</p></li>.
  - Commit messages — Revise, revise, Use correct timestamp need squashing into the PR title plus a body that explains the why (whatwg/html squash-merges from the PR description, so the description is the commit message that lands).

  Overall

  Not mergeable yet: Blocker 1 is a functional regression for srcdoc, javascript:, error pages, object/embed, and multipart continuations, and Blocker 2 means the fallback timestamp's coarsening is computed from a Document that either doesn't exist
  or is the wrong one. Both are fixable in one restructure — capture the unsafe time up front, extract full timing info once into a nullable variable, coarsen after window's associated Document is set, and give the null-controller path its own entry
  creation. The remaining findings are cheap follow-ons. Say the word if you want this trimmed to just the two blockers for posting.

[annevk]

Still has logic errors I'm afraid.

Review

  Summary

  Both previous blockers are fixed — the fallback timestamp is now captured at step 1 and coarsened after window's associated Document exists, and the fetch-controller-null paths get their own entry-creation branch. But the rework introduces three
  new blockers: an unbound variable that the new branch hinges on, and two wrong argument values in the expanded create the navigation timing entry calls. Roughly 3 blockers, 7 suggestions, 3 nits.

  Blockers

  - source:113183 / source:113398 — fetchTimingInfo is never introduced on the path that tests it. It's Let-bound inside the nested <ol> of If navigationParams's fetch controller is not null (source:113182-113190), but If <var>fetchTimingInfo</var> 
  is not null: at source:113398 is at top level. When the fetch controller is null — exactly the case the new "Otherwise" arm exists for — the variable was never bound. Fix: Let fetchTimingInfo be null. next to Let navigationStartTime be null.
  (source:113176), then Set fetchTimingInfo to … inside the branch.
  - source:113428-113432 and source:113455 — the cacheMode argument is the wrong concept, and both branches trip an assert. Navigation Timing hands cacheMode straight to Resource Timing's setup the resource timing entry, whose first step is "Assert
  that cacheMode is the empty string, "local", or "validated"". The fetch branch passes Fetch's request cache mode ("default"/"no-store"/"reload"/"no-cache"/"force-cache"/"only-if-cached") and the otherwise branch passes the literal "default" —
  neither is ever a legal value, so every navigation asserts. The intended value is the response's cache state (Fetch: "the empty string, "local", or "validated"; intended for usage by Service Workers and Resource Timing"). Fix: pass
  navigationParams's response's cache state in the fetch branch and the empty string in the otherwise branch; that also deletes the Let cacheMode step entirely. You'll need to add a cache state entry to the response dependency list at
  source:2705-2716 — HTML doesn't reference it yet.
  - source:113439 and source:113456 — false is passed for criticalCHRestart, which is a DOMHighResTimeStamp. Navigation Timing declares the sixth parameter as "a DOMHighResTimeStamp criticalCHRestart" and assigns it to the criticalCHRestart IDL
  attribute (also DOMHighResTimeStamp). Should be 0 in both branches.

  Suggestions

  - source:113349-113353 — the cross-origin isolated capability argument is dead: it is always false here. Allowed to use (source:36024-36039) returns false at step 2 if the document is not fully active, and fully active (source:106771-106775)
  requires being the navigable's active document — which only happens at Make active in activate history entry (source:112057), long after this algorithm returns. So the fallback time is always coarsely clamped even in a cross-origin isolated agent
  cluster. Same shape as the pre-existing wart at source:106340-106345; worth either a note or computing the capability without going through allowed to use.
  - source:113449 — rename nullFetchTimingInfo; it isn't null. HTML already has this exact construction at source:35685 as fallbackTimingInfo. That precedent also sets end time; leaving it 0 here means responseEnd and duration report 0 for
  srcdoc/javascript:/error-page documents (convert fetch timestamp returns 0 for 0). Confirm that's intended.
  - source:113176-113196 + source:113345-113358 — navigationStartTime is redundant once blocker 1 is fixed: it is non-null exactly when fetchTimingInfo is. Dropping it and reading loadTimingInfo's navigation start time at source:113451 removes one
  variable and one invariant to keep in sync.
  - source:113428-113432 — the "if navigationParams's request is not null" guard contradicts its neighbours. source:113411-113414 and source:113423-113425, in the same branch, dereference navigationParams's request unconditionally. (Moot if you
  switch to the response's cache state.)
  - source:113455 — the otherwise branch passes literal null for serviceWorkerTiming while the fetch branch passes the response's service worker timing info. Both are null in practice for a synthetic response, so using the same expression in both
  branches reads more uniformly.
  - PR description is now stale. "Also remove duplicated steps" no longer describes the change — it now adds an entry-creation branch for the fetch-controller-null paths plus two new arguments. Worth also noting the never-bound navigationTimingType
  fix, and filling in the checklist: srcdoc/javascript:/error-page/object/embed/multipart-continuation documents go from a null dereference to a real navigationStart/timeOrigin and now get a PerformanceNavigationTiming entry. There's no coverage for
  any of that in navigation-timing/ today.
  - Cross-spec (Fetch), carried over — extract full timing info asserts non-null, but Fetch only populates it for destination exactly "document" (Fetch §4.1), while HTML sets the navigation request's destination to the container's local name for
  nested navigables (source:110500-110502). The assert therefore fails for every iframe/frame navigation, at what is now the only call site. Needs a Fetch fix.

  Nits

  - source:113190-113191 — closing tags over-indented by one. </ol> is at 5 spaces (should be 4, matching <ol> at source:113182) and </li> at 4 (should be 3, matching <li> at source:113178).
  - source:113183-113186 — lines aren't greedy-filled (65/82/40/82 columns); source:113185 is 40 columns mid-paragraph.
  - Commit messages Revise, revise, Apply review fixes, typo still need squashing.

  Overall

  Structurally this is now the right shape, and the two hard problems from the last round are genuinely solved. What's left is concentrated in the two new create the navigation timing entry call sites: the cacheMode/criticalCHRestart values are both
  wrong and the fetchTimingInfo binding doesn't exist on the branch that reads it. All three are small, local edits — one more round should do it. Rebasing onto current main would also make the GitHub diff readable again.

[noamr]

Still has logic errors I'm afraid.

Review

Fixed... A lot of these are issues not related directly to response's timing info, so I overlooked them. But I think it's clean now.

[annevk]

I think this generally makes sense. What do you think of this final review:

AI review

  New finding

  - source:113439-113440 — the otherwise branch hardcodes "the empty string" for cacheMode while taking its neighbours from the real response. The same call passes navigationParams's response's service worker timing info and body info, but literal
  empty string for cache state. That branch is not only for synthetic responses: per the note at source:108412-108416, navigate is supplied with a response for the object/embed processing models and for multipart/x-mixed-replace parts after the first
  (source:108476, fetch controller null, request null). Those are real network responses with a meaningful cache state, silently discarded — so PerformanceNavigationTiming's deliveryType will never report "cache" for them. Since a response's cache
  state defaults to the empty string, navigationParams's response's cache state is correct for object/embed/multipart and identical ("") for srcdoc/javascript:/error pages. Fix: use the same expression as the fetch branch (source:113418-113419) in
  both.

  Still open from last round

  - source:113338-113343 — the cross-origin isolated capability argument is always false, because allowed to use (source:36017-36032) requires fully active and document isn't the navigable's active document until Make active in activate history entry
  (source:112050). Worth a note if you don't want to change it.
  - source:113429-113433 — the end time clause is observably a no-op (both the default 0 and the navigation start time convert to 0 through convert fetch timestamp, since the time origin is the navigation start time), and it puts an absolute moment
  in a field Fetch and source:35688-35690 fill with a relative one. Drop it or explain it.
  - Fetch (cross-spec) — extract full timing info asserts non-null, but Fetch only populates full timing info when destination is exactly "document", while HTML sets it to the container's local name for nested navigables (source:110493-110495).
  Assert fails for every iframe/frame navigation at source:113179-113183. Worth filing before this lands.
  - Hygiene — 8 commits to squash; no checklist/WPT link despite the observable change (navigation-timing/ has no srcdoc/about:blank coverage); rebase onto main.
  - Nit, pre-existing — source:113250 and source:113253 use <var>navigable</var>, the one variable in this algorithm that's still unbound (should be navigationParams's navigable).

  Overall

  Nothing regressed, and nothing new is a blocker. The cacheMode asymmetry in the otherwise branch is the one substantive thing I'd want fixed before merge — it's a one-line change that makes both branches identical in that argument and stops
  object/embed/multipart navigations from losing their cache state.

[noamr]

I think this generally makes sense. What do you think of this final review:

AI review

All seems like good suggestions! Done