HTTP cache partitioning #943
Conversation
|
@domenic , @annevk: |
|
friendly ping. |
|
Sorry for the slow movement here as I was OOO. |
|
Addressed feedback, made triple keying and agent cluster key the default based on discussion with Firefox. Also added httpCache to http-network-fetch section. PTAL, thanks! |
There was a problem hiding this comment.
This looks great to me editorially, and to the extent of my understanding of the fetch algorithms. We again will need @annevk for final signoff, both because he's the editor, and because I am not 100% confident in my domain knowledge here for checking the semantics.
But yeah, @annevk, I think this and the dependent whatwg/html#4966 are now in pretty solid shape, and your review would be appreciated.
|
/cc @jkarlin Thanks Domenic! |
|
This looks really good to me, very nice work! We'll have to do some additional abstraction as we deploy this key across more features, but it's almost self-evident how that would be done. The main thing I'm worried about here is the concern @domenic had as well, that it's not entirely clear where we will end up. shivanigithub/http-cache-partitioning#2 and privacycg/proposals#4 go into this. I wonder if the initial take could be a bit more hand-wavy about the second key (the top-level one seems like a given at this point) and point to an outstanding issue that captures the debate. |
|
Addressed the concern about triple keying and added a note pointing to the issue. |
This will provide the foundation for whatwg/fetch#943 and other related changes discussed in whatwg/fetch#904.
It's useful for whatwg/fetch#943 to be able to distinguish between a site and agent cluster key as #4734 changes how agent clusters can be keyed.
|
I addressed a bunch of nits and created an HTML PR draft for the remaining integration issue. I'm going to be out for two weeks and hopefully in that time there will be some more progress on the keying debates linked in #943 (comment) as I'm not entirely comfortable merging this as-is. Perhaps we should only list the top-level origin's site as key and state that additional keys may be added or some such for now. (The HTTP cache is already optional so that doesn't seem too bad.) @shivanigithub have you looked at the impact on web-platform-tests btw? Should we have some coverage for this? Are tests impacted? |
|
We do have web platform tests for this. Here's the chromium CL: https://chromium-review.googlesource.com/c/chromium/src/+/1890952 |
It's useful for whatwg/fetch#943 to be able to distinguish between a site and agent cluster key as #4734 changes how agent clusters can be keyed.
It's useful for whatwg/fetch#943 to be able to distinguish between a site and agent cluster key as #4734 changes how agent clusters can be keyed.
|
Removed the definition of originAgentClusterKey and made the second key a bit more vague. |
|
Thanks @shivanigithub. I updated HTML to define "site" and "obtain a site" and we should be able to start using those tomorrow (takes a while before exported terms are indeed exported). So I expect we can merge this then or the day after. If anyone needs more time this would be a good time to say so. |
|
Do we want web platform tests? |
|
Web platform tests exist here |
|
Thanks! That seems to only cover the popup case, right? Not iframes? |
An environment's top-level origin is null during the initial navigation (before the response arrived) and otherwise represents the origin of the top-level document. It is currently implementation-defined for non-dedicated workers, but hopefully that can be sorted soon. An environment's top-level creation URL is the URL of the top-level document. It is null for workers as they do not need the concept. Needed for whatwg/fetch#943 and whatwg#5558.
|
I squashed all existing commits and rebased on master. I also reworded the initial commit message to include some more information. In a subsequent commit I will attempt to account for the new infrastructure from whatwg/html#5491 as discussed at whatwg/html#5558. |
|
This still fails to build because the HTML side hasn't landed yet. @shivanigithub could you take a look? |
An environment's top-level origin is null during the initial top-level navigation (before the response arrives) and otherwise represents the origin of the top-level document. It is currently implementation-defined for non-dedicated workers, but hopefully that can be sorted soon. An environment's top-level creation URL is the URL of the top-level document. It is null for workers as they do not need the concept. Needed for whatwg/fetch#943 and #5558. Co-authored-by: Anne van Kesteren <annevk@annevk.nl>
|
@shivanigithub @domenic was asking for additional test coverage above, do you know if that has been sorted since then? Is there a PR to remove |
|
No updates on the web platform tests as of now. |
annevk
added
the
needs tests
|
https://bugs.chromium.org/p/chromium/issues/detail?id=1064765 is the bug for tracking the new test to be added. |
|
They normally do, we try to be pretty strict about tests as they often uncover issues that might warrant further specification changes. See https://whatwg.org/working-mode#changes. I guess if @domenic is okay with it I'm happy with going ahead here, but we should at least have a PR for removing |
|
Iframe test added: and .tentative removed: Thanks @alexmturner! |
|
Great, does Safari pass all those tests? |
|
Safari's passing the cross-origin tests, but isn't passing the same-origin ones (i.e. it appears that the cache is not shared between the two same-origin windows). Chrome is not passing the cross-origin tests as the split cache is not yet on by default. Full results: https://wpt.fyi/results/fetch/http-cache/split-cache.tentative.html |
|
That's very strange. @youennf thoughts? @alexmturner thanks! As for the tests, from a quick skim they look good, but the naming could use a bit of work to instead talk about top-level site and also cross-site rather than cross-origin. One thing that might be worth doing is to cover a bunch more MIME types to ensure that any kind of specialized caches are also covered. @bakulf pointed me to https://searchfox.org/mozilla-central/source/toolkit/components/antitracking/test/browser/browser_staticPartition_cache.js#158-183 which unfortunately were not written as web-platform-tests. (I don't think that should block this PR though.) |
| <a for="environment">top-level creation URL</a>'s <a for=url>origin</a>. | ||
|
|
||
| <p class=note>This happens for top-level navigations only. | ||
| </ol> |
There was a problem hiding this comment.
Can you remind me why we're doing this here, instead of having HTML ensure top-level origin is always non-null by setting it to the origin of the top-level creation URL?
There was a problem hiding this comment.
My thinking was that a null -> origin transition would be clearer than an origin -> origin transition. And it would also make it clearer here that we compute the origin directly from a URL rather than the response.
There was a problem hiding this comment.
Hmm, that feels like it kind of defeats the point of environment/ESO being an abstraction that other specs don't have to worth about the details of, but can just use and let HTML handle the management. But I don't feel too strongly.
There was a problem hiding this comment.
I think generally that's true and will remain true, but there's a couple places that will need to care about the distinction and networking is one of those. (Other places generally only get access to an environment settings object.)
|
Small update re Safari and the WPTs. I believe the WPT failure is likely related to Safari failing the first test in https://wpt.fyi/results/fetch/http-cache/freshness.html ("HTTP cache reuses a response with a future Expires"). Strangely, when I test locally (on wpt.live), Safari passes all freshness and split-cache tests. I'm not sure what the cause of the disparity is. I'll update the naming to reflect the switch from origin to site and have a look at extending the tests for different MIME types soon. |
|
The terminology in tests was updated via web-platform-tests/wpt#24173 @annevk : Can the "needs tests" label be now removed? Is there anything else blocking the merge of this PR? |
|
Is there a PR for renaming the tests? That is, to drop .tentative? |
|
Yes, it's here: web-platform-tests/wpt#24070 |
annevk
added
topic: http
and removed
needs tests
|
Thanks for making this happen @shivanigithub! This is a pretty big step for https://privacycg.github.io/storage-partitioning/ thanks to your efforts over the past ten months. We now have all the infrastructure in place for partitioning network state. 🎉 |
Adds a section to define the HTTP cache associated with a request as per HTTP cache partitioning.
github issue: #904
Explainer: https://github.com/shivanigithub/http-cache-partitioning/
Preview | Diff