1409210 - Don't let predictor prefetch resources whose responses have a Vary header set

Status: RESOLVED FIXED

(Core :: Networking, enhancement, P2)

Description

[u408661]

This is the safest fix for the case where we prefetch a resource, but then the version that we've prefetched would vary based on the headers we create for the actual request (per nsHttpChannel::ResponseWouldVary).

Comment 1

[Honza Bambas (:mayhemer)]

Should we have some kind of a whitelist for some headers contained in Vary?  I can very often see Vary: Accept-Encoding, which I think we could ignore (let predictor prefetch).  There might be others to add to this whitelist.

Also as you know, Vary is checked before forced validity, so technically this is only an optimization, not a fault fix.

Comment 2

[u408661]

No, I don't believe we should (unless we have a whitelist I missed in nsHttpChannel::ResponseWouldVary).

And this is, indeed, a fault fix. Consider the case where the predictor prefetches something, and the response comes back with Vary: foo (for some legitimate value of foo). When the channel goes to get the actual resource, and notices that it has a foo header different than what's in the cache for the prefetched version (which is totally possible, if not necessarily likely, especially for something like accept-encoding), it will refetch that version.

Furthermore, consider the case where the server keeps track of all requests for a resource (for example, ad networks keeping track of impressions to charge customers appropriately). (Yes, since this somehow modifies server-side state, one could argue that it shouldn't be a GET, but that ship has sailed.)

Combine those two cases, and you have a nasty doubling of requests to ad networks from Firefox, and I get another nasty-gram in my email :)

Comment 3

[u408661]

Attachment: Bug 1409210 - Don't prefetch resources with a Vary header.

Conceivably, we could allow a few more prefetches than this would (based
on the headers in the original request matching up to a header listed in
the Vary response header), but this is safer in case (for example)
future requests of this resource end up sending a cookie that wasn't set
on the original request. In practice, the difference is likely to be
small enough that this broader stroke won't make a huge impact on the
number of things we do or don't prefetch.

Review commit: https://reviewboard.mozilla.org/r/190288/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/190288/

Comment 4

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8919454 [details]
Bug 1409210 - Don't prefetch resources with a Vary header.

https://reviewboard.mozilla.org/r/190288/#review195828

Looks good. Thanks!

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/326eb2eebb66
Don't prefetch resources with a Vary header. r=valentin

Comment 6

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/326eb2eebb66