Closed Bug 576621 Opened 10 years ago Closed 10 years ago

clearing cache does NOT clear cached images

Categories

(Core :: ImageLib, defect, major)

x86
Windows XP
defect
Not set
major

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: steve, Assigned: bholley)

References

Details

(Keywords: privacy, regression)

Attachments

(2 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.6) Gecko/20100625 Firefox/3.6.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.6) Gecko/20100625 Firefox/3.6.6

When a web developer clears the cache, they expect that any resources in the page will be refetched. Instead, Firefox will cache images and not re-download them even after the developer has cleared the cache. This is bad - developers who are focused on performance aren't see the true pain of loading their sites.

Reproducible: Always

Steps to Reproduce:
   1. start Firefox
   2. start a packet sniffer such as HttpWatch or Firebug Net Panel
   3. clear your cache
         a. type control-shift-delete
         b. choose "Everything" for time range to clear
         c. only check "Cache" under Details
         d. select "Clear Now"
   4. enter "http://www.google.com/" in the URL bar
   5. notice that http://www.google.com/intl/en_ALL/images/srpr/logo1w.png is downloaded
   6. go to some other URL such as "http://search.yahoo.com/"
   7. clear your cache (see previous steps)
   8. enter "http://www.google.com/" in the URL bar again
   9. notice that logo1w.png is NOT downloaded, and yet it appears in the page
  10. hold down the shift key and click on the Reload button
  11. notice that logo1w.png IS downloaded


Actual Results:  
Images (such as logo1w.png) are not re-downloaded even after the cache is cleared.

Expected Results:  
All images used in the page should be refetched after clearing cache.

The fact that logo1w.png DOES show up during shift+Reload shows that it IS required by the page, but isn't be refetched after clearing the cache.

This has broken sometime recently, perhaps since Firefox 3.4. 

Scripts are properly cleared from the cache and refetched.

I don't think this is BFCache. Honza has modified Firebug 1.6a16 to show BFCache fetches in Net Panel - but logo1w.png isn't showing up.

This happens on all sites.
Component: General → ImageLib
Product: Firefox → Core
QA Contact: general → imagelib
Probably a regression from imagelib no longer using a special necko cache device...

We likely need an API for clearing the image cache, plus front end use of said api.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Are you sure that the memory cache and disk cache are really cleared ? See about:cache

I know that you can't really clear the image cache (as they're still required to be shown when you scroll a page before you refresh it - just like the content), but they're supposed to be reloaded from the network again (and/or the memory and disk cache).
Status: NEW → UNCONFIRMED
Ever confirmed: false
(In reply to comment #1)
> We likely need an API for clearing the image cache, plus front end use of said
> api.

see bug 473054 and bug 516599 for pointers
> but they're supposed to be reloaded from the network again

Sort of.  It may or may not work, depending on the validator goop in imgLoader.
We've definitely got an API for it:
http://mxr.mozilla.org/mozilla-central/source/modules/libpr0n/public/imgICache.idl

However, it doesn't seem to be hooked up to anything. Where should the frontend hook go?
I am absolutely astonished that this isn't a duplicate!

This doesn't block, as it's from the rewrite of the image cache, and hence is pretty old (and not a regression).
blocking2.0: ? → -
(In reply to comment #6)
> I am absolutely astonished that this isn't a duplicate!

It sort of is a dupe of bug 516599, depending on how you interpret dolske's offhand comment of "we should probably do this too".
> This doesn't block, as it's from the rewrite of the image cache, and hence is
> pretty old (and not a regression).

Regression from 3.0, no?

> Where should the frontend hook go?

Probably right here:

https://hg.mozilla.org/mozilla-central/annotate/d2f3475002d4/browser/base/content/sanitize.js#l113
Status: UNCONFIRMED → NEW
Ever confirmed: true
Right, but not a _new_ regression.(In reply to comment #8)
> > This doesn't block, as it's from the rewrite of the image cache, and hence is
> > pretty old (and not a regression).
> 
> Regression from 3.0, no?

Right - not a new regression.
Ok, i guess it doesn't qualify as a regression then, but it does seem like this would be a pretty big problem for developers, no? Renominating based on that, though of course up to you if you feel that that should block.
blocking2.0: - → ?
Attached patch patch v1 (obsolete) — Splinter Review
Plus it seems pretty easy to fix. Here's a patch.

I haven't actually tested it yet because I'm doing a long rebuild. I'll report back in 20 minutes or so.
The private browsing part of that should have the clear in the try, not the catch.
Attached patch patch v2 (obsolete) — Splinter Review
> The private browsing part of that should have the clear in the try, not the
> catch.

Good catch. I also somehow managed to write the cacheclear twice in the sanitize part. That was probably the worst bug-to-length ratio ever.
Attachment #455773 - Attachment is obsolete: true
and clearCache() is called twice in sanitize.js
Attached patch patch v3Splinter Review
Got this working. Flagging joe for review on the imagelib side, and gavin for review on the browser side.

Pushed to try as e76097093fd8.
Assignee: nobody → bobbyholley+bmo
Attachment #455779 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #455802 - Flags: review?(joe)
Attachment #455802 - Flags: review?(gavin.sharp)
Duplicate of this bug: 516599
This looks green on try.
Comment on attachment 455802 [details] [diff] [review]
patch v3

>diff --git a/browser/base/content/sanitize.js b/browser/base/content/sanitize.js
>--- a/browser/base/content/sanitize.js
>+++ b/browser/base/content/sanitize.js
>@@ -112,21 +112,26 @@ Sanitizer.prototype = {
>   items: {
>     cache: {
>       clear: function ()
>       {
>         const Cc = Components.classes;
>         const Ci = Components.interfaces;
>         var cacheService = Cc["@mozilla.org/network/cache-service;1"].
>                           getService(Ci.nsICacheService);
>+        var imageCache = Cc["@mozilla.org/image/cache;1"].
>+                          getService(Ci.imgICache);
>         try {
>           // Cache doesn't consult timespan, nor does it have the
>           // facility for timespan-based eviction.  Wipe it.
>           cacheService.evictEntries(Ci.nsICache.STORE_ANYWHERE);
>         } catch(er) {}
>+        try {
>+          imageCache.clearCache(false); // true=chrome, false=content
>+        } catch(er) {}
>       },

Here you should probably put var imageCache just above its relevant try block, separated by a blank line from the earlier try block.

> NS_IMETHODIMP
> imgLoader::Observe(nsISupports* aSubject, const char* aTopic, const PRUnichar* aData)
> {
>-  NS_ASSERTION(strcmp(aTopic, NS_PREFBRANCH_PREFCHANGE_TOPIC_ID) == 0,
>-               "invalid topic received");
> 
>-  if (strcmp(NS_ConvertUTF16toUTF8(aData).get(), "image.http.accept") == 0) {
>-    nsCOMPtr<nsIPrefBranch> prefs = do_QueryInterface(aSubject);
>-    ReadAcceptHeaderPref(prefs);
>+  // Handle pref changes

Unnecessary comment.
Attachment #455802 - Flags: review?(joe) → review+
Attachment #455802 - Flags: review?(gavin.sharp) → review+
Attached patch patch v4Splinter Review
Updated to address joe's comments. Pushed to mozilla central:

https://bugzilla.mozilla.org/show_bug.cgi?id=576621
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
This needs a test as well.
Flags: in-testsuite?
Keywords: privacy
blocking2.0: ? → final+
Duplicate of this bug: 621379
I am not likely to ever go back and write a test for this
Flags: in-testsuite? → in-testsuite-
You need to log in before you can comment on or make changes to this bug.