Last Comment Bug 89419 - [PATCH] Caching of images loaded from a 302 is broken
: [PATCH] Caching of images loaded from a 302 is broken
Status: VERIFIED FIXED
: regression, verified1.8.1.18, verified1.9.0.5, verified1.9.1
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: All All
: P1 major with 16 votes (vote)
: mozilla1.9.2a1
Assigned To: Joe Drew (not getting mail)
:
: Milan Sreckovic [:milan]
Mentors:
http://capricorn.woot.net/~jdrew/bugs...
: 101502 168089 224282 229839 257727 265052 281486 281754 295810 308252 308296 (view as bug list)
Depends on: 328903 473161 479328
Blocks: 68423 121518 466230 48202 Heidi 158715 223142 229839 CVE-2008-5012 458845 470838
  Show dependency treegraph
 
Reported: 2001-07-05 10:18 PDT by Crispin Flowerday (not receiving bug mail)
Modified: 2014-04-26 03:32 PDT (History)
50 users (show)
asa: blocking1.8b3-
asa: blocking1.8rc1-
vladimir: blocking1.9.1+
hskupin: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed patch for fixing caching of the images for 302 response (785 bytes, patch)
2002-02-22 01:08 PST, Nivedita (gone)
no flags Details | Diff | Splinter Review
trace of browser interaction (10.72 KB, text/plain)
2002-02-22 01:10 PST, Nivedita (gone)
no flags Details
a suggested patch (1004 bytes, patch)
2003-04-03 03:50 PST, Jim Dunn
no flags Details | Diff | Splinter Review
Handle OnRedirect requests (3.90 KB, patch)
2003-05-07 13:43 PDT, Jim Dunn
darin.moz: review-
Details | Diff | Splinter Review
New patch that addresses previous comments (3.33 KB, patch)
2003-05-14 14:08 PDT, Jim Dunn
tor: review+
darin.moz: superreview+
Details | Diff | Splinter Review
Un-bitrot jdunn's patch, and address bug 458845 at the same time (29.50 KB, patch)
2008-10-20 19:37 PDT, Joe Drew (not getting mail)
no flags Details | Diff | Splinter Review
how about we don't mash two patches together (11.33 KB, patch)
2008-10-20 20:03 PDT, Joe Drew (not getting mail)
vladimir: review+
Details | Diff | Splinter Review
Only change the cache key, and proxy notification callbacks (11.81 KB, patch)
2008-11-21 14:27 PST, Joe Drew (not getting mail)
no flags Details | Diff | Splinter Review
Fix a major bug in the previous patch (11.85 KB, patch)
2008-11-21 14:38 PST, Joe Drew (not getting mail)
bzbarsky: review-
Details | Diff | Splinter Review
Fix reftest failures found in bug 355126 (11.85 KB, patch)
2008-11-27 13:40 PST, Joe Drew (not getting mail)
bzbarsky: review+
vladimir: superreview+
Details | Diff | Splinter Review
Fix leaks caused by cycle (12.77 KB, patch)
2008-12-18 15:57 PST, Joe Drew (not getting mail)
bzbarsky: review+
vladimir: superreview+
Details | Diff | Splinter Review

Description Crispin Flowerday (not receiving bug mail) 2001-07-05 10:18:14 PDT
From Bugzilla Helper:
User-Agent: Mozilla/4.77 [en] (X11; U; Linux 2.4.4 i686)
BuildID:    2001061506

If an image is served via a redirect, e.g an HTTP response of:

HTTP/1.1 302 Moved Temporarily
Server: Zeus/4_0
Date: Thu, 05 Jul 2001 16:47:37 GMT
Connection: close
Expires: Thu, 01 Jan 1970 00:00:00 GMT
Location: /images/image.gif
Pragma: no-cache
Cache-Control: no-cache


The image is loaded, but the original URL ( the one that responded with the
redirect) has the image cached with it, even though it is asking not to be
cached.


Reproducible: Always
Steps to Reproduce:
1. Have an image served via a cgi script that returns a 302 response, with
caching turned off
2. The image will be loaded correctly, but on subsequent requests, the cached
object is used, rather than reloading the original request.
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2001-07-05 11:20:09 PDT
Chances are, the URL being redirected to does not send any no-cache headers....

confirming bug.
Comment 2 Bradley Baetz (:bbaetz) 2001-07-05 11:41:33 PDT
Is there a URL which shows this?

If the new URL doesn't have any cache headers, then we are allowed to cache the
image at the new url. This may be an imglib bug, but I'd need a testcase to be sure.

We never cache redirects currently, even the cachable responses.
Comment 3 Crispin Flowerday (not receiving bug mail) 2001-07-06 01:44:56 PDT
I am afraid I dont have access to a website where I can put a test case up, but
if I have a fastcgi script returning the response shown in my original report,
and then I look at about:cache, it shows:

       Key: http://pallas:2000/fcgi/redirect.fcgi
     Data size: 768 Bytes
   Fetch count: 1
 Last Modified: Fri Jul 6 10:39:47 2001
       Expires: Fri Jul 6 12:15:01 2001

           Key: http://pallas:2000/images/image.gif
     Data size: 768 Bytes
   Fetch count: 1
 Last Modified: Fri Jul 6 10:39:48 2001
       Expires: Fri Jul 6 12:15:01 2001


The image was served served normally - i.e it had no expires headers, and allows
the browser to cache it.

I believe that the image should be cached normally, but the actual redirect
should not be.
Comment 4 Crispin Flowerday (not receiving bug mail) 2001-07-06 02:36:43 PDT
A demo of the bug is at http://moonraker.ddts.net:7000/ . the image should
alternate between red and blue every time the page is looked at (click the link
to show the problem).
Comment 5 benc 2001-07-17 09:47:19 PDT
Bradley:
I would assume we cache re-directs, otherwise, we would poll the server
everytime the end-user went to "/" or reloaded pages that have re-directed
graphics...
Comment 6 Bradley Baetz (:bbaetz) 2001-07-17 10:54:19 PDT
benc: correct. We do hit the server each time (just verified with ethereal on
www.mozilla.org/hacking)

Theres a bug on this, I think.

The urls given here no longer work, so I can't look at this in more detail...
Comment 7 Crispin Flowerday (not receiving bug mail) 2001-07-18 01:51:55 PDT
The moonraker.ddts.net should be working now - my cable modem got a bit
confused, if that doesn't work, try http://213.107.104.188:7000/ (which should
work until it changes address)
Comment 8 gordon 2001-07-25 12:25:32 PDT
Darin, this looks a bit more like HTTP than Cache.
Comment 9 benc 2001-07-29 06:24:36 PDT
Mozilla 0.9.2, Win 98.

I'm looking at the new test page, and it when I hit reload, it loads the new
image every time, whatever my cache setting is, including "never".

I think is working the way people want it. Perhaps the implmentation of the
original problem was slightly different?
Comment 10 Crispin Flowerday (not receiving bug mail) 2001-07-29 11:17:07 PDT
The issue is not what happens when you hit the reload button - that is correct.
The bug is that when you link back to the page, the cached image is used. Tyr
clicking on the link just below the image on the test page. This should cause
the page to reload, and the image to change colour.

I have just tryed this again with the latest nightly build - 2001062823, under
Linux and it just gives me the blue image.
Comment 11 John Taylor 2001-07-30 16:43:22 PDT
-> darin
Comment 12 Bradley Baetz (:bbaetz) 2001-07-30 16:56:23 PDT
actually, after poking at the imglib cache stuff last night, -> imagelib. Its
using the initial url as the thing to cache.
Comment 13 Bradley Baetz (:bbaetz) 2001-09-25 06:31:02 PDT
*** Bug 101502 has been marked as a duplicate of this bug. ***
Comment 14 Tom Mraz 2001-09-25 06:50:17 PDT
Also the same behavior should be if 307 and 303 response is sent. On the other
hand the current behavior is OK for 301 response.
Also raising the severity to major because we want to conform to http RFC.

Comment 15 Jesse Ruderman 2001-10-12 15:31:38 PDT
See also bug 69486, Cannot block images which have redirects.
Comment 16 Tom Mraz 2002-02-07 01:06:21 PST
Nominating sooner than to Future because it's non conformant behaviour to the
HTTP RFC.

Comment 17 Nivedita (gone) 2002-02-18 22:57:07 PST
taking from pavlov
Comment 18 Nivedita (gone) 2002-02-22 01:08:11 PST
Created attachment 70884 [details] [diff] [review]
proposed patch for fixing caching of the images for 302 response

This happens as, in the method ProcessRedirection, NS_OpenURI is called for the
new location, hence a new request in itself, which has no cache related headers
hence does not know that its response should not be cached. I have also
attached the http request and response for the above transaction.

Since, index.html, redirect.fcgi and red.gif do not have cache headers hence it
is normal for them to be cached. But blue.gif should not be cached. I have
added check to set the mLoadFlags as LOAD_BYPASS_CACHE, if the response has
cache related headers. Though the patch  works if the caching preference "Every
time I view the page", it doesnt work against the other preferences of cache. 

Darin: Can you please convey your thoughts on above
Comment 19 Nivedita (gone) 2002-02-22 01:10:36 PST
Created attachment 70885 [details]
trace of browser interaction

added for the comment 70884
Comment 20 Crispin Flowerday (not receiving bug mail) 2002-02-22 01:48:41 PST
> Since, index.html, redirect.fcgi and red.gif do not have cache headers hence it
> is normal for them to be cached. But blue.gif should not be cached.

I am sure that statement is not correct, index.html, red.gif and blue.gif all
don't have the no-cache headers, it is the redirect.fcgi which has a redirect
and headers saying not to cache:


HTTP/1.1 302 Moved Temporarily
Server: Zeus/4.1
Date: Fri, 22 Feb 2002 09:45:05 GMT
Connection: close
Expires: Thu, 01 Jan 1970 00:00:00 GMT
Location: /red.gif
Pragma: no-cache
Cache-Control: no-cache


mozilla then seems to get /red.gif, see that it can cache that file and assume
it can cache the redirect as well.
Comment 21 Darin Fisher 2002-02-22 11:43:22 PST
nivedita: iirc the problem here is that imagelib uses the original URL as its
cache key instead of the final URL resulting from the redirect.  necko is not
caching the redirect.  instead imagelib is caching the image that resulted from
the redirect *under the original URL*.  if imagelib cached it under the final
URL instead then on a subsequent request for the original URL, imagelib would
have to fetch the URL from necko.  as a result, necko would check its cache and
see that the URL must be refetched (Pragma: no-cache).  then, imagelib would get
new data or a 304.  pavlov has code for handling the 304 case (to avoid
redecoding image data).  nivedita: your patch is not at all the correct
solution.  i believe this is purely an imgLoader bug.
Comment 22 Nivedita (gone) 2002-02-25 00:14:51 PST
giving it back to pavlov, as he has a fix for it, as per the comment 21  from 
Darin.
Comment 23 Jim Dunn 2003-02-03 13:28:38 PST
This sounds like a dupe of Bug 121084.  Granted this one is
older, but that one has more info, a suggested patch and a testcase
that shows the problem.  If people agree, I would like to dup
this bug of that one.  
Comment 24 Tom Mraz 2003-02-03 23:55:12 PST
It seems to me (if I understand the bug 121084) that the patch in that bug would
make this bug more visible. So to fix the bug 121084 correctly we would need to
fix this bug first.
See Darin's comment 183 in the bug 121084.

So definitely not a dupe.
Comment 25 Jim Dunn 2003-02-04 06:46:13 PST
"mass" re-assigning of bugs from pav to myself
Comment 26 Jim Dunn 2003-02-19 12:10:59 PST
updating target milestone
Comment 27 Jim Dunn 2003-04-02 10:11:01 PST
Is there a testcase still around?
I would like to verify this against an issue I found
involving the session history basically caching a redirect...

Is the test something like
http://home.maine.rr.com/jimdunn/simple_redirect.html
and if so, then is the bug... 
1) load this url
2) we see 3 HTTP GET's
3) click on reload, 
4) we see only 2 GET's?  (i.e. we never do the middle GET for 
   http://ads07.hyperbanner.net:1007/gif.cfm?ID=1&Page=1&Ver=27)
Comment 28 Tom Mraz 2003-04-02 23:27:01 PST
jdunn:

yes, this is exactly the problem. Esp. because the original URL
http://ads07.hyperbanner.net:1007/gif.cfm?ID=1&Page=1&Ver=27 could redirect to
different image after subsequent reloads and if it's skipped the image never
changes.
Comment 29 Crispin Flowerday (not receiving bug mail) 2003-04-03 01:08:41 PST
Hmm, sorry I rearranged my websites and test cases, the testcase is now at:

http://bugs.flowerday.cx/mozilla89419/
Comment 30 Jim Dunn 2003-04-03 03:50:07 PST
Created attachment 119285 [details] [diff] [review]
a suggested patch 

Thanks for the test... I did hunt down the problem earlier,
the redirect is in the document's session history, when we go to 
load it, we check the history and end up loading the final
uri instead.  By setting the loadtype for Redirects to bypass
history we avoid putting the redirect uri into the history and
therefore don't find it later.	

I am guessing there is a much cleaner/better way to do this, but
need some help.  I just posted this patch to get the creative
juices flowing
Comment 31 Jim Dunn 2003-04-03 04:02:27 PST
Adding a couple of other people for their ideas since this is hitting
nsDocShell (or at least I think that is where the fix lies)
Comment 32 Boris Zbarsky [:bz] (still a bit busy) 2003-04-03 09:44:09 PST
radha and adam are probably the ones most familiar with this code....
Comment 33 Jim Dunn 2003-05-07 13:43:05 PDT
Created attachment 122695 [details] [diff] [review]
Handle OnRedirect requests

What I found is that necko was doing what it was supposed to be doing
however, as darin pointed out, we were storing as cache key, the
redirect url... so while necko was resolving the redirect and getting
the correct gif, imageLib would just see that the "request" was in 
cache (cacheChan->IsFromCache(&isFromCache)) && isFromCache) and so
we wouldn't re-get from necko.	
Darin suggested (along with Pav) that imageLib should probably 
handle OnRedirect.  This patch enabled OnRedirect requests to be
handled by imgRequest.
Comment 34 Darin Fisher 2003-05-07 14:39:42 PDT
Comment on attachment 122695 [details] [diff] [review]
Handle OnRedirect requests

>Index: imgLoader.cpp

>+      newChannel->SetNotificationCallbacks(request);
...
>+    newChannel->SetNotificationCallbacks(request);
...
>+    channel->SetNotificationCallbacks(request);

make sure you break this reference cycle in OnStopRequest.  also,
can these calls be made from one common place?


>Index: imgRequest.cpp

>+                              nsIHttpEventSink, nsIInterfaceRequestor)

nit: add line break between interfaces.


>+NS_IMETHODIMP
>+imgRequest::OnRedirect(nsIHttpChannel *aHttpChannel, nsIChannel *aNewChannel)
>+{
>+  NS_ENSURE_ARG_POINTER(aNewChannel);

no need for runtime pointer check.  use NS_ASSERTION instead.
junk in should equal junk out... necko should not be able to
get away with calling OnRedirect with a bogus aNewChannel.


>+  RemoveFromCache();

hmm... iirc, pav was worried about what would happen if multiple
image requests were queued up on this one cache entry.	i'm not
sure this code is wrong, but i'll have to think about it some
more.  you might want to try loading a page with the same image
repeated multiple times (the image should be loaded via redirect
of course).
Comment 35 Stuart Parmenter 2003-05-07 15:50:33 PDT
Please don't use macros that have returns in them in imagelib code.  (i.e.
things like NS_ENSURE_ARG_POINTER())
Comment 36 Jim Dunn 2003-05-14 14:08:28 PDT
Created attachment 123325 [details] [diff] [review]
New patch that addresses previous comments

I think I address all previous comments.  The only part I left in
was the RemoveFromCache() call in the imgRequest::OnRedirect.

I have a testcase (http://home.maine.rr.com/jimdunn/bugs/redirect_img.html)
that loads a redirected image in a variety of manners (and it loads the
same redirect uri a number of times).  Without the "RemoveFromCache" it
appears that we are running into the same issue of on each LoadImage request,
we get the redirected.uri, which is still in the cache and so we don't 
seem to follow the redirect.  I am continuing to look at this but wanted to
put up the lastest patch, which addresses the previous comments.
Comment 37 Jim Dunn 2003-05-15 06:45:56 PDT
>RemoveFromCache()
So I have done some extensive testing on whether we want this call in
or not.  Without it, we don't fix the problem, so we either need it or
some other form of "invalidating" the redirect.cgi entry in the cache.

However in testing, I created a fairly intensive test that
loads this redirect image 3 times in the first frame,
3 times in a 2nd frame interspersed with 20 other LARGE images
and then 3 times in a third frame.  All 9 of these image
LoadImage requests end up finding the redirect.cgi entry in the
cache and re-use that.  Then using the same "cache entry", the
redirect fixes the entry.  So I think removing from cache is the
right thing.
Comment 38 Darin Fisher 2003-05-15 16:23:30 PDT
Comment on attachment 123325 [details] [diff] [review]
New patch that addresses previous comments

looks good to me... just add some documentation to OnRedirect explaining what
is going on here.  sr=darin with that change.
Comment 39 Hermann Schwab 2003-06-11 05:06:26 PDT
This bug has a reworked patch with SR nearly a month now, had targets of 1.2 and
1.4b, breaks parity with Netscape 4.8
What is wrong with it? Should it bit-rot?
Comment 40 Christian :Biesinger (don't email me, ping me on IRC) 2003-06-11 08:23:01 PDT
Hermann: The patch does not have review yet.
Comment 41 Marc Boon 2003-10-23 01:55:22 PDT
Will someone please review this and make sure it gets checked in?
We are now more than 4 months later, 1.5 just released, and this major bug with
a target milestone of 1.4 beta is just sitting here.
Comment 42 Darin Fisher 2003-10-24 21:54:27 PDT
taking for 1.6 beta.

marc: sorry! please understand that with the loss of the netscape team, we have
a lot of orphaned bugs in the database (even some like this one that include
patches).  the mozilla developers are doing our best to keep things moving
along, but there are soooo many dark corners where things can easily get lost :-/
fortunately, work is underway to reorganize the bug system and to assign better
owners to the huge number of orphaned or otherwise underowned bugs.
Comment 43 Dave 2003-11-02 02:49:11 PST
I hate to add spam to an already-long bug discussion, especially one that seems
like a done deal waiting for review.  But while investigating Bug 224282, I read
RFC 2068 and RFC 2616.  From both of those specs, in 10.3.3:  "This response is
only cachable if indicated by a Cache-Control or Expires header field."  I just
want to be clear whether this spec is being applied correctly, because various
comments and the original bug report make it seem like we should fix it so that
the response to the redirected URI still gets cached with the original URI
unless there is an explicit no-cache.  The spec seems to say that in the case of
a 302, we need an explicit directive _allowing_ caching; and that we _don't_
need an explicit directive preventing it.  After reading and re-reading, I am
still having trouble following parts of this bug's discussion, so I may be
misunderstanding something; but take this as a reminder just in case (not to
mention that I hope I am reading the spec correctly).

Additionally, could someone please look at Bug 224282 and dupe it against this
bug or Bug 168089 as may be necessary?  (Sounds most like this bug, although the
reporter claims it is a problem in Firebird only.)  Thanks!
Comment 44 Tom Mraz 2003-11-02 23:31:47 PST
*** Bug 224282 has been marked as a duplicate of this bug. ***
Comment 45 Tom Mraz 2003-11-02 23:32:59 PST
*** Bug 168089 has been marked as a duplicate of this bug. ***
Comment 46 Darin Fisher 2003-11-05 13:38:35 PST
Comment on attachment 123325 [details] [diff] [review]
New patch that addresses previous comments

tor: this ones been in pav's queue for a while... can you please take a look? 
thanks!
Comment 47 Darin Fisher 2004-01-26 10:58:02 PST
update: about a month ago, tor and i discussed this bug some.  we were concerned
that there might be a reference cycle between the channel and the image request
(based on the image request setting itself as the notification callbacks for the
channel).  i don't want to make the mistake of checking this patch in until the
possibility of that problem has been completely eliminated.

-> moz 1.7a
Comment 48 Daniel de Wildt 2004-02-13 00:27:23 PST
*** Bug 229839 has been marked as a duplicate of this bug. ***
Comment 49 Daniel de Wildt 2004-02-13 04:38:31 PST
I have tested the patch in attachment 123325 [details] [diff] [review] and it looks that the images are
cached in with the correct key. This solves the problems with redirects and
imgCache::put. 

What is with the imgCache::get in imgLoader? 
Redirects will now never have a cache hit. Is it possible in
imgLoader::LoadImage to perform a new imgCache::get when AsyncOpen got a
redirection?
Comment 50 Arne Burmeister 2004-08-27 05:23:08 PDT
The problem also appears on an iframe:

The src attribute references a servlet redirecting ansering 302. A reload does
not access the servlet any more but reloads the former redirect target.
Comment 51 WADA 2005-02-08 12:19:37 PST
(In reply to comment #50)
> The problem also appears on an iframe:
> The src attribute references a servlet redirecting ansering 302. A reload does
> not access the servlet any more but reloads the former redirect target.
IFRAME case is possibly different from usual element such as IMG, because
session histry has relation to reload.
See Bug 279048 comment #1.

Comment 52 WADA 2005-02-11 20:00:44 PST
(In reply to comment #50)
> The problem also appears on an iframe:
I confirmed your problem.
  If SRC value(URL) of <IFRAME> is static but the URL is redirected to other URL
  by "302 Found", "HTTP GET" is issued to the last redirected URL when "Reload"
  instead of URL specified on <IFRAME>, then no chance of new URL if "Reload".
No cache related issue is involved in your problem, although it looks as if "302
Not Found" is cached (See Bug 279048 for detail).

Arne Burmeister, open new bug for your "302 Found" & IFRAME & "Reload" problem
with Product=Core/Component=History:Session, please.  
Comment 53 WADA 2005-02-13 23:24:17 PST
I've opend Bug 282198 for problem of comment #50 (redirect of IFRAME's URL case). 
Comment 54 Boris Zbarsky [:bz] (still a bit busy) 2005-02-15 09:22:53 PST
*** Bug 281486 has been marked as a duplicate of this bug. ***
Comment 55 Boris Zbarsky [:bz] (still a bit busy) 2005-02-15 09:23:42 PST
*** Bug 281754 has been marked as a duplicate of this bug. ***
Comment 56 WADA 2005-02-15 12:29:00 PST
(In reply to comment #47)
Darin Fisher, what is "Data size: 24000 bytes" entry in cache?
When Bug 281486 comment #5 situation("302 then 304"), "Location: URL" on "302"
is returned? Or cache entry for "Location: URL" is returned to requester?
If cache entry is returned, which entry is returned to requester?
 - Cache entry for "Location: URL" to which "304" is returned
 - Cache entry for "Location: URL" to which "200" was returned
If cache entry is returned, bug 281486 comment #5 (see #7 also) may be one of
causes of the problem, because timestamp of "Data size: 24000 bytes" entry is
not updated when "302 then 304".
Comment 57 WADA 2005-02-15 18:25:38 PST
(In reply to comment #56)
Above  "Data size: 24000 bytes" is cache entry in next flow.
This funny "Data size: 24000 bytes" entry is the entry will be deleted by 
RemoveFromCache(); in imgRequest::OnRedirect of attachment 123325 [details] [diff] [review]?
(and worrying of comment #34, description of comment #36 & comment #37?)

  (1) HTML.html contains <IMG SRC=IMG.php>
      and IMG.php is redirected to JPEG-1.jpg, JPEG-2.jpg, ... ,JPEG-N.jpg
  (2) HTML.html,JPEG-1.jpg, ... JPEG-N.jpg are cached
  (3) T1 : "Shift+Reload"  
      HTTP GET HTML.html
      200
      HTTP GET IMG.php
      302 Found, Location: JPEG-A.jpg
      HTTP GET JPEG-A with If-Modified-Since
      304 Not Modified
        Cache Entries
          KEY: IMG.php,
               Data size:     24000 bytes
               Fetch count:   1
               Last modified: T1
               Expires:       1970-01-01 09:00:00
          KEY: IMG.php,
               Data size:     0 bytes
               Fetch count:   (Incremented)
               Last modified: T1
               Expires:       1970-01-01 09:00:00
  (4) T2 : "Reload"  
      HTTP GET HTML.html with If-Modified-Since
      304 Not Modified
      HTTP GET IMG.php
      302 Found, Location: JPEG-B.jpg
      HTTP GET JPEG-B with If-Modified-Since
      304 Not Modified
        Cache Entries
          KEY: IMG.php,
               Data size:     24000 bytes
               Fetch count:   (Incremented)
               Last modified: T2-X-1 (probably T2, but not sure)
               Expires:       T2-X-2 (greater then T2-X-1, but near)
          KEY: IMG.php,
               Data size:     0 bytes
               Fetch count:   (Incremented)
               Last modified: T2 (updated)
               Expires:       1970-01-01 09:00:00
  (4) T3 : "Reload"  
      Same flow as (3) except URL of Location: on "302"
        Cache Entries
          KEY: IMG.php,
               Data size:     24000 bytes
               Fetch count:   (Incremented)
               Last modified: T2-X-1 (Not updated)
               Expires:       T2-X-2 (Not updated)
          KEY: IMG.php,
               Data size:     0 bytes
               Fetch count:   (Incremented)
               Last modified: T3 (updated)
               Expires:       1970-01-01 09:00:00

Comment 58 WADA 2005-02-15 20:53:46 PST
Correction of data in comment #57.
Cache entry data of 'T3 :"Reload"' is correct, but data for T1 & T2 was incorrect.
T1's data was when "200, Cache-Control:no-store" for HTTP GET JPEG-n.jpg after
"302". (I confused on my log data).
Sorry for spam.
Comment 59 WADA 2005-02-18 07:59:59 PST
(In reply to comment #56)
> what is "Data size: 24000 bytes" entry in cache?
It was CacheEntryDescriptor.
I misunderstood that this descriptor entry is maintained by HTTP/CACHE combination.
But next result says HTTP protocol handler won't produce such entry when redirect.
  - When URL-RANDOM.php is redirected to HTML-N.html or JPEG-N.jpg randomly,
    and when URL-RANDOM.php is entered in URL_Bar and loaded,
    no cache descriptor entry(size=24000 bytes) for URL-RANDOM.php was created.
    Only data entry(size=0 byte) was created for URL-RANDOM.php
    which is redirected.
As final jpeg has both descriptor entry(size=24000) & data entry(size=file size),
redirected URL(URL-RANDOM.php) also has both cache entries,
because it's SRC URL of <IMG> tag then descriptor entry(size=24000)) is written
by image handler,
and data entry(size=0, for "302"/Location: response) is written by HTTP protocol
handler side.
Sorry for my silly questions without studying on spec or logic.

[Summary on cache descriptor entry(size=24000, Key=URL-RANDOM.php) when redirected]
This descriptor entry(size=24000) for URL-RANDOM.php was updated(or
deleted/redefined) when "Shift+Reload". 
  - "Fetch count:" was always reset to 1,
    although "Fetch count:" of data entry(size=0) was incremented.
    (This indicates delete/redefine.)
  - "Last Modified:" was always greater than or equal to
    "Last Modified:" of data entry(size=0, for "302 Found" response).
  - "Expires:" seemed to be "Last Modified:" + five minutes.
This descriptor entry(size=24000) was not updated when "Reload".
  -  "Fetch count:" is always incremented but "Last Modofied:" and "Expired:"
     were never updated(stayed same value on last "Shift+Reload").

Questions on patch(attachment 123325 [details] [diff] [review]).
(Q1) Above descriptor entry seems to be deleted/redefined later
     (after imgRequest::OnRedirect execution) when "Shift+Reload".
     Won't later execution of current logic when "Shift+Reload" be affected
     by "mChannel=aNewChannel;" in this OnRedirect listener?
(Q2) Is patch's "RemoveFromCache();" for this descriptor entry?
     (size=24000 entry for URL-RANDOM.php)?
     (Sorry but I couldn't find who does do it, what is done by this statement.)
Comment 60 Matthias Versen [:Matti] 2005-05-28 12:26:23 PDT
*** Bug 295810 has been marked as a duplicate of this bug. ***
Comment 61 Josh Birnbaum 2005-09-13 00:20:23 PDT
*** Bug 308252 has been marked as a duplicate of this bug. ***
Comment 62 Josh Birnbaum 2005-09-13 00:35:13 PDT
*** Bug 265052 has been marked as a duplicate of this bug. ***
Comment 63 Elmar Ludwig 2005-09-14 01:44:53 PDT
*** Bug 308296 has been marked as a duplicate of this bug. ***
Comment 64 Darin Fisher 2005-09-23 16:10:43 PDT
*** Bug 257727 has been marked as a duplicate of this bug. ***
Comment 65 Asa Dotzler [:asa] 2005-10-10 16:10:49 PDT
Comment on attachment 123325 [details] [diff] [review]
New patch that addresses previous comments

darin, how safe is this patch and do you think it's really critical to get this
into the release given how old the bug is?
Comment 66 Darin Fisher 2005-10-10 16:15:15 PDT
The patch has some risk associated with it, and may even be out of date.  Notice
that the patch is from 2003!

This bug seems to show up rather frequently, however, which is why I put it back
on the radar for 1.8rc1.  Unfortunately, however, I think it is probably a top
candidate for the chopping block :-/
Comment 67 Christian :Biesinger (don't email me, ping me on IRC) 2005-10-10 16:31:25 PDT
the patch should be changed to use nsIChannelEventSink...
Comment 68 Asa Dotzler [:asa] 2005-10-10 17:37:46 PDT
thanks for the quick feedback darin.
Comment 69 Matthew Cheale 2007-01-15 01:09:52 PST
No offence but when's this five and half year old major bug going to be fixed?  This is exactly what we wish to do to cut down the bandwidth use and image load speed, through cached image usage, on our site
Comment 70 Matthew Cheale 2007-02-12 01:23:22 PST
I got mailed suggesting I setup a test case and help with #67.  I can't help with #67, I'm no application dev, but I can do a test case:

The test case:
http://sayten.34sp.com.local/moztests/89419/

The source:
http://sayten.34sp.com.local/moztests/89419/index.phps
http://sayten.34sp.com.local/moztests/89419/image.phps

Ignore the warnings, it's only a test case.  On subsequent loads or clicks of click me a second image shows what the last image was supposed to be and the time image.php was called for you session.  So basically you'll see the top image never change and if the last top is not the same as the current bottom that shows that it's being displayed from cache and not the actual 302 location.  Otherwise if they're both not changing and the time isn't changing that shows that the browser isn't even sending a GET request to the image.php script, it's just loading it from it's cache.
Comment 71 Darin Fisher 2007-02-12 08:49:37 PST
"patches welcome"
Comment 72 info665-s1 2007-02-22 16:21:10 PST
> No offence but when's this five and half year old major bug going to be fixed?

This proves just that - that a major bug can't be fixed for 5 years! This totally discourages new bugs reporting as it seems no one cares. By the way there were some other bugs reports for the same problem marked as duplicates - some were even announced as solved but actually the problem remains. Obviously the right approach and reasons for the problem has not been found. I gave up about two years ago.
Comment 73 russell-bugzilla 2007-02-23 04:11:36 PST
line848: It could be fixed: you could fix it, or you could pay someone to fix it. If you can't put your money where your mouth is, perhaps you should put a sock there instead.

I hope this and your comment will be removed as they are both irrelevant, since this applies to all bugs of all ages.
Comment 74 Joe Drew (not getting mail) 2008-10-14 21:28:34 PDT
I am pretty sure that this is what is causing bug 458845.
Comment 75 Joe Drew (not getting mail) 2008-10-15 21:50:14 PDT
Crispin's URL no longer works, so I've put it up on my server.
Comment 76 Joe Drew (not getting mail) 2008-10-20 19:37:14 PDT
Created attachment 344006 [details] [diff] [review]
Un-bitrot jdunn's patch, and address bug 458845 at the same time

This is an un-bitrotted patch that rolls Jim Dunn's changes and the changes necessary to fix bug 458845 into one go that fixes imglib's redirect problems.

I haven't yet tested it extensively, but I will do so tomorrow. If people are interested, I will also create tryserver builds, but I suspect that nobody is champing at the bit for this bug to be fixed, considering it's more than 7 years old.
Comment 77 Joe Drew (not getting mail) 2008-10-20 20:03:47 PDT
Created attachment 344012 [details] [diff] [review]
how about we don't mash two patches together

Well, that was kind of dumb. I mashed two patches together by accident. Versioned mercurial queues saved me, though!
Comment 78 Hrishikesh Barua 2008-10-20 20:55:25 PDT
(In reply to comment #76)
> Created an attachment (id=344006) [details]
> Un-bitrot jdunn's patch, and address bug 458845 at the same time
> 
> This is an un-bitrotted patch that rolls Jim Dunn's changes and the changes
> necessary to fix bug 458845 into one go that fixes imglib's redirect problems.
> 
> I haven't yet tested it extensively, but I will do so tomorrow. If people are
> interested, I will also create tryserver builds, but I suspect that nobody is
> champing at the bit for this bug to be fixed, considering it's more than 7
> years old.

Hi Joe, I am assuming that this bug is present in Firefox 3 as well. We've had to have numerous workarounds in our apps because of this bug - am sure there are a lot of people who would like to see your fix go in. Keep up the great work. Better late than never :)
Comment 79 Boris Zbarsky [:bz] (still a bit busy) 2008-10-21 06:14:42 PDT
Joe, note that this patch makes the imgIRequest API documentation false (since the image now _is_ the post-redirect one).  That's probably OK as long as we audit all consumers of that API function.  Having such a list would be a good start on that...
Comment 80 Samuel Sidler (old account; do not CC) 2008-10-24 11:26:40 PDT
Comment on attachment 344012 [details] [diff] [review]
how about we don't mash two patches together

Vlad, can you review this or find an expedient reviewer for it?
Comment 81 Vladimir Vukicevic [:vlad] [:vladv] 2008-10-24 14:10:08 PDT
Comment on attachment 344012 [details] [diff] [review]
how about we don't mash two patches together

This looks fine to me, but stuart should glance at it.
Comment 82 Boris Zbarsky [:bz] (still a bit busy) 2008-10-24 14:14:58 PDT
Uh... how can it possibly be fine given comment 79?  At the very least, the API documentation needs changing.
Comment 83 Vladimir Vukicevic [:vlad] [:vladv] 2008-10-24 16:19:11 PDT
(In reply to comment #82)
> Uh... how can it possibly be fine given comment 79?  At the very least, the API
> documentation needs changing.

I mean that the code as changed looks fine to me, but as I told Sam, I don't know the imglib code in detail, hence wanting stuart to also look at it.  Your comments should also be addressed as well.
Comment 84 Joe Drew (not getting mail) 2008-10-24 16:50:45 PDT
Sam, I hadn't asked for r or sr because I hadn't finished the legwork on the patch yet. There's no use doing that if things need to be changed in such a way that reviews will need to be redone.
Comment 85 Joe Drew (not getting mail) 2008-10-27 17:31:06 PDT
Here is a list of files that touch imgIRequest that also contain the string "GetURI":

/accessible/src/html/nsHTMLImageAccessible.cpp
/content/base/src/nsContentAreaDragDrop.cpp
/content/base/src/nsContentUtils.cpp
/content/base/src/nsImageLoadingContent.cpp
/content/canvas/src/nsCanvasRenderingContext2D.cpp
/content/html/document/src/nsImageDocument.cpp
/embedding/browser/webBrowser/nsContextMenuInfo.cpp
/layout/generic/nsBulletFrame.cpp
/layout/generic/nsImageFrame.cpp
/layout/style/nsComputedDOMStyle.cpp
/layout/style/nsStyleContext.cpp
/layout/style/nsStyleStruct.cpp
/layout/xul/base/src/nsImageBoxFrame.cpp
/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp
/modules/libpr0n/src/imgLoader.cpp
/modules/libpr0n/src/imgRequest.cpp
/modules/libpr0n/src/imgRequestProxy.cpp
/security/manager/boot/src/nsSecureBrowserUIImpl.cpp

I'm inspecting these for anything that could be affected by this change.
Comment 86 Joe Drew (not getting mail) 2008-10-27 17:56:38 PDT
nsImageLoadingContent returns the current request's URI for GetCurrentURI(). I doubt this will cause a problem, but it has the same fallout as GetURI() changing.

nsBulletFrame::DidSetStyleContext() might do a reload in cases where it wouldn't have before, but this is likely more correct.

nsComputedDOMStyle::GetContent() will now report the image's redirected URL. I don't know if this is bad or good.

Various things in nsStyleStruct.cpp will now return that images are different. Similarly for nsImageBoxFrame, nsTreeBodyFrame. These are all likely what we want.

Does anyone have any comments or thoughts about these differences?
Comment 87 Boris Zbarsky [:bz] (still a bit busy) 2008-10-27 22:03:08 PDT
Note that there are certainly JS consumers of nsIImageLoadingContent.  There might be some of imgIRequest as well.

> nsComputedDOMStyle::GetContent() will now report the image's redirected URL. I
> don't know if this is bad or good.

This is bad, imo.
Comment 88 Boris Zbarsky [:bz] (still a bit busy) 2008-11-03 20:36:38 PST
If we want to store the channel and we're listening for redirects anyway,
should we just stop doing the loadgroup thing we're doing now?  I thought we
only did that so that we wouldn't have to listen for redirects...
Comment 89 Bradley Baetz (:bbaetz) 2008-11-03 23:38:14 PST
(In reply to comment #88)
> If we want to store the channel and we're listening for redirects anyway,
> should we just stop doing the loadgroup thing we're doing now?  I thought we
> only did that so that we wouldn't have to listen for redirects...

We have to set the same load group on the new image request so that pressing stop in the browser can stop the new image request as well.

Or at least that was the case 6 years ago; the docshell behaviour has probably changed since then.
Comment 90 Boris Zbarsky [:bz] (still a bit busy) 2008-11-04 07:25:30 PST
The new image request gets the same loadgroup as the old one automatically; that's not an issue.  The loadgroup thing I'm talking about is a 1.9 change that makes the mRequest that imgRequest holds a loadgroup containing the HTTP channel, not the HTTP channel itself.  See the CVS log for imgRequest.
Comment 91 Al Billings [:abillings] 2008-11-04 13:24:39 PST
Verified for 1.8.1.18 using the one working testcase in the list of them here (http://capricorn.woot.net/~jdrew/bugs/mozilla89419/index.html) with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.18) Gecko/2008102918 Firefox/2.0.0.18.
Comment 92 Joe Drew (not getting mail) 2008-11-21 14:27:16 PST
Created attachment 349493 [details] [diff] [review]
Only change the cache key, and proxy notification callbacks

Rather than change the API of imglib, which isn't necessary, let's just change the key that images are cached at. This fixes the bug without introducing unnecessary churn.

Also, this patch proxies events to any previous notification callbacks on the channel.

Boris, I've filed bug 466230 as a follow-up to this one to remove the loadgroup handling imgRequest does.

Before this lands, I will need to come up with tests for the bug.
Comment 93 Joe Drew (not getting mail) 2008-11-21 14:38:33 PST
Created attachment 349497 [details] [diff] [review]
Fix a major bug in the previous patch

Somehow I missed that the previous patch didn't actually fix this bug. This patch fixes that.
Comment 94 Boris Zbarsky [:bz] (still a bit busy) 2008-11-26 06:59:00 PST
Comment on attachment 349497 [details] [diff] [review]
Fix a major bug in the previous patch

This needs updating to fix the issues found when landing bug 355126 on branch.
Comment 95 Boris Zbarsky [:bz] (still a bit busy) 2008-11-26 07:00:57 PST
At this point this is a regression from 1.8.1.x and 1.9.0.x.
Comment 96 Al Billings [:abillings] 2008-11-26 11:28:25 PST
Is this a regression only on 1.9.0.x? I just checked this again on OS X with 1.8.1.18 using http://capricorn.woot.net/~jdrew/bugs/mozilla89419/index.html and it looks fixed there.
Comment 97 Samuel Sidler (old account; do not CC) 2008-11-26 11:39:47 PST
Al, we're landing this on 1.9.0 in bug 355126, along with new improvements to the 1.8.1 branch there as well.
Comment 98 Boris Zbarsky [:bz] (still a bit busy) 2008-11-26 12:01:58 PST
Al, this is a regression on trunk.
Comment 99 Joe Drew (not getting mail) 2008-11-26 17:43:11 PST
For all those following along, the 1.8.1 and 1.9.0 branch work for this bug happened in bug 355126. (Firefox 2.0.0.18 and later contains the fix for this bug; Firefox 3.0.5 will also have the fix for this bug.)

We still haven't finished the work for Firefox 3.1 (Gecko 1.9.1) yet, though, which is why this bug remains open.
Comment 100 Joe Drew (not getting mail) 2008-11-27 13:40:53 PST
Created attachment 350387 [details] [diff] [review]
Fix reftest failures found in bug 355126

This is equivalent to what landed on 1.9.0 in bug 355126.
Comment 101 Boris Zbarsky [:bz] (still a bit busy) 2008-12-01 16:40:17 PST
Comment on attachment 350387 [details] [diff] [review]
Fix reftest failures found in bug 355126

>+++ b/modules/libpr0n/src/imgRequest.cpp
>+imgRequest::GetInterface(const nsIID & aIID, void **aResult)
>+  if (!mPrevChannelSink || aIID.Equals(NS_GET_IID(nsIChannelEventSink)))
>+    return QueryInterface(aIID, aResult);
>+  else {

No need for else after return.

>+imgRequest::OnChannelRedirect(nsIChannel *oldChannel, nsIChannel *newChannel, PRUint32 flags)
>+  nsCOMPtr<nsIURI> uri;
>+  newChannel->GetURI(getter_AddRefs(uri));

On trunk (and only on trunk) you might want GetOriginalURI here.  That won't affect HTTP, but for something like about: or chrome:// there's a difference. For example, when redirecting to a chrome:// URI the originalURI will be the chrome:// URI while the URI returned by GetURI will be a jar:file:// URI.  Not sure which one you want here.

r=bzbarsky with the nits picked.
Comment 102 Al Billings [:abillings] 2008-12-02 09:58:57 PST
Verified for 1.9.0.5 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.5pre) Gecko/2008120105 GranParadiso/3.0.5pre. This is fixed now.
Comment 103 Tomas 2008-12-06 12:52:46 PST
Status is steel new. It is fixed?
Comment 104 Joe Drew (not getting mail) 2008-12-06 13:14:00 PST
Tomas, please see comment 99.
Comment 105 Vladimir Vukicevic [:vlad] [:vladv] 2008-12-09 13:39:57 PST
Comment on attachment 350387 [details] [diff] [review]
Fix reftest failures found in bug 355126

sr=me
Comment 106 Joe Drew (not getting mail) 2008-12-10 16:29:01 PST
Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/rev/74a85a545f0a

After some baking in m-c, will push to 1.9.1.
Comment 107 Robert Sayre 2008-12-11 01:31:05 PST
had to back this out due to leaks on tinderbox
Comment 108 Joe Drew (not getting mail) 2008-12-18 15:57:18 PST
Created attachment 353744 [details] [diff] [review]
Fix leaks caused by cycle

Normally, attachment 350387 [details] [diff] [review] worked fine. However, in the case where a channel failed to open - as was the case in the wyciwyg channels that appear in the docshell mochitests for bug 270414 - we would never break the cycle between the channel and ourselves. This attachment fixes that.
Comment 109 Joe Drew (not getting mail) 2008-12-18 16:12:52 PST
Forgot one other thing about this new patch - we cannot call Cancel on the load group inside of imgLoader::Cancel(), because the channel might not have added itself to the load group if it failed to open. This will be more completely dealt with in bug 466230, but for now, we just call Cancel on the channel itself.
Comment 110 Boris Zbarsky [:bz] (still a bit busy) 2008-12-18 18:02:02 PST
So the issue here is that this code is actually running before asyncOpen is called on mChannel?
Comment 111 Joe Drew (not getting mail) 2008-12-18 18:50:59 PST
No - asyncOpen is called, but fails. imgLoader then calls Cancel on the imgRequest because it can't progress any further.
Comment 112 Boris Zbarsky [:bz] (still a bit busy) 2008-12-19 15:00:00 PST
Comment on attachment 353744 [details] [diff] [review]
Fix leaks caused by cycle

Ah, I see.  I'd prefer that the caller used a different call to signal the asyncOpen failure.  this call could call Cancel() and then drop the channel (no need for the death-grip in that case).  Should be good with a public imgRequest method, since that's what the caller has, right?

r=bzbarsky with that change.
Comment 113 Joe Drew (not getting mail) 2008-12-22 14:22:24 PST
Made those changes and pushed in http://hg.mozilla.org/mozilla-central/rev/f309ea52af68

After some baking, will push to 1.9.1.
Comment 114 Joe Drew (not getting mail) 2009-01-05 15:44:50 PST
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3327b0298d1b
Comment 115 Boris Zbarsky [:bz] (still a bit busy) 2009-01-12 06:47:52 PST
OK, the "fix leaks caused by cycle" patch is wrong.  In particular, it regresses bug 373701, because it's canceling the wrong thing.  I filed bug 473161 on this.
Comment 116 Henrik Skupin (:whimboo) 2009-02-04 16:22:30 PST
Verified on trunk and 1.9.1 with the following builds on OS X and Vista:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090204 Shiretoko/3.1b3pre ID:20090204020327

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090203 Minefield/3.2a1pre (.NET CLR 3.5.30729) ID:20090203020449

Is an automated testcase possible for this regression?
Comment 117 Joe Drew (not getting mail) 2009-02-06 09:18:13 PST
Yes - that's bug 470838. Already written, waiting for review/checkin.

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