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

Add network.setCacheBehavior command #721

Merged
merged 10 commits into from
Jul 3, 2024
Merged

Add network.setCacheBehavior command #721

merged 10 commits into from
Jul 3, 2024

Conversation

jgraham
Copy link
Member

@jgraham jgraham commented May 29, 2024

This currently only allows setting the request cache mode to 'no-store' which bypasses the cache entirely. If we have future use cases for setting different cache modes, this appraoch would be extensible to those use cases.


Preview | Diff

@jgraham
Copy link
Member Author

jgraham commented May 29, 2024

Draft Fetch PR: whatwg/fetch#1756

@jgraham
Copy link
Member Author

jgraham commented May 29, 2024

One question here is whether this interacts with cache-mode specific headers in the expected way. In particular this will end up setting the headers so that not only do we not use the local cache, but we also don't populate any HTTP caches for this request.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Show resolved Hide resolved
Copy link
Contributor

@whimboo whimboo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks fine to me. Lets have @OrKoN or @Lightning00Blade to have another look.

@OrKoN
Copy link
Contributor

OrKoN commented Jun 3, 2024

@sadym-chromium would you mind reviewing?

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@juliandescottes
Copy link
Contributor

Had a quick look at how Firefox behaves when disabling cache from DevTools (which we intend to reuse for BiDi).

  • for CORS preflight requests, Firefox will also send the preflight requests every time
  • for the various image/stylesheet caches, Firefox will also bypass them and re-issue the requests
  • however for requests handled by a service worker, Firefox will not disable the cache used by the service worker

index.bs Outdated Show resolved Hide resolved
@jgraham
Copy link
Member Author

jgraham commented Jun 11, 2024

So I'm not really sure what to do with this.

It seems like talking in terms of the cache mode has made an interop difference visible: Firefox neither reads from nor writes to the cache, but Chrome does write to it. That would affect what happens if you do a no-cache request followed by a cached request.

However for the non-HTTP caches, working purely in terms of the cache mode on the request seems like it maybe doesn't work well? Per fetch it seems like requests use the CORS preflight cache irrespective of their cache-mode. So we could have a webdriver-specific rule that we should clear the preflight cache for a request if the override mode is, say, no-store or reload, but then the WebDriver API has different behaviour from the DOM API. Or we could have a separate flag to bypass the CORS cache (i.e. something like network.setCacheMode({cacheMode, contexts, bypassCORS})`, but that seems potentially unwieldy.

This also applies to all the possible in-memory caches, but even more so since the details may differ between browsers.

Or the other option is to just support the bare "bypass all caches" use case instead of specifying a specific cache mode i.e. make the command network.setCacheBypass({bypass, contexts}). That seems close to what CDP has, but it's rather inflexible if we want to offer more control (similar to existing DOM APIs) in the future.

@css-meeting-bot
Copy link
Member

The Browser Testing and Tools Working Group just discussed Bypass network cache behaviour.

The full IRC log of that discussion <jgraham> Topic: Bypass network cache behaviour
<jgraham> github: https://github.com//pull/721
<orkon> jgraham: this one is about request interception and how we force bypassing the network cache. I put a PR which was designed around the idea of setting the cache mode on requests. So when you have a request, it will set the cache mode to no-store, which means no reading or writing to the http cache. Unfortunately, it does not quite match what
<orkon> browsers do at the moment when you disable caches in devtools: 1) cors preflight cache which is always used 2) image cache and possible other caches So the question is how much flexibility do we care about here? Should we try work with low level primitvies? Or do we actually only care about bypassing all caches?
<orkon> jgraham: is that going to be the easier way to solve this?
<jgraham> q?
<orkon> q+
<jgraham> ack orkon
<jgraham> ScribeNick: jgraham
<jgraham> orkon: We also found that Firefox uses no-store by default but chrome uses reload, so Chrome still writes responses to the cache. We couldn't find use cases which require controlling different kinds of caches individually. Maybe there are some wpt use cases, but we haven't run into them yet. For testing scenarios you mostly care about bypassing everything. Also for interception to catch every request.
<jgraham> So it sounds like a single bypass cache setting would be sufficient. We could extend in the future with additional parameters if we needed. Question is which cache mode should the bypass use, or how important is that?\
<jgraham> q+
<orkon> ScribeNick: orkon
<jgraham> ack next
<orkon> jgraham: I think it is important that we specify a specific cache mode. bypassing the cache meaning no writing to cache makes more sense.
<orkon> jgraham: we empty the cors cache or whether we just bypass it. But then when you disconnect webdriver session, the cache will be still populated with the cache
<orkon> jgraham: I noticed that the spec has primitivees for clearing the cache but not by-passsing
<orkon> jgraham: and what happens with preflight requests if you toggle the bypass setting
<orkon> q+
<jgraham> q?
<jgraham> ack orkon
<orkon> ScribeNick: jgraham
<jdescottes> q+
<jgraham> orkon: I need to look at what we do for the CORS cache. We can change the cache mode; if we use reload we don't need to change but Firefox would. For memory caches we clear the cache. I need to double check on bypass vs clear. For HTTP we definitely don't clear, but I'm not sure about the other cache types
<jgraham> ack jdescottes
<jgraham> ScribeNick: orkon
<orkon> jdescottes: in Firefox we only bypass but do not actively clear anything, at least with DevTools
<orkon> jgraham: I think bypassing makes more sense than clearing. It sounds like at least for some of the caches it is hard to implement and memory caches can be cleared at random. So perhaps it is less imporant.
<orkon> q+
<jgraham> ack next
<jgraham> ScribeNick: jgraham
<jgraham> orkon: I think what happens to the cache after you've done your tests is less important. it's more important to be able to test a cold start of the application and measure the time taken, data transferred, etc.
<jgraham> ScribeNick: orkon
<orkon> q?
<orkon> jgraham: we should change the command to just set cache bypass to true or false and for now we should set the specific cache mode and it should probably bypass the cors cache and be a bit more handwavy about memory caches
<orkon> q+
<jgraham> ack next
<jgraham> ScribeNick: jgraham
<jgraham> orkon: I think there's an open HTML issue about the image cache, but implementations may also have more caches than that.
<orkon> ScribeNick: orkon
<orkon> jgraham: it might be that for image cache we could specify better but we could also mention implementation-specific caches
<jgraham> q?

jgraham and others added 3 commits June 24, 2024 14:47
This currently only allows setting the request cache mode to 'no-store' which bypasses
the cache entirely. If we have future use cases for setting different cache modes, this
appraoch would be extensible to those use cases.
Co-authored-by: Nikolay Vitkov <[email protected]>
This just takes a single true/false value for whether to bypass the cache.
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@whimboo whimboo changed the title Add network.setCacheMode command Add network.setCacheBypass command Jun 27, 2024
index.bs Outdated Show resolved Hide resolved
Copy link
Contributor

@OrKoN OrKoN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The summary of changes that are needed:

  1. The fetch spec PR needs an update to reflect latest changes.
  2. Remove the ability to set global cache bypass or define how it works for contexts created after the global override was set.
  3. @sadym-chromium's suggested change to use an enum instead of a boolean so that it could be extended later
  4. indentation issue pointed out by @lutien

Also correctly specify how to handle mixed per-navigable and global
settings, so that:

* Updating the global setting always overrides any per-navigable
settings.
* Per-navigable settings are only stored where they're different to
the global setting.

This should match the behavior of subscribing to events.
index.bs Outdated Show resolved Hide resolved
Co-authored-by: Alex Rudenko <[email protected]>
@whimboo whimboo changed the title Add network.setCacheBypass command Add network.setCacheBehavior command Jul 3, 2024
index.bs Outdated Show resolved Hide resolved

1. Set the [=default cache behavior=] to |behavior|.

1. [=map/Clear=] [=navigable cache behavior map=].
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: the global cache behavior erases all the specific browsing context's cache behavior previously set.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think that's the same as the way that enabling an event globally works; if you subscribe for a specific context and then subscribe for all contexts and then unsubscribe for all contexts you don't end up with the event subscription for the original set of contexts.

Co-authored-by: Henrik Skupin <[email protected]>
@jgraham jgraham requested a review from whimboo July 3, 2024 13:55
Copy link
Contributor

@whimboo whimboo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like something is wrong given that the preview fails to build. @jgraham can you please check?

index.bs Show resolved Hide resolved
@jgraham jgraham requested a review from whimboo July 3, 2024 20:17
Copy link
Contributor

@whimboo whimboo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks it looks fine to me now as well.

@whimboo
Copy link
Contributor

whimboo commented Jul 3, 2024

I'm going to update the existing tentative tests (which failed to merge upstream) via https://bugzilla.mozilla.org/show_bug.cgi?id=1906100.

@jgraham jgraham merged commit a46d71b into main Jul 3, 2024
5 checks passed
@jgraham jgraham deleted the cache_mode_override branch July 3, 2024 20:25
github-actions bot added a commit that referenced this pull request Jul 3, 2024
SHA: a46d71b
Reason: push, by jgraham

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants