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

RESOLVED FIXED in mozilla0.9.2

Status

()

Core
Networking: HTTP
P3
major
RESOLVED FIXED
17 years ago
17 years ago

People

(Reporter: rpotts (gone), Assigned: Darin Fisher)

Tracking

Trunk
mozilla0.9.2
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: checked in on trunk, critical for 0.9.2 - blizzard will check in, URL)

Attachments

(4 attachments)

(Reporter)

Description

17 years ago
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.
(Reporter)

Comment 1

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

Comment 2

17 years ago
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)
Assignee: pavlov → neeti
Component: ImageLib → Networking: HTTP
QA Contact: tpreston → benc

Comment 3

17 years ago
-> darin
Assignee: neeti → darin
(Assignee)

Comment 4

17 years ago
-> mozilla 0.9.2 (hopefully)
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla0.9.2
(Assignee)

Comment 5

17 years ago
Created attachment 40050 [details] [diff] [review]
patch to fix broken "doomEntryIfExpired" cache feature
(Assignee)

Comment 6

17 years ago
Created attachment 40053 [details] [diff] [review]
v1.1 cache patch
(Assignee)

Comment 7

17 years ago
Created attachment 40054 [details] [diff] [review]
v1.0 http patch
(Assignee)

Comment 8

17 years ago
both patches are required to fix the problem.
(Assignee)

Comment 9

17 years ago
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.
+    if (mResponseHead->MustRevalidate() == PR_FALSE) {

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

r=bbaetz with that change
(Assignee)

Comment 11

17 years ago
Created attachment 40055 [details] [diff] [review]
v1.1 http patch (with bbaetz's suggestion)
sr=jst
(Assignee)

Comment 13

17 years ago
checked in on trunk
Keywords: patch
Whiteboard: checked in on trunk
Is this something that we want in 0.9.2, too?
(Assignee)

Comment 15

17 years ago
blizzard: yes
Whiteboard: checked in on trunk → checked in on trunk, critical for 0.9.2 - blizzard will check in

Comment 16

17 years ago
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.
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.