Closed Bug 1393540 Opened 7 years ago Closed 5 years ago

rel=preload and our speculative load queue

Categories

(Core :: DOM: Core & HTML, enhancement, P3)

Product:
Core
Component:
DOM: Core & HTML
Version:
57 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED DUPLICATE of bug 1594449
Tracking Flags:
Tracking Status
firefox57 --- affected

People

(Reporter: dragana, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

part 1 - Add "as" attribute to html parser
240.83 KB, patch
Details | Diff | Splinter Review
part 2 - Hook link rel=preload to HTML parser
4.25 KB, patch
hsivonen
: feedback-
Details | Diff | Splinter Review 
Working on rel=preload for no-cache resources I discovered that rel=preload link is not added to our speculative load queue:
https://dxr.mozilla.org/mozilla-central/rev/f0abd25e1f4acced652d180c34b7c9eda638deb1/parser/html/nsHtml5TreeBuilderCppSupplement.h#203

therefore, for example:
...
<link rel=preload href="resources/dummy.js" as=script>
....
<script src="resources/dummy.js"></script>

will actually first trigger load for the script element instead of the link element.
I m not sure if we should add rel=preload here... I am looking.
I did not know that this code exist. maybe we can use this for rel=preload. We will need to add some resources. This can preload scripts, images, style.
This will not work for link header, but I could add that.

Olli, can we use this, e.g .mDocument->ScriptLoad()->PreloadURI(), for rel=preload.
I did not this code exists, at the first glance I do not see reason why not.
Flags: needinfo?(bugs)
But only for scripts.
hsivonen would know more about speculative loading.
Flags: needinfo?(bugs)
Flags: needinfo?(hsivonen)
The relevant code is at https://searchfox.org/mozilla-central/source/parser/html/nsHtml5TreeBuilderCppSupplement.h#138 . It runs on the parser thread.

You'd need to add code there to check for <link rel=preload as=foo> and append items to mSpeculativeLoadQueue accordingly.

When the queue is consumed on the main thread, the methods starting at https://searchfox.org/mozilla-central/source/parser/html/nsHtml5TreeOpExecutor.cpp#949 get called. I take it that rel=preload as=foo doesn't add new semantics to those, right?
Flags: needinfo?(hsivonen)
Out of curiosity, do we have evidence that rel=preload as=foo is needed in Firefox, i.e. that the parser doesn't find the URLs on its own quickly enough? I wonder to what extent the feature is motivated by Blink and WebKit having a less comprehensive speculative parser than we do.
(In reply to Henri Sivonen (:hsivonen) from comment #5)
> The relevant code is at
> https://searchfox.org/mozilla-central/source/parser/html/
> nsHtml5TreeBuilderCppSupplement.h#138 . It runs on the parser thread.
> 

In the mean time I figured that, but I need to add a new nsHtml5AttributeName::ATTR_AS for 'as' attribute. I found some files that  are automatically generated. Can you please explain me what I need to do.


> You'd need to add code there to check for <link rel=preload as=foo> and
> append items to mSpeculativeLoadQueue accordingly.
> 
> When the queue is consumed on the main thread, the methods starting at
> https://searchfox.org/mozilla-central/source/parser/html/
> nsHtml5TreeOpExecutor.cpp#949 get called. I take it that rel=preload as=foo
> doesn't add new semantics to those, right?

I will fix this by adding <link rel=preload> to https://searchfox.org/mozilla-central/source/parser/html/nsHtml5TreeBuilderCppSupplement.h#138, without calling ScriptPreload. This will just start rel=preload early enough.

I am not sure if <link rel=preload> gives you all information needed in ScriptPreload. For example 'bool aScriptFromHead' <link rel=preload> can be in <head> but actual <script> does not have too. Furthermore, I am not sure about bool aAsync and bool aDefer.
(In reply to Henri Sivonen (:hsivonen) from comment #6)
> Out of curiosity, do we have evidence that rel=preload as=foo is needed in
> Firefox, i.e. that the parser doesn't find the URLs on its own quickly
> enough? I wonder to what extent the feature is motivated by Blink and WebKit
> having a less comprehensive speculative parser than we do.

This is coming from Google. In some cases it is useless, like in the example described here, since we start script preload very early. Link can be a http header in which case preload will be triggered even before html parsing starts. There is also idea of sending 103 Early Hint response before real response, The early hint will contain Link headers. This is an IETF draft:   https://tools.ietf.org/html/draft-kazuho-early-hints-status-code-00
Priority: -- → P3
(In reply to Dragana Damjanovic [:dragana] from comment #7)
> In the mean time I figured that, but I need to add a new
> nsHtml5AttributeName::ATTR_AS for 'as' attribute. I found some files that 
> are automatically generated. Can you please explain me what I need to do.

The doc is at: https://searchfox.org/mozilla-central/source/parser/html/java/README.txt#45
Taking this, as Dragana is on vacation, and we'd like to get it landed soonish.
Assignee: nobody → hurley
can you please take into account bug 1394778, since it is almost done.
Automated update from the htmlparser hg repo.
Actual work to hook up the HTML parser.

Henri - could you take a look to see if this is sensible? I pretty much just copy/pasted things from the elements we already handled prefetching for and applied those to the various "as" types.
Attachment #8907322 - Flags: feedback?(hsivonen)
Comment on attachment 8907322 [details] [diff] [review]
part 2 - Hook link rel=preload to HTML parser

Review of attachment 8907322 [details] [diff] [review]:
-----------------------------------------------------------------

Mostly OK but marking feedback- to draw attention to the comments.

::: parser/html/nsHtml5TreeBuilderCppSupplement.h
@@ +229,5 @@
>                    aAttributes->getValue(nsHtml5AttributeName::ATTR_CROSSORIGIN);
>                  mSpeculativeLoadQueue.AppendElement()->InitPreconnect(
>                    url, crossOrigin);
>                }
> +            } else if (rel.LowerCaseEqualsASCII("preload")) {

It would be appropriate to iterate over the value skipping whitespace and potential other tokens. Some of the other preloads omit that complexity, because in the presence of whitespace the non-speculative code will cause a (non-speculative) load later anyway, but this feature is speculative-only, so this is the only chance to get things right.

@@ +245,5 @@
> +                    aAttributes->getValue(nsHtml5AttributeName::ATTR_CROSSORIGIN);
> +                  nsHtml5String integrity =
> +                    aAttributes->getValue(nsHtml5AttributeName::ATTR_INTEGRITY);
> +                  bool async =
> +                    aAttributes->contains(nsHtml5AttributeName::ATTR_ASYNC);

Is there a spec for this stuff? Does the spec add the async and defer attributes on <link>? If so, why? For <script> elements, we pass async and defer along from the parser so that they can be deprioritized, but it seems weird to declare a preload as <link> and immediately deprioritize it.

@@ +271,5 @@
> +                  mSpeculativeLoadQueue.AppendElement()->InitStyle(
> +                    url, charset, crossOrigin, referrerPolicy, integrity);
> +                } else if (as.LowerCaseEqualsASCII("video")) {
> +                  mSpeculativeLoadQueue.AppendElement()->InitImage(
> +                    url, nullptr, nullptr, nullptr, nullptr);

Our preloading code for <video> only handles the poster attribute, so it's the same as an image preload. What does "video" mean for rel=preload? If it means loading the actual video file, this code is wrong. If it means loading the poster image, the spec is bad, because then "video" would be redundant with "image".
Attachment #8907322 - Flags: feedback?(hsivonen) → feedback-
(In reply to Henri Sivonen (:hsivonen) from comment #14)
> Comment on attachment 8907322 [details] [diff] [review]
> part 2 - Hook link rel=preload to HTML parser
> 
> Review of attachment 8907322 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Mostly OK but marking feedback- to draw attention to the comments.
> 
> ::: parser/html/nsHtml5TreeBuilderCppSupplement.h
> @@ +229,5 @@
> >                    aAttributes->getValue(nsHtml5AttributeName::ATTR_CROSSORIGIN);
> >                  mSpeculativeLoadQueue.AppendElement()->InitPreconnect(
> >                    url, crossOrigin);
> >                }
> > +            } else if (rel.LowerCaseEqualsASCII("preload")) {
> 
> It would be appropriate to iterate over the value skipping whitespace and
> potential other tokens. Some of the other preloads omit that complexity,
> because in the presence of whitespace the non-speculative code will cause a
> (non-speculative) load later anyway, but this feature is speculative-only,
> so this is the only chance to get things right.

To be clear... do you mean the value of the rel attribute, or the value of the as attribute?

> @@ +245,5 @@
> > +                    aAttributes->getValue(nsHtml5AttributeName::ATTR_CROSSORIGIN);
> > +                  nsHtml5String integrity =
> > +                    aAttributes->getValue(nsHtml5AttributeName::ATTR_INTEGRITY);
> > +                  bool async =
> > +                    aAttributes->contains(nsHtml5AttributeName::ATTR_ASYNC);
> 
> Is there a spec for this stuff? Does the spec add the async and defer
> attributes on <link>? If so, why? For <script> elements, we pass async and
> defer along from the parser so that they can be deprioritized, but it seems
> weird to declare a preload as <link> and immediately deprioritize it.

Spec (such as it is right now) is at https://w3c.github.io/preload/. The only mention of async & defer I see in the spec (on second look) seems to indicate that those attributes aren't available on <link>, so I'll get rid of those.

> @@ +271,5 @@
> > +                  mSpeculativeLoadQueue.AppendElement()->InitStyle(
> > +                    url, charset, crossOrigin, referrerPolicy, integrity);
> > +                } else if (as.LowerCaseEqualsASCII("video")) {
> > +                  mSpeculativeLoadQueue.AppendElement()->InitImage(
> > +                    url, nullptr, nullptr, nullptr, nullptr);
> 
> Our preloading code for <video> only handles the poster attribute, so it's
> the same as an image preload. What does "video" mean for rel=preload? If it
> means loading the actual video file, this code is wrong. If it means loading
> the poster image, the spec is bad, because then "video" would be redundant
> with "image".

It's not explicitly specified, but I imagine it means loading the actual video file. If we don't have existing speculative loading for <video> beyond the poster image, I'll go ahead and leave that out for now (like I have for other tags we don't currently have speculative loading for at the moment).
Flags: needinfo?(hsivonen)
(In reply to Nicholas Hurley [:nwgh][:hurley] (also hurley@todesschaf.org) from comment #15)
> > > +            } else if (rel.LowerCaseEqualsASCII("preload")) {
> > 
> > It would be appropriate to iterate over the value skipping whitespace and
> > potential other tokens. Some of the other preloads omit that complexity,
> > because in the presence of whitespace the non-speculative code will cause a
> > (non-speculative) load later anyway, but this feature is speculative-only,
> > so this is the only chance to get things right.
> 
> To be clear... do you mean the value of the rel attribute, or the value of
> the as attribute?

The value of the rel attribute. That is rel="   foo   preload   bar  " should be treated as a preload.

> If we don't have existing speculative loading for <video> beyond the poster image,

We don't.

> I'll go ahead and leave that out for now (like I have for other tags we don't currently have speculative loading for at the moment).

OK.
Flags: needinfo?(hsivonen)
So this is fun. I have a patch that compiles, and (locally) seems to run fine. But it fails on try (seems like it's either not loading the resource on try, or the load is not causing resource timing to be updated... but only on try). The other fun part is, when I try this on a one-click loaner (since it works fine locally), it fails in a totally different fashion (doesn't even appear to run the checks needed, according to the error message I'm getting).

So yeah, this hasn't fallen off my radar, but it's got me rather stumped at the moment.
Assignee: hurley → nobody
Component: DOM → DOM: Core & HTML
Blocks: rel=preload
No longer blocks: rel=preload
Blocks: rel=preload
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: