1206298 - Image returned by browser even though removed from SW cache.

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Marcos Caceres [:marcosc]]

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.

Comment 1

[Josh Matthews [:jdm]]

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.

Comment 2

[Marcos Caceres [:marcosc]]

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?

Comment 3

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Should this be blocking v1?

Comment 4

[Andrew Overholt [:overholt]]

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 :)

Comment 5

[(no longer active)]

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.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 7

[Josh Matthews [:jdm]]

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.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Sorry for my confusion.  I missed comment 2.

Comment 9

[(no longer active)]

Attachment: Part 1: Update the expiration time on the fake cache entry object for an intercepted request in the non-e10s case

Comment 10

[(no longer active)]

Attachment: Part 2: Add a test to make sure that we respect Cache-Control headers on the synthesized response

Comment 11

[(no longer active)]

Attachment: Part 2: Add a test to make sure that we respect Cache-Control headers on the synthesized response

Sorry, the todo_is() calls in the previous patch should have been is().

Comment 12

[Patrick McManus [:mcmanus]]

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!

Comment 13

[(no longer active)]

Josh: ping?

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a5f81cb4dcd7
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6c7c4323490

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a5f81cb4dcd7
https://hg.mozilla.org/mozilla-central/rev/c6c7c4323490