87710 - Image cache does not re-validate images before reusing them...

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P3)

Description

[rpotts (gone)]

Currently, once an image is cached, it is never revaildated with the server.  

THis means that if the image was the result of dynamic content on the server
(ie. the same image URL returns different images for each request) only the
initial image will be displayed.

This bug was spun off from bug #69855 once all of the other issues were resolved.

Comment 1

[rpotts (gone)]

Oops, that should have been bug #68955 :-)

Comment 2

[Stuart Parmenter]

my code:

if (mCacheEntry && chan) {
  nsCOMPtr<nsICachingChannel> cacheChannel(do_QueryInterface(chan));
  if (cacheChannel) {
    nsCOMPtr<nsISupports> cacheToken;
    cacheChannel->GetCacheToken(getter_AddRefs(cacheToken));
    if (cacheToken) {
      nsCOMPtr<nsICacheEntryDescriptor> 
entryDesc(do_QueryInterface(cacheToken));
      if (entryDesc) {
        PRUint32 expiration;
        /* get the expiration time from the caching channel's token */
        entryDesc->GetExpirationTime(&expiration);

        /* set the expiration time on our entry */
        mCacheEntry->SetExpirationTime(expiration);
      }
    }
  }
}

gets 0 back from the http channel's get expiration time.  this is an http bug.  
(or the website not giving an expire time bug)

Comment 3

[Stuart Parmenter]

-> darin

Comment 4

[Darin Fisher]

-> mozilla 0.9.2 (hopefully)

Comment 5

[Darin Fisher]

Attachment: patch to fix broken "doomEntryIfExpired" cache feature

Comment 6

[Darin Fisher]

Attachment: v1.1 cache patch

Comment 7

[Darin Fisher]

Attachment: v1.0 http patch

Comment 8

[Darin Fisher]

both patches are required to fix the problem.

Comment 9

[Darin Fisher]

the trick to my patches are to set the cache expiration time appropriately so
that imagelib will be forced to ask http for the data at the right time.

Comment 10

[Bradley Baetz (:bbaetz)]

+    if (mResponseHead->MustRevalidate() == PR_FALSE) {

-> if (!mResponseHead->MustRevalidate()) {

r=bbaetz with that change

Comment 11

[Darin Fisher]

Attachment: v1.1 http patch (with bbaetz's suggestion)

Comment 12

[Johnny Stenback (:jst)]

sr=jst

Comment 13

[Darin Fisher]

checked in on trunk

Comment 14

[Christopher Blizzard (:blizzard)]

Is this something that we want in 0.9.2, too?

Comment 15

[Darin Fisher]

blizzard: yes

Comment 16

[John Morrison]

Hey darin. If it's the right fix then it's the right fix, but I think this 
change unwound the ``mysterious miracle speedup'' from last week. For 
certain, Mac times on the trunk have popped up to ~2400 on today's trunk build 
from ~2100 on the branch on cached page loads. We might want to track down what 
the dynamics of this are.

Comment 17

[Christopher Blizzard (:blizzard)]

Checked into the 0.9.2 branch as well.  John, if you're worried about the
performance of this then you should open another bug for that information.  I
need to close this for tracking.