Behaviour of "reload" navigationType in " inner navigate event firing algorithm" algorithm

[rwlbuis]

What is the issue with the HTML Standard?

In https://html.spec.whatwg.org/multipage/nav-history-apis.html#inner-navigate-event-firing-algorithm step 32.7, only navigationType "push" and "replace" are stated to run "URL and history update steps". However in the chromium implementation, it is also run for "reload" navigationType, and some WPT tests assume this behavior in order for them to pass. Shouldn't this be reflected in the spec?

Relevant commit from chromium's navigate_event.cc:
" // In the spec, the URL and history update steps are not called for reloads.
// In our implementation, we call the corresponding function anyway ."

[annevk]

cc @domenic

[domenic]

You omitted the important part of that comment:

  // In the spec, the URL and history update steps are not called for reloads.
  // In our implementation, we call the corresponding function anyway, but
  // |type| being a reload type makes it do none of the spec-relevant
  // steps. Instead it does stuff like the loading spinner and use counters.

Can you state which part of the URL and history update steps are run in Chromium, which the WPTs expect, but are not run in the spec? I cannot find any browsing the code myself.

[rwlbuis]

You omitted the important part of that comment:

  // In the spec, the URL and history update steps are not called for reloads.
  // In our implementation, we call the corresponding function anyway, but
  // |type| being a reload type makes it do none of the spec-relevant
  // steps. Instead it does stuff like the loading spinner and use counters.

Can you state which part of the URL and history update steps are run in Chromium, which the WPTs expect, but are not run in the spec? I cannot find any browsing the code myself.

Apologies for the omission, I mainly did it because the comment is a bit long, but it was misleading to leave that out.

Having said that, the comment may be wrong. This is what happens on navigation-methods/return-value/reload-intercept.html (I set a bt in NotifyAboutTheCommittedToEntry):

#1 0x798de3f50a7a base::debug::StackTrace::StackTrace()
#2 0x798dddc11e07 blink::NavigationApiMethodTracker::NotifyAboutTheCommittedToEntry()
#3 0x798dddc13df1 blink::NavigationApi::UpdateForNavigation()
#4 0x798dddb67416 blink::DocumentLoader::UpdateForSameDocumentNavigation()
#5 0x798dddb66db1 blink::DocumentLoader::RunURLAndHistoryUpdateSteps()
#6 0x798dddc0e88a blink::NavigateEvent::CommitNow()
#7 0x798dddc0eab4 blink::NavigateEvent::MaybeCommitImmediately()
#8 0x798dddc17fff blink::NavigationApi::DispatchNavigateEvent()
#9 0x798dddb8fbf2 blink::FrameLoader::StartNavigation()
#10 0x798ddd3da6ce blink::LocalFrame::Navigate()
#11 0x798dddc165ba blink::NavigationApi::PerformNonTraverseNavigation()
#12 0x798dddc16a78 blink::NavigationApi::reload()
#13 0x798dde56fd80 blink::(anonymous namespace)::v8_navigation::ReloadOperationCallback()
#14 0x798dd7ca666e Builtins_CallApiCallbackGeneric

I also debugged that the used frame load type is "reload" in the above scenario. WDYT?

[domenic]

Sorry for losing track of this over the holidays.

So the question is, does the current spec have a path from the "inner navigate event firing algorithm" to "notify about the committed-to entry" in the intercepted reload case.

I think you are right this is broken. This also came up in #10919.

• Event's interception state is not "none", so we return false in step 37.
• We return to "fire a push/replace/reload navigate event" step 8, and pass that false return value onward.
• We return to "reload" step 1.4, and in step 1.5, we bail out. Nothing happens. Oops.

So yeah, I think for reloads we're definitely missing something. We might also be missing something for same-document traverses; I'm unsure. Did you run into something similar for traverses?

It seems like we'll need to add something to the spec that encapsulates the parts of Blink's UpdateForSameDocumentNavigation that get run here. Stepping through the code in a live example would be better, but just trying to run it in my head, for reloads it seems like the relevant parts correspond to the spec's:

• update document for history step application (passing along previousEntryForActivation and doNotReactivate = true).
• update the navigation API entries for a same-document navigation.

@noamr, would you be able to help getting this resolved on the spec side? @rwlbuis, do you still have cycles to review fixes in this area?

[noamr]

I went through the spec and I think the closest equivalent to what we do in chrome is to extend the condition and call URL and history update steps also for reloads. I don't understand what makes this "not appropriate".

[natechapin]

I went through the spec and I think the closest equivalent to what we do in chrome is to extend the condition and call URL and history update steps also for reloads. I don't understand what makes this "not appropriate".

I think that may be the correct solution, but I think the URL and history update steps are currently not resilient to historyHandling being set to "reload". In particular, I'm not sure Finalize a same-document navigation should be called in the "reload" case.

[domenic]

Right, I feel like most of the steps would end up being skipped. E.g. you're not creating a new entry; you're reloading the existing entry.

[noamr]

Right, I feel like most of the steps would end up being skipped. E.g. you're not creating a new entry; you're reloading the existing entry.

OK, so it's two different spec-designs for roughly the same thing.
I wonder if in the future when we add things to these same-document steps, would we benefit from them being centralized with per-type exceptions, or from being split, partially duplicated with nuances? The chromium implementation suggests the former but I don't have a strong opinion.

[domenic]

I think it has to be case-by-case. If you're skipping most of the steps, then that's a sign separate algorithms would be better. If you're skipping one or two, then branches are reasonable.

In this case, I think you'd skip most of the steps, and the common steps are probably just the two I listed in #10621 (comment).

[noamr]

OK I went through all the steps a few times, and I think #10989 is what this comes down to.