305462 - "Clear Cache Now" doesn't clear bfcache

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Jesse Ruderman]

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b4) Gecko/20050821
Firefox/1.0+ ID:2005082116

"Clear Cache Now" doesn't clear bfcache.  I think it should, since bfcache
usually holds at least as much private information as the normal cache for the
same page.

Comment 1

[Ria Klaassen (not reading all bugmail)]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050821
Firefox/1.0+ ID:2005082121

I see this too.

Comment 2

[Jesse Ruderman]

Attachment: testcase

Comment 3

[Chris Beard]

bryner, could you take a look?  thanks

Comment 4

[Brian Ryner (not reading)]

This is going to be easier once all of the session histories can be enumerated,
which is going to be part of bug 292965.  Deferring this until that one is fixed.

Comment 5

[Brian Ryner (not reading)]

Attachment: patch

Comment 6

[Darin Fisher]

What about nsICacheSession::evictEntries?  It seems like this observer event is
only generated by a call to nsICacheService::evictEntries, which takes a
parameter to control the storage type to evict.  In other words, this API does
not map directly to the button in the preferences panel.  Perhaps it would be
better to generate the observer event from the prefs panel.  It would almost
make sense for the cache service to observe that event instead (but I might hold
off on that change for now).

Comment 7

[Brian Ryner (not reading)]

(In reply to comment #6)
> not map directly to the button in the preferences panel.  Perhaps it would be

browser/base/content/sanitize.js:

        cacheService.evictEntries(ci.nsICache.STORE_ON_DISK);
        cacheService.evictEntries(ci.nsICache.STORE_IN_MEMORY);

So it will get invoked twice, true... I don't really know a good way to avoid 
that though, since the content viewer caching doesn't correspond entirely to 
either memory or disk cache (it's an in-memory cache, to be sure, but should 
also go away if the disk cache is cleared, since it contains similar 
information).

> better to generate the observer event from the prefs panel.  It would almost

I specifically wanted to avoid that, because it would require changes to any 
Gecko client that has a "clear cache" option.  The back forward cache is a 
detail that Gecko clients shouldn't need to worry about.

Comment 8

[Brian Ryner (not reading)]

Attachment: patch

This patch is more inclusive about sending the notifications.  I changed the
call sites in browser and xpfe to avoid notifying more than once, however, a
client that does flush memory and disk separately will still empty the bfcache
as designed.

Comment 9

[Marria Nazif]

This looks good to me, except i would put some big disclaimer comment by
the EvictAllContentViewers function explaining how inefficient it is.
I marked as approved so you can go ahead and submit after adding the comment.

Comment 10

[Darin Fisher]

One more concern:  I believe that the nsICacheService is an API designed to be
called on any thread (hence mCacheServiceLock).  nsIObserverService is not
threadsafe, however.  So, if someone decided to trigger cache eviction on a
background thread, then suddenly lot's of other stuff would run on that thread.
 I think this means that you need to use NS_GetProxyForObject.  See
nsCategoryManager, which has to deal with exactly the same problem.

Comment 11

[Brian Ryner (not reading)]

Attachment: with comments addressed

Comment 12

[Darin Fisher]

Comment on attachment 197804 [details] [diff] [review]
with comments addressed

s/objcets/objects/

please add a comment in nsICacheService.idl detailing the observer topic. 
someone should not have to look at the source code to see what this thing
means.	for example, you might want to follow the documentation found at the
bottom of nsIHttpProtocolHandler.idl.

otherwise, patch looks good to me.  sr=darin with those nits picked.

Comment 13

[Brian Ryner (not reading)]

checked in.

Comment 14

[Brian Ryner (not reading)]

Attachment: branch patch

Comment 15

[Brian Ryner (not reading)]

Checked in on the branch