Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The abort-refresh-immediate.window.js test seems to be depending on non-spec behavior #14942

Open
bzbarsky opened this issue Jan 18, 2019 · 2 comments

Comments

@bzbarsky
Copy link
Contributor

As far as I can tell, the test has the following structure (focusing on the HTTP header part for now, and just the fetch case):

  1. Load a URL that sends back a Refresh: 0 header.
  2. When the load event fires, start a fetch, then call document.open.

The test then asserts that the fetch is canceled before it completes, presumably by the document.open call seeing a navigation in progress (from the Refresh) and hence canceling the fetch.

In terms of the spec, this doesn't seem right. https://html.spec.whatwg.org/multipage/semantics.html#shared-declarative-refresh-steps step 13 says to navigate when the refresh has "come due" which is defined as the given number of seconds (0, in this case) elapsing after the document is "completely loaded". This last is set in the task queued at https://html.spec.whatwg.org/multipage/parsing.html#completely-loaded which is step 12 of https://html.spec.whatwg.org/multipage/parsing.html#the-end

But the load event fires from a task queued in step 7 of that same algorithm. Both tasks uses the same task source, so the load event should fire before the navigation caused by the Refresh header is started, and indeed before the refresh timer is started. So per spec as currently written, this test should consistently fail. There's the question of what should happen with "tasks queued to navigate" tracked in whatwg/html#3447 but in this case there is no "task queued to navigate": there's a task queued to mark the document "completely loaded", which will then start a timer, which when it expires will queue a task to navigate....

OK, so what happens in browsers? In at least Firefox when this test passes the sequence of events is as follows:

  1. Document completes loading and fires load event.
  2. The fetch starts.
  3. The document.open call happens, doesn't cancel the fetch because there's no navigation ongoing.
  4. We go back to processing the "complete loading" steps and start the 0s timer for the refresh.
  5. The timer wins the race against the fetch (because it's for 0s, so fires pretty much immediately as soon as we get back to the event loop, while the fetch has to go hit some other process running an HTTP server, etc).
  6. The navigation started by the refresh timer reaches https://html.spec.whatwg.org/multipage/browsing-the-web.html#navigate step 9 and aborts the document, which cancels the fetch.
  7. The cancelation of the fetch and resulting tasks and microtasks win the race against the load started by the refresh timer, so we get to t.step_func_done() before we actually load the new document from the refresh.

The test will fail via hitting the t.unreached_func if the fetch ends up winning the race against the refresh timer in step 5 above.

The test will fail via timeout (because the load handler gets removed during the first load event, so nothing ever ends the test) if the fetch cancellation loses the race against the refresh load in step 7 above.

I'm not really sure what the right thing is here in terms of the spec. The 1s/4s tests in
abort-refresh-multisecond-header.window.js and abort-refresh-multisecond-meta.window.js seem to imply that open() should not do any aborting just because we're expecting to queue a task that will start a timer that when it fires will navigate... So arguably the same should be true for a 0s timer.

Now how to test that, given the race betwen the timer-triggered navigation and its aborting of the document and the fetch... I don't know.

@TimothyGu @domenic @annevk

@TimothyGu
Copy link
Member

TimothyGu commented Jan 20, 2019

This test came out of when I was implementing the aborting behavior in Chrome. In Chrome, when the document.open call happens the refresh has already been queued, which would make document.open abort it as the note explicitly says that tasks queued to navigate should be aborted as well. (This does not seem to be the case in Firefox, as step 4 occurs after step 3.) I then decided to cancel all those refreshes queued that have a timeout of more than 0, as a 0 timeout is basically “going to navigate now.”

Now the question is whether the assumption that declarative refreshes are queued before load event is fair. As much as the spec is concerned, I think so, as Refresh-header refreshes are queued in https://html.spec.whatwg.org/multipage/browsing-the-web.html#initialise-the-document-object, which is before the load event gets fired. But if that assumption turns out to not match implementations, let’s just remove this test.

@bzbarsky
Copy link
Contributor Author

as Refresh-header refreshes are queued in https://html.spec.whatwg.org/multipage/browsing-the-web.html#initialise-the-document-object, which is before the load event gets fired

Sort of. Again, what's queued in there is waiting until some number of seconds (possibly 0) until after the document is "completely loaded", which happens after the load event is fired. That's certainly what Firefox does; I haven't dug into the code in other browsers yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants