Closed Bug 1206298 Opened 4 years ago Closed 4 years ago

Image returned by browser even though removed from SW cache.

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox43 --- affected
firefox44 --- fixed

People

(Reporter: marcosc, Assigned: ehsan)

References

Details

(Whiteboard: [see comment 5])

Attachments

(2 files, 1 obsolete file)

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?
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 :)
Blocks: ServiceWorkers-postv1
No longer blocks: ServiceWorkers-v1
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.
Flags: needinfo?(josh)
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
Flags: needinfo?(josh)
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 → ---
Whiteboard: [see comment 5]
Blocks: ServiceWorkers-v1
No longer blocks: ServiceWorkers-postv1
Sorry, the todo_is() calls in the previous patch should have been is().
Attachment #8677700 - Flags: review?(josh)
Attachment #8677699 - Attachment is obsolete: true
Attachment #8677699 - 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+
Assignee: nobody → ehsan
Josh: ping?
Attachment #8677700 - Flags: review?(josh) → review+
https://hg.mozilla.org/mozilla-central/rev/a5f81cb4dcd7
https://hg.mozilla.org/mozilla-central/rev/c6c7c4323490
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
See Also: → 1329506
You need to log in before you can comment on or make changes to this bug.