1500089 - rel="prefetch" seems to not care which document has reached STATE_STOP to kick off prefetching

Status: NEW

(Core :: DOM: Networking, enhancement, P3)

Description

[Mike Conley (:mconley) (:⚙️)]

I ran into this when trying to land the patches from bug 1472212, which puts Activity Stream into its own content process. I was getting timeouts when running the fetch-destination.https.html tests - specifically, the last one which does prefetch.

Essentially, if I'm reading this correctly, nsPrefetchService::Prefetch relies upon a document reaching STATE_STOP[1] to kick off prefetching - but it doesn't need to be the document that has the <link> node. It can be _any_ document, since it's attaching to the process-global document loader service[2].

I'm running into this because for years now, we've been doing this perceived performance optimization where we open an invisible about:newtab in the background after a tab opens in order to ensure that the next time you open a new tab, it's ready to go immediately.

According to rr, fetch-destination.https.html has been taking advantage of this accidentally - there are SVG images being loaded in that invisible about:newtab, and when they load, the cause the document loader service to notice a STATE_STOP (because SVGs are documents). This kicks off prefetching.

With Activity Stream in its own content process, this STATE_STOP is never observed.

I suspect that it's accidental that the test, and rel="prefetch" in general is relying on the document loader service. If an unrelated document reaches STATE_STOP, it doesn't make sense to me that it'd maybe kick off prefetching in a separate, unrelated document (which might still be loading!).

So, two questions:

1. Should we be making rel="prefetch" rely on the STATE_STOP of the document that the <link> is in, instead of any document in the process?
2. Should we modify nsPrefetchService to immediately start preloading if the document the <link> is injected into has already finished loading?

[1]: https://searchfox.org/mozilla-central/rev/eef79962ba73f7759fd74da658f6e5ceae0fc730/uriloader/prefetch/nsPrefetchService.cpp#998-999
[2]: https://searchfox.org/mozilla-central/rev/eef79962ba73f7759fd74da658f6e5ceae0fc730/uriloader/prefetch/nsPrefetchService.cpp#547-554

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Hey dragana - smaug suggested you might have some thoughts on this, since you've hacked around in this area recently for the rel="preload" stuff.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

I suspect this sensitivity to other document loads is related to how the test has been failing intermittently (bug 1489438)

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Or ehsan, do you have any insight on my questions in comment 0?

Comment 4

[(no longer active)]

Wow, how have we not caught this issue in the past 17 years is beyond me.  Based on reading this code, it seems that prefetch has basically been broken ever since we've had tabbed browsing support (correct me if I'm wrong.)

Not sure if you have read the original bug that introduced the code in question, but if you read for example bug 12274 comment 26, I think it's clear that Darin was trying to implement the sane behavior that you would expect, basically "yes" to answer your first question.

What has changed since then, I believe, is that Gecko has grown up and learned how to handle multiple top-level documents being open at the same time (aka tabbed browsing!).  The code for the prefetch service seems to have been written with the assumption of a single document per process or something like that (note how the service pauses and resumes its work based on a single top-level page <https://searchfox.org/mozilla-central/rev/eef79962ba73f7759fd74da658f6e5ceae0fc730/uriloader/prefetch/nsPrefetchService.cpp#998-1001>).

The answer to question #2 is a lot harder to get to.  If you read the original bug, people back then were worried about "server load", but a lot has changed since then, I would posit that most sites do not go down because of browsers prefetching any more.  So I guess to answer #2 I would say "sure, that would be nice", but I will keep in mind the data that the original optimization has been based on (IOW, 17 year old anecdotes) so if doing something else here would your life easier I would certainly keep that option open.

Before doing anything here, I would take a look at how the other two open source engines handle prefetches, and think hard about why you'd want to do something different.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

The prefetch test has now been disabled in bug 1489438.

Comment 6

[Dragana Damjanovic [:dragana]]

There is a bug for this already (I cannot find it at the moment).