Skip to content
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

Should the list of available images key on referrer policy too? #5541

Closed
domfarolino opened this issue May 13, 2020 · 19 comments
Closed

Should the list of available images key on referrer policy too? #5541

domfarolino opened this issue May 13, 2020 · 19 comments
Assignees

Comments

@domfarolino
Copy link
Member

Right now the list of available images keys on the tuple (url, CORS settings attribute, maybe(Origin)). I think since referrerpolicy has been added to the list of relevant mutations (#5434), that the list of available images should also key on it.

We're accidentally already testing this as well, in an img lazyload test. https://github.com/web-platform-tests/wpt/blob/master/html/semantics/embedded-content/the-img-element/image-loading-lazy-referrerpolicy-change.sub.html shows that we're asserting that a deferred image is fetched with the correct referrer policy when its load is resumed, as opposed to being pulled from the list of available images / memory cache.

@annevk @zcorpan WDYT? If this seems reasonable I'll make a PR

@domfarolino domfarolino self-assigned this May 13, 2020
@annevk
Copy link
Member

annevk commented May 14, 2020

I'll defer to @emilio.

In general we need to clean up this cache because of https://privacycg.github.io/storage-partitioning/.

@domfarolino
Copy link
Member Author

Since the list of available images is a per-document concept, does that alone solve the partitioning problems? It seems like document A cannot reuse an entry in document B's list of available images, so I'm not sure if this list falls under the scope of what the Privacy CG link discusses:

User agent state that is keyed by a single origin or site

@annevk
Copy link
Member

annevk commented May 14, 2020

User agents may copy entries from one Document object's list of available images to another at any time (e.g. when the Document is created, user agents can add to it all the images that are loaded in other Documents), but must not change the keys of entries copied in this way when doing so, and must unset the ignore higher-layer caching flag for the copied entry.

@domfarolino
Copy link
Member Author

Thanks, didn't see that :( Well in the mean time, I think adding referrerpolicy as a key makes sense, let me know what you think @emilio

@emilio
Copy link
Contributor

emilio commented May 14, 2020

Yeah, I think this makes sense. What do implementations do though?

@annevk
Copy link
Member

annevk commented Jun 3, 2020

I think I would prefer not taking referrer policy into account after thinking about this some more, FWIW. See also https://bugzilla.mozilla.org/show_bug.cgi?id=1174921.

@domfarolino
Copy link
Member Author

I read the bugzilla thread, but even after doing so I don't understand why we wouldn't treat referrerPolicy and crossOrigin attribute mutations differently, in terms of memory cache matching.

Is it just because we think sites won't vary the image response based on the referrer? Regarding one of the comments in that thread:

Reloading in those cases creates unnecessary traffic. If the response actually depends on the referer header "Vary: Referer" might be used.

Whose to say what unnecessary traffic is? We're doing the same for crossOrigin mutations, so I don't get why we wouldn't treat them equally.

@annevk
Copy link
Member

annevk commented Jun 4, 2020

Mutations? This is only whether it's part of the cache key, right? And yeah, it's mostly that it seems more likely that the server would differentiate based on credentials than on referrer policy. Note that even if you include referrer policy, if the full referrer is send the server still couldn't meaningfully vary on referrer as the referrer isn't part of the cache key, just the referrer policy.

@domfarolino
Copy link
Member Author

Mutations?

  1. Fetch an <img>. The request has the default crossorigin attr value, and referrerpolicy value
  2. The load finishes and is added to the list of available images, which is keyed on the crossorigin attribute (among other things)
  3. Later if you mutate the image setting img.referrerPolicy = differentPolicy, a new request is made, it matches an entry in the list of available images, and no network request is sent
  4. But if you did a similar mutation like img.crossOrigin = differentState, a new request is made, does not match an entry in the list of available images, the request will go to the network

I guess it just feels weird that referrerPolicy and crossOrigin mutations are both "relevant", but on is caught by the list of available images cache. I guess if those are separate-enough concepts it doesn't matter too much to me, but just feels a little weird

@annevk
Copy link
Member

annevk commented Jun 4, 2020

It seems a similar thing applies to the width and sizes attributes, or setting any attribute to the same value (although maybe that is not considered changed, despite DOM considering it changes, that's ambiguous and should be fixed).

@domfarolino
Copy link
Member Author

It seems a similar thing applies to the width and sizes attributes

The main difference is what @zcorpan said:

This makes sense to me since it changes the request.

...where as width and height aren't changing the request. But if you feel strongly about this (at least more than I do, which is not very strong) feel free to close this issue.

@annevk
Copy link
Member

annevk commented Jun 4, 2020

And by request you mean the request concept? But we're not using that as cache key as I pointed out (e.g., a different referrer doesn't matter, just a different referrer policy) so it still seems somewhat arbitrary to me.

A larger concern here is Chrome/WebKit's undefined memory cache which Gecko has for some things as well (e.g., style sheets). We really need to define better where it sits in the pipeline (ideally in Fetch I'd think), how things are keyed, and what kind of types are supported. And then when it comes to keying we would make decisions across all those types, rather than just for images or style sheets or some such.

@domfarolino
Copy link
Member Author

And by request you mean the request concept?

Yep. Anyways I agree with you on the fact that memory cache needs to be defined somewhere. Are there any open issues about this I can subscribe to? Regardless, this issue doesn't seem to be going anywhere, so I'll close it but if anyone feels strongly about this feel free to comment or re-open.

@annevk
Copy link
Member

annevk commented Jun 4, 2020

whatwg/fetch#590 is a good starting point. I think preload/list of available images/style sheets/etc. all should end up with the same kind of logic. The undefined scanner of the HTML parser should probably end up there too.

@domfarolino
Copy link
Member Author

And yeah, it's mostly that it seems more likely that the server would differentiate based on credentials than on referrer policy

It seems a similar thing applies to the width and sizes attributes, or setting any attribute to the same value (although maybe that is not considered changed, despite DOM considering it changes, that's ambiguous and should be fixed).

Circling back to these after I thought about it some more. Why is referrerpolicy in the list of relevant mutations? My thought was we were OK adding it because it affects the request in a way that the server can observe. If we didn't think think that this was important enough, should we not have added that attribute to the relevant mutations?

By acknowledging that this impacts the request enough such that it makes sense to be a relevant mutation mutation, it seems inconsistent to assume that the server would not respond differently such that we can always serve it from the cache.

@annevk
Copy link
Member

annevk commented Jul 22, 2020

@domfarolino I wouldn't oppose changing that, but let's discuss that in a new issue (and copy Emilio)?

@domfarolino
Copy link
Member Author

It would have maybe been a little better to discuss in #5244, but I realize you're not expected to see very single issue lol. I just figured Firefox would be OK with this change, because Firefox already treated referrerpolicy mutations as "relevant" for a while, IIRC

@annevk
Copy link
Member

annevk commented Jul 23, 2020

I'm not sure, but I might have seen that and decided it was okay since Firefox has partial support. But also, since then I have developed a better understanding of the trade-offs involved.

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

No branches or pull requests

4 participants