Closed Bug 1409210 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: Networking, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: u408661, Assigned: u408661)

References

(Blocks 1 open bug)

Details

(Whiteboard: [necko-triaged])

Attachments

(1 file)

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).
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.
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 :)
Attachment #8919454 - Flags: review?(valentin.gosu)
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!
Attachment #8919454 - Flags: review?(valentin.gosu) → review+
Pushed by hurley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/326eb2eebb66
Don't prefetch resources with a Vary header. r=valentin
https://hg.mozilla.org/mozilla-central/rev/326eb2eebb66
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.