Closed Bug 734062 Opened 12 years ago Closed 12 years ago

stale image in the image cache when an updated version is fetched into HTML5 offline cache in background

Categories

(Core :: Graphics: ImageLib, defect)

Product:
Core
Component:
Graphics: ImageLib
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla19
Tracking Flags:
Tracking Status
firefox16 --- affected
firefox17 --- affected
firefox18 --- affected
firefox19 --- fixed

People

(Reporter: weilou.happy, Assigned: mayhemer)

References

(Blocks 1 open bug)

Details

(Whiteboard: bug morphed - see comment 8)

Attachments

(4 files, 2 obsolete files)

test files
44.92 KB, application/x-gzip
Details
Offline cache test with added/removed/updated images
1.74 MB, application/octet-stream
Details
v1.2
6.41 KB, patch
mayhemer
: review+
mayhemer
: checkin+
Details | Diff | Splinter Review
unused variable fix
1.18 KB, patch
joe
: review+
mayhemer
: checkin+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 6.0) AppleWebKit/535.11 (KHTML, like Gecko) Chrome/17.0.963.66 Safari/535.11

Steps to reproduce:

Other browsers tested:
     Safari 5.1.2 on Vista: OK
     Safari on iOS 5.1: OK
  Chrome 17.0.963.66 on Vista: Fail

What steps will reproduce the problem?
1. Create a test page to let this page work offline via http://www.w3.org/TR/html5/offline.html
2. Try to update a file which is saved offline. E.g. Changing some texts in the .html file.

More info:
Chrome on Vista also has this bug. You might wanna check out my bug report on Chromium: https://code.google.com/p/chromium/issues/detail?id=117375


Actual results:

This file is not updated.


Expected results:

This file should can be updated.
Did you notice the big red text at the bottom of your w3.org URL and did you read https://developer.mozilla.org/en/Offline_resources_in_Firefox ?

Please provide an example URL
Now I provide detailed steps to reproduce this bug:

This bug still exists on Firefox 11. Safari 5.1.5 on PC and Safari on iOS 5.1 can both work fine.

1, Set MIME type "text/cache-manifest" to .appcache file on server.

2, Create an HTML file with codes below:

<!DOCTYPE html>
<html manifest="offline.appcache">
	<meta charset="utf-8">
	<title>A Firefox's Bug</title>
	<link href="index.css" rel="stylesheet">
	ABC

3, Create a CSS file index.css with codes below:

@charset "UTF-8";

body {
	color: red;
}

4, Create an offline.appcache file with codes below:

CACHE MANIFEST
# v1
index.css

5, Upload these 3 files to server.

6, Open your Firefox 11 stable version and then access to your page which is on server.

7, you should see characters "ABC" are in red.

8, Close this page's tab on Firefox.

9, Change the codes in index.css file with codes below:

@charset "UTF-8";

body {
	color: blue;
}

10, Upload this CSS file to server.

11, Change the codes in offline.appcache file to codes below:

CACHE MANIFEST
# v2
index.css

12, Upload offline.appcache file to server.

13, Access to your page which is on server.

14, Wait several minutes and then close this page's tab on Firefox.

15, Access to your page which is on server.

You should see characters in page are in blue. But what you see on Firefox 11 is that characters are still in red. This is a bug.

If anyone don't believe this is a bug relevant to HTML5's offline web app feature on Firefox, follow the reproduce steps provided by me. And then leave comment.

Mozilla should fix this bug ASAP.
No one works on fixing this bug?
Thanks for taking time to write the detailed steps to reproduce!

I guess the issue is that the manifest itself gets cached: when I follow the steps to reproduce using apache with default settings on Mac OS 10.7 the changes to the manifest file do not take effect immediately. But if I set

  ExpiresByType text/cache-manifest "access plus 0 seconds"

(as recommended on the MDC page Matti linked to), it works as you expect. Can you confirm that's the case? If not, we'll probably need the http log https://developer.mozilla.org/en/HTTP_Logging
Whiteboard: [closeme 2012-05-16]
Thank you for your reply! Nickolay_Ponomarev.

At the beginning you could't see update on display. And then you disabled browser cache for manifest file. It works OK. That's great!

And now I also find my issue and I find a new bug of Firefox.

The reason why I could't see update on display is because I set browser cache for some files like HTML, CSS, images, etc. After I disable the browser cache for these files, my issue fixed. :)

But I find a new issue relevant to Firefox's HTML5 offline web app API implementation. Follow steps below to reproduce this bug:

1, Disable browser cache for all files on server setting. E.g. manifest file, HTML file, CSS file, images file, etc.
2, Open latest stable version of Firefox, Chrome and Safari to access to a offline web page.
3, Update HTML file, a CSS file and an image file. Of course, update manifest file. The format of the image which I chose is JPG.
4, Open the offline web page on Firefox, Chrome and Safari. Wait a moment.
5, Open the offline web page on Firefox, Chrome and Safari AGAIN.

What's the bug:

On Chrome and Safari, you can see from the display that HTML file, CSS file and image file are all changed.
On Firefox, you can see from the display that HTML file and CSS file are changed. But you will find from the display that image file is not changed.

Nickolay_Ponomarev, you might can test to try to update an offline image file.
Please check the HTTP logs: https://developer.mozilla.org/en/HTTP_Logging
(the HTTP requests/responses and their headers)

It's impossible to tell what exactly happens in your case without knowing your exact server configuration / test files.
Ah, I now see what you meant and can reproduce on Nightly/Mac. Thanks!

STR:
1. Download the attached testcase, which contains two different versions of an HTML page (test.html), a stylesheet (index.css), and an image (test.png), plus the offline manifest.
2. Run "./run.sh 1", which renames the first version of each file to the file's canonical name (i.e. 1test.html -> test.html)
3. Load the test.html file via a web server, allow Firefox to create the offline cache for the page.
4. Run "./run.sh 2", which replaces the first version of each file with the second version.
5. Change the system time to get Firefox to reload the assets (I changed it 1 hour into the future).
6. Hit "Go" (Cmd+L, Enter) on the test.html page to reload it. (Cmd+R works too.)
7. Wait for the new assets to be fetched (you can check the progress in the HTTP log by searching for "http (request|response)").
8. Hit "Go" again.

Actual results: The HTML page and the CSS were updated (the text was changed, and its color changed from blue to red), but the old image is still displayed.

Expected results: The new image is displayed.

Note: about:cache shows that an updated image is in the offline cache, so the problem is not in necko.

Also note that Shift-reloading the page, as well as restarting the browser displays the correct image.

---

The issue is that imagelib relies on Necko to tell it when to discard the cached image. It tries to load the image's URI and if the response is read from the cache, imagelib assumes the resource was not changed and continues using its cache for the entry in question, see this code:

http://mxr.mozilla.org/mozilla-central/source/image/src/imgLoader.cpp#2145
NS_IMETHODIMP imgCacheValidator::OnStartRequest(nsIRequest *aRequest, nsISupports *ctxt)
{
  // If this request is coming from cache and has the same URI as our
  // imgRequest, the request all our proxies are pointing at is valid, and all
  // we have to do is tell them to notify their listeners.

The problem here is that the offline cache for the image is updated asynchronously, i.e. not in response to the load request from the imagelib. In other words:
- on step 6 imagelib gets image #1 from the offline cache.
- on step 7 the offline cache is updated, but the imagelib doesn't know about it
- on step 8 imagelib gets image #2 from the offline cache (cached response, same URL)
Status: UNCONFIRMED → NEW
Component: Untriaged → ImageLib
Ever confirmed: true
OS: Windows Vista → All
Product: Firefox → Core
QA Contact: untriaged → imagelib
Hardware: x86 → All
Summary: Firefox's HTML5 offline web app bug → stale image in the image cache when an updated version is fetched into HTML5 offline cache in background
Whiteboard: [closeme 2012-05-16] → bug morphed - see comment 8
Version: 10 Branch → Trunk
Attached file test files — 

    
    

    
  
First idea: compare aRequest->applicationCache->clientID with mRequest->applicationCache->clientID.  If equal, you have the same image.  (applicationCache may be null on any of aRequest or mRequest).

    
    

    
  
3 months passed. The fix to this bug is needed.

    
    

    
  
I can confirm and reproduce the bug with Firefox ESR 10.0.6 and 14.0.1 using a simplified test scenario (cache-test.zip attached):

1. Put the files into an apache www folder, activate mod_expires and make sure the .htaccess is processed.
2. Clear browser history cache and offline cache
3. Browse to cache-test/page1.html and allow offline caching.
(In firebug console you can see offline cache logging)
4. Switch server-side to version v2 of the cache-test. On Windows you can use switch-cache-test.bat for that.
(The v2 test pages have each one image removed [2+6], one added [3+7] and one changed [4+8]. html and cache manifest is changed accordingly.)
5. Reload cache-test/page1.html
(In firebug console you see update progress again. In a http sniffer you'll see that all new/modified files are retrieved from the server, incl img 4 and 8. That's because of .htacces -> ExpiresDefault "access plus 0 seconds")
6. Reload again to see the new offline-cached page version.

Expected result: The complete new version of the web site (both pages) should be offline-cached and shown with updated images.

Actual result: You see the updated html pages, you see the new images 3+7, but the OLD images 4 and 8 (the updated ones are really colorful) are still used. Going from page1 to 2 and back doesn't help, clicking reload multiple times doesn't help.

The offline cache was really updated but modified images are stale until you do one of the following (block network if you think the browser might not have the modified image yet):
- Close and restart the browser
- Hit SHIFT + Reload
- In Firebug move the mouse over the modified image html element. The tooltip will show the updated image and it will be available on next normal reload.

In my test it didn't matter whether privacy settings -> "saved visited pages" was on or off.

    
    

    
  
Can somebody try to implement the suggestion in comment 10?

    
    

    
  
Unbelievable! Half year passed since this bug was reported, the bug hasn't been fixed. This can't be accepted. #BadImpressionToMozilla

    
    

    
  
> Can somebody try to implement the suggestion in comment 10?
Honza: I tried it a while ago and, as far as I remember, it didn't work as is.

IIRC, when I tried something that compiled successfully (with something like mRequest.request.getInterface(nsIApplicationCacheContainer).applicationCache, only with C++ syntax), it turned out mRequest or mRequest.request was nulled out after loading).

Note: I'm writing this from memory, since I lost the notebook where my attempt at this was saved.

    
    

    
  
I am waiting the fix to this bug. :-)

    
    

    
  
This bug should be fixed ASAP!!!

    
    

    
  
I'll try to fix this.
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED

    
    

    
  
Thank you, Honza.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v1 (obsolete)
        — 
          — Splinter Review
      

    


  
- make the image request that is held in image cache keep ref to app cache it has load from
- during validation check both the image-cacheed and the new load are coming from the same application cache or both just from http cache
- when the previous check fails, reload the image

Problem of the image cache is that it maps entries by URL.  But content under the same URL can be different in HTTP cache and appcache.  We can also have different appcaches with potentially different content under the same URL.


Try run: https://tbpl.mozilla.org/?tree=Try&rev=b620d48d79f1

        Attachment #679298 -
        Flags: review?(jmuizelaar)

        Attachment #679298 -
        Flags: review?(jmuizelaar) → review?(joe)

    
    

    
  
Comment on attachment 679298 [details] [diff] [review]
v1

Review of attachment 679298 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/src/imgRequest.cpp
@@ +444,5 @@
>  
> +namespace { // anon
> +
> +nsresult
> +GetApplicationCache(nsIRequest* aRequest, nsIApplicationCache** _result)

This should return already_AddRefed<nsIApplicationCache> instead of using XPCOM-style nsresults (and just return nullptr in the error case).

@@ +451,5 @@
> +
> +  nsresult rv;
> +
> +  nsCOMPtr<nsIApplicationCacheChannel> appCacheChan =
> +    do_QueryInterface(aRequest);

put this on a single line

@@ +496,5 @@
> +  NS_ENSURE_SUCCESS(rv, true);
> +  rv = newAppCache->GetClientID(newAppCacheClientId);
> +  NS_ENSURE_SUCCESS(rv, true);
> +
> +  if (oldAppCacheClientId != newAppCacheClientId)

Are app cache client IDs globally unique? It'd be nice to not have to store a reference to the app cache inside the imgRequest.

        Attachment #679298 -
        Flags: review?(joe) → review+

    
    

    
  
(In reply to Joe Drew (:JOEDREW! \o/) from comment #22)
> Are app cache client IDs globally unique? It'd be nice to not have to store
> a reference to the app cache inside the imgRequest.

They are unique per profile and are persistent during a profile lifetime.

I'd personally rather keep the whole object, it is actually very small, just keeping group and client ids.

Do you have any serious concerns about not keeping the ref?

I want to introduce a new method |bool nsIApplicationCache.equals(nsIApplicationCache)| that will do the check of appcache object equality, the same we do here.  In reality it happens very rarely we would have two objects with the same clientid, but it still can happen.  Check for clientid is just an additional check.  In 99% of cases the test for pointer equality will pass.  And check for pointer equality is much cheaper then check for relatively long strings equality.

    
    

    
  
(In reply to Honza Bambas (:mayhemer) from comment #23)
> I'd personally rather keep the whole object, it is actually very small, just
> keeping group and client ids.
> 
> Do you have any serious concerns about not keeping the ref?

No, not really. Do what feels best.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v1.1 (obsolete)
        — 
          — Splinter Review
      

    


  
- addressed review comments except keeping a ref to an app cache object
- slightly changed the compare algorithm to a) fix an error in it b) make it more obvious it's faster to check for pointers

        Attachment #679298 -
        Attachment is obsolete: true

        Attachment #679362 -
        Flags: review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v1.2
        — 
          — Splinter Review
      

    


  
An even a bit better one:

- addressed review comments except keeping a ref to an app cache object
- slightly changed the compare algorithm to a) fix an error in it b) make it more obvious it's faster to check for pointers
- updated comments
- changed name of the new method to just CacheChanged, since, for instance, when we have an image in the image cache loaded from appcache (any one) and the new load comes from http cache (appcache == null), then cache used to load is actually also different

Carrying r+, but Joe, feel free to recheck the patch and drop r.

        Attachment #679362 -
        Attachment is obsolete: true

        Attachment #679369 -
        Flags: review+

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/8e14e87e07c4
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19

    
    

    
  
Thank you guys for the bug fix.

    
    

    
  
Will Firefox 17 stable version be the first stable version that hasn't this bug?

    
    

    
  
(In reply to Wei Lou from comment #31)
> Will Firefox 17 stable version be the first stable version that hasn't this
> bug?

No, it is fixed in Firefox 19.

I don't think this will get uplift even to 18.  It is not a regression and we are late in the cycle (less then a week to channel merge).

          status-firefox16:
          --- → affected

          status-firefox17:
          --- → affected

          status-firefox18:
          --- → affected

          status-firefox19:
          --- → fixed

    
    

    
  
Thank you for your answer, Honza.

    
    

    
  
This is not a regression, but a pretty serious and long standing bug. It hinders serious development of HTML5 offline apps.
Does that mean that this will be not included into the ESR line before 24 or is there a chance to mark it relevant for at least 17.0.x?

    
    

    
  
Comment on attachment 679369 [details] [diff] [review]
v1.2

I'll try based on comments making this a "very wanted fix", still I don't believe this is much worth of approval even for the aurora channel.

[Approval Request Comment]

Bug caused by (feature/regressing bug #): Introduction of offline application cache support (years ago)

User impact if declined: We may not display a correct image when a page is later (second time) loaded with a different cache (HTTP -> app cache, or app cache -> newer version app cache)

Testing completed (on m-c, etc.): landed day ago on m-c, no automated test for this particular code present

Risk to taking this patch (and alternatives if risky): Potential very minor slowdown in image loading time, since we may prioritize a load from disk cache over from the image cache in cases where it would not be potentially needed (however, this risk is IMO very low according the simplicity of the patch).

String or UUID changes made by this patch: none

Not requesting for beta, since beta is about to become release in about a week, this is simply to late.

        Attachment #679369 -
        Flags: approval-mozilla-aurora?

    
    

    
  
Comment on attachment 679369 [details] [diff] [review]
v1.2

Review of attachment 679369 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/src/imgRequest.cpp
@@ +476,5 @@
> +{
> +  nsresult rv;
> +
> +  nsCOMPtr<nsIApplicationCache> newAppCache = GetApplicationCache(aNewRequest);
> +  NS_ENSURE_SUCCESS(rv, true); // cannot determine, play safely

My compiler was so kind to tell me you're using 'rv' uninitialized here.

    
    

    
  
(In reply to :Ms2ger from comment #36)
> My compiler was so kind to tell me you're using 'rv' uninitialized here.

Windows cl is not... Thanks!  Quickfixing right now.

    
    

    
  


  

        Attachment #680584 -
        Flags: review?(joe)

        Attachment #680584 -
        Flags: review?(joe) → review+

    
    

    
  
Comment on attachment 679369 [details] [diff] [review]
v1.2

Dropping aurora approval request.  It is too late to do it.

        Attachment #679369 -
        Flags: approval-mozilla-aurora?

    
    

    
  
Can someone please verify if this is fixed on Firefox 19 beta 5?

ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/19.0b5-candidates/build1/

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: