Closed Bug 1198792 Opened 5 years ago Closed 4 years ago

Appcache manifest becomes indefinitely cached after a "clear cache"


(Core :: Networking: Cache, defect)

40 Branch
Not set



Tracking Status
firefox44 --- fixed


(Reporter: dom, Assigned: mayhemer)



(1 file)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/44.0.2403.157 Safari/537.36

Steps to reproduce:

1) Visit site using appcache
2) Check about:cache?storage=appcache&context=, manifest.appcache has expiration
3) History -> Clear recent history (all but site pref. checked, time range: everything) -> Clear Now
4) Visit site again
5) Check about:cache?storage=appcache&context=, manifest.appcache now has NO expiration

Issue can be easily replicated with this simple node setup.

Actual results:

manifest.appcache is stored after clearing cache the first time, without any expiration date. Hence locking up the website in question eternally at that version.

Expected results:

manifest.appcache on the second visit should have the same expiration as on the initial visit.
For peace of mind, same problem holds true on some commercial websites.

Follow the above steps, same problem.
Andrea, do you know who can look appcache issues?
Flags: needinfo?(amarchesini)
Not really... jdm maybe? Better to ask Andrews.
Flags: needinfo?(amarchesini) → needinfo?(overholt)
Honza is the appcache expert.
Flags: needinfo?(overholt) → needinfo?(honzab.moz)
I'll try to take a look but I'm pretty busy right now.
Assignee: nobody → honzab.moz
Flags: needinfo?(honzab.moz)
Hey sorry guys, any progress on this one? Or an estimate as to when this may get looked at?
Honza or Andrew, anyone with time to look at this?

It impacts anyone who ever visited a site that uses appcache and has cleared their history.

It breaks the Web, since the site is forever stuck at that last appcache manifest version.
Flags: needinfo?(overholt)
Generally speaking, the appcache spec is a mess and we discourage people from using it.  It may be deprecated or even removed at some point soon (the spec group for the appcache has disbanded, for instance).

But I agree this is a particularly ugly bug, so we'll try to fix it soon.  But Honza is the only person who knows appcache well so it will depend on his timeframe. I'm needinfo'ing him so it stays on his radar.

> manifest.appcache now has NO expiration

I'd be curious to know whether this means we never revalidate/check for a new manifest, or if we just fall back to the default RFC 2616 section 13..42 heuristic (using Last-Modified, assuming that's still present in the cache entry), which would be at least a little less bad.
Flags: needinfo?(overholt) → needinfo?(honzab.moz)
There are few weird things that hides this bug during normal usage and cause this behavior after clear all.

App cache keeps a hashtable of active caches.  That is not cleared by the clear-history dialog (a bug).

An nsHttpChannel writing to an appcache is always having two entries: a cache entry to read from and a cache entry to write to.  The read entry is supposed to be an entry from any currently existing appcache version (if there is any) to ensure common http caching semantics when updating the appcache.

When an nsHttpChannel, in a fresh session of the browser, is told to write an offline cache entry for a currently non-existing appcache, it opens for 'read' an entry in the normal HTTP cache and gets a new, empty one (another bug).

Hence, when we then clear all with the clear-history dialog, cached content is gone, but the hashtable is not cleared.  So, another instance of an nsHttpChannel, that writes an appcache again, tries to open the 'read' cache entry again from an appcache.  The appcache code finds an active appcache in the (uncleared) hashtable and tries to load the entry right from that appcache (which actually no longer exists).  Hence, it fails.  The channel ends up with no 'read' entry.

Only two places we correctly set the expiration time on the 'written' offline cache entry are nsHttpChannel::ContinueProcessNormal->InitCacheEntry() and CallOnStartRequest->InitOfflineCacheEntry.  But both are conditioned by presence of the 'read' entry.

So, we have a bunch of bugs here:
- the 'read' entry should open with OPEN_READONLY flag (a larger impact change I'm afraid)
- the appcache in-memory hashtable should clear when we clear the appcache from UI
- we should update expiration on offline cache entries begin written regardless we have the cache entry to read from (a fix that will fix this bug)

Easiest thing to do seems to me to call UpdateExpirationTime() when there is no mCacheEntry in nsHttpChannel::ContinueProcessNormal()
Ever confirmed: true
Flags: needinfo?(honzab.moz)
Attached patch v1Splinter Review
I rather came up with a simpler solution.  The main bug is we don't clean up the hashtables.  When we do, the system as a whole behaves correctly (despite it's thanks other bugs.)  I don't want to touch nsHttpChannel when appcache is about to be removed anyway.
Attachment #8674334 - Flags: review?(jduell.mcbugs)
Comment on attachment 8674334 [details] [diff] [review]

Review of attachment 8674334 [details] [diff] [review]:

Honza, do you think we should ask for uplift to aurora/beta?
Attachment #8674334 - Flags: review?(jduell.mcbugs) → review+
Keywords: checkin-needed
(In reply to Jason Duell [:jduell] (needinfo? me) from comment #11)
> Comment on attachment 8674334 [details] [diff] [review]
> v1
> Review of attachment 8674334 [details] [diff] [review]:
> -----------------------------------------------------------------
> Honza, do you think we should ask for uplift to aurora/beta?

The patch is simple enough, I can ask for uplift after few days of m-c baking.
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.