Closed Bug 1206298 Opened 4 years ago Closed 4 years ago
Image returned by browser even though removed from SW cache
Part 1: Update the expiration time on the fake cache entry object for an intercepted request in the non-e10s case
6.35 KB, patch
|Details | Diff | Splinter Review|
9.65 KB, patch
|Details | Diff | Splinter Review|
I am adding an image to a SW cache that has a `Cache-Control: max-age=1`, so I expect it to expire. However, when I request that image again 5+ seconds later, the image still shows up. I am expecting the image to have been destroyed from the cache.
Skimming the SW spec, I don't see anything that deals specifically with headers for cached responses. https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#cache-lifetimes also suggests that this behaviour is per spec.
That the cache doesn't self-manage based on HTTP cache directives kinda sucks:( Will take that up with the spec people. However, I am explicitly removing the image from the cache before attempting to reload it. So, I would expect trying to load the image to fail (I know the image is actually in the image cache, not HTTP cache - just looking it at it from author expectation). Wondering what others think should happen here?
Should this be blocking v1?
I'm going to say this shouldn't block our shipping of SW and that if the spec needs to change we can fix it when it does (and in conjunction with other UAs shipping SW at that time). Marcos, obviously feel free to disagree :)
I think comment 1 is missing the fact that Marcos was talking about setting the Cache-Control max-age header. Josh, can you please confirm? Looking at the imagelib code, we do get the max age of the cache entry: <http://mxr.mozilla.org/mozilla-central/source/image/imgRequest.cpp#597>. That timestamp seems to be initialized in nsHttpChannel::UpdateExpirationTime() which is called from nsHttpChannel::InitCacheEntry(), but as far as I can tell, we bypass calling that function (which is normally called from ContinuteProcessNormal) in the case of cache entries for synthesized content. If my analysis is correct, I think this needs to block v1, otherwise the cache entries will have an inconsistent state and I am not sure what other things it will break.
Cache API does not look at http cache headers by design. You have to write the eviction code yourself. If you want http cache semantics, then just rely on the http cache. Implementing an auto-remove feature for Cache API is something that was proposed in v2 of Anne's storage spec. Specifically, the cache boxes.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → INVALID
Ehsan pointed out in person that this is more complicated than it first appears. Specifically, the image cache (separate from the HTTP cache) checks the cache headers on the HTTP response but isn't able to find them from our synthesized responses.
Sorry for my confusion. I missed comment 2.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Sorry, the todo_is() calls in the previous patch should have been is().
Attachment #8677700 - Flags: review?(josh)
Comment on attachment 8677698 [details] [diff] [review] Part 1: Update the expiration time on the fake cache entry object for an intercepted request in the non-e10s case Review of attachment 8677698 [details] [diff] [review]: ----------------------------------------------------------------- thanks!
Attachment #8677698 - Flags: review?(mcmanus) → review+
You need to log in before you can comment on or make changes to this bug.