Skip to content

Update navigation's document creation to properly use fetch callbacks #8095

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

Open
domenic opened this issue Jul 11, 2022 · 4 comments
Open

Update navigation's document creation to properly use fetch callbacks #8095

domenic opened this issue Jul 11, 2022 · 4 comments

Comments

@domenic
Copy link
Member

domenic commented Jul 11, 2022

Even after #6315, it's looking like the creation of Documents by feeding the parser network bytes will not be rigorous. It's going to be something like:

Create an HTML parser and associate it with the document. Each task that the networking task source places on the task queue while fetching runs must then fill the parser's input byte stream with the fetched bytes and cause the HTML parser to perform the appropriate processing of the input stream.

The first task that the networking task source places on the task queue while fetching runs must process link headers given document, navigationParams's response, and "media", after the task has been procesed by the HTML parser.

If any task would result in script execution, then the task queuing must be delayed until
scripts may run for the newly-created document document.

When no more bytes are available, the user agent must queue a global task on the networking task source given document's relevant global object to run the following steps:

This obviously could be better. We should be using Fetch's processEarlyHintsResponse, processResponse, processResponseEndOfBody, and operate on the response body stream.

And, we can ideally be more rigorous about waiting for "scripts may run for the newly-created document", although that'll be especially tricky. Maybe we can just spin the event loop and not feed the parser anything until the session history is updated, once we have properly separated response header processing with processResponse.

Creating this as a tracking issue for that future work.

@domenic
Copy link
Member Author

domenic commented Oct 24, 2022

This might not be so bad. We can just use incrementally read.

A few questions:

  • @noamr do you remember why we delay "process link headers" until after the first task is done feeding the HTML parser? It seems cleaner to do it before. I didn't find any clues in Process Preload link headers #7622 .

  • What happens if we hit processBodyError? This means there was an error reading from the network socket or something. Based on my experiences as a user, I think browsers treat this as an end-of-stream, i.e. processBodyError = processEndOfBody?

@noamr
Copy link
Collaborator

noamr commented Oct 24, 2022

This might not be so bad. We can just use incrementally read.

A few questions:

  • @noamr do you remember why we delay "process link headers" until after the first task is done feeding the HTML parser? It seems cleaner to do it before. I didn't find any clues in Process Preload link headers #7622 .

Some links are processed immediately, and links with media or imagesrcset attributes can only be processed once we know the media state of the document, which can only be computed when we see (or don't see) the viewport meta tag (until we have something like the proposed Viewport header). By reading the first chunk we assume that for the purposes for preloading the viewport attributes of the document are accurate.

This was put in the spec as a reflection of what Chrome/Safari do in practice.

  • What happens if we hit processBodyError? This means there was an error reading from the network socket or something. Based on my experiences as a user, I think browsers treat this as an end-of-stream, i.e. processBodyError = processEndOfBody?

I haven't dived into the streams API but I believe it calls processBodyError and then processEndOfBody because the stream is closed.

@domenic
Copy link
Member Author

domenic commented Oct 24, 2022

By reading the first chunk we assume that for the purposes for preloading the viewport attributes of the document are accurate.

This was put in the spec as a reflection of what Chrome/Safari do in practice.

Oh, ick, so we don't wait for a deterministic point in time like </head>, but instead we use whatever the first network chunk is? That's terrible. But I guess it's a separate issue to investigate and maybe fix...

@noamr
Copy link
Collaborator

noamr commented Oct 24, 2022

By reading the first chunk we assume that for the purposes for preloading the viewport attributes of the document are accurate.
This was put in the spec as a reflection of what Chrome/Safari do in practice.

Oh, ick, so we don't wait for a deterministic point in time like </head>, but instead we use whatever the first network chunk is? That's terrible. But I guess it's a separate issue to investigate and maybe fix...

It's not that bad considering that it's just a preload and doesn't have an observable effect. But yea I'm not a huge fan of this. I'd rather we count on a Viewport header, and in desktop don't even bother with it because the viewport header is not respected. But even before getting to all those details, I'd like to see that people are using link headers and that they make some sort of a difference to someone.

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

No branches or pull requests

3 participants
@noamr @domenic and others