Closed Bug 89419 Opened 23 years ago Closed 16 years ago

[PATCH] Caching of images loaded from a 302 is broken

Categories

(Core :: Graphics: ImageLib, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: crispin, Assigned: joe)

References

(Blocks 3 open bugs, )

Details

(4 keywords)

Attachments

(2 files, 9 obsolete files)

10.72 KB, text/plain
Details
12.77 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
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.
Chances are, the URL being redirected to does not send any no-cache headers....

confirming bug.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
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.
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).
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...
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...
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)
Darin, this looks a bit more like HTTP than Cache.
Assignee: gordon → neeti
Component: Networking: Cache → Networking: HTTP
QA Contact: tever → benc
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?
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.
-> darin
actually, after poking at the imglib cache stuff last night, -> imagelib. Its
using the initial url as the thing to cache.
Assignee: neeti → pavlov
Component: Networking: HTTP → ImageLib
QA Contact: benc → tpreston
*** Bug 101502 has been marked as a duplicate of this bug. ***
Blocks: 48202
Blocks: 68423
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.

Severity: normal → major
OS: Linux → All
Hardware: PC → All
See also bug 69486, Cannot block images which have redirects.
Target Milestone: --- → Future
Nominating sooner than to Future because it's non conformant behaviour to the
HTTP RFC.

Keywords: mozilla1.0
taking from pavlov
Assignee: pavlov → nivedita
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
added for the comment 70884
> 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.
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.
giving it back to pavlov, as he has a fix for it, as per the comment 21  from 
Darin.
Assignee: nivedita → pavlov
Blocks: 158715
Keywords: mozilla1.0mozilla1.2
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.  
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.
Blocks: Heidi
"mass" re-assigning of bugs from pav to myself
Assignee: pavlov → jdunn
updating target milestone
Target Milestone: Future → mozilla1.4alpha
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)
Status: NEW → ASSIGNED
Target Milestone: mozilla1.4alpha → mozilla1.4beta
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.
Hmm, sorry I rearranged my websites and test cases, the testcase is now at:

http://bugs.flowerday.cx/mozilla89419/
Attached patch a suggested patch (obsolete) — Splinter Review
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
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)
radha and adam are probably the ones most familiar with this code....
Attached patch Handle OnRedirect requests (obsolete) — Splinter Review
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.
Attachment #70884 - Attachment is obsolete: true
Attachment #119285 - Attachment is obsolete: true
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).
Attachment #122695 - Flags: review-
Please don't use macros that have returns in them in imagelib code.  (i.e.
things like NS_ENSURE_ARG_POINTER())
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.
Attachment #122695 - Attachment is obsolete: true
>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.
Attachment #123325 - Flags: superreview?(pavlov)
Attachment #123325 - Flags: review?(darin)
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.
Attachment #123325 - Flags: superreview?(pavlov)
Attachment #123325 - Flags: superreview+
Attachment #123325 - Flags: review?(pavlov)
Attachment #123325 - Flags: review?(darin)
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?
Keywords: 4xp
Hermann: The patch does not have review yet.
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.
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.
Assignee: jdunn → darin
Status: ASSIGNED → NEW
Whiteboard: [patch needs r=]
Target Milestone: mozilla1.4beta → mozilla1.6beta
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!
*** Bug 224282 has been marked as a duplicate of this bug. ***
*** Bug 168089 has been marked as a duplicate of this bug. ***
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!
Attachment #123325 - Flags: review?(pavlov) → review?(tor)
Attachment #123325 - Flags: review?(tor) → review+
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
Status: NEW → ASSIGNED
Target Milestone: mozilla1.6beta → mozilla1.7alpha
Blocks: 229839
*** Bug 229839 has been marked as a duplicate of this bug. ***
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?
Priority: -- → P2
Target Milestone: mozilla1.7alpha → mozilla1.7beta
Blocks: 223142
Target Milestone: mozilla1.7beta → mozilla1.8alpha
Target Milestone: mozilla1.8alpha1 → mozilla1.8beta
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.
Blocks: 265052
(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.

(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.  
I've opend Bug 282198 for problem of comment #50 (redirect of IFRAME's URL case). 
*** Bug 281486 has been marked as a duplicate of this bug. ***
*** Bug 281754 has been marked as a duplicate of this bug. ***
(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".
(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

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.
(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.)
*** Bug 295810 has been marked as a duplicate of this bug. ***
Flags: blocking1.8b3?
Flags: blocking1.8b3? → blocking1.8b3-
*** Bug 308252 has been marked as a duplicate of this bug. ***
No longer blocks: 265052
*** Bug 265052 has been marked as a duplicate of this bug. ***
*** Bug 308296 has been marked as a duplicate of this bug. ***
Blocks: 121518
*** Bug 257727 has been marked as a duplicate of this bug. ***
Target Milestone: mozilla1.8beta1 → mozilla1.8beta5
Flags: blocking1.8rc1?
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?
Attachment #123325 - Flags: approval1.8rc1?
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 :-/
the patch should be changed to use nsIChannelEventSink...
Attachment #123325 - Flags: approval1.8rc1?
thanks for the quick feedback darin.
Flags: blocking1.8rc1? → blocking1.8rc1-
Target Milestone: mozilla1.8beta5 → mozilla1.9alpha
Priority: P2 → P1
Depends on: 328903
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
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.
"patches welcome"
Assignee: darin.moz → nobody
Status: ASSIGNED → NEW
QA Contact: tpreston → imagelib
> 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.
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.
I am pretty sure that this is what is causing bug 458845.
Assignee: nobody → joe
Crispin's URL no longer works, so I've put it up on my server.
Blocks: 458845
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.
Attachment #123325 - Attachment is obsolete: true
Well, that was kind of dumb. I mashed two patches together by accident. Versioned mercurial queues saved me, though!
Attachment #344006 - Attachment is obsolete: true
(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 :)
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...
Whiteboard: [patch needs r=]
Attachment #344012 - Flags: review?(vladimir)
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?
Attachment #344012 - Flags: superreview?(pavlov)
Attachment #344012 - Flags: review?(vladimir)
Attachment #344012 - Flags: review+
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.
Uh... how can it possibly be fine given comment 79?  At the very least, the API documentation needs changing.
(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.
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.
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.
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?
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.
Keywords: fixed1.8.1.18
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...
(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.
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.
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.
Blocks: 466230
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.
Attachment #344012 - Attachment is obsolete: true
Attachment #344012 - Flags: superreview?(pavlov)
Attachment #349493 - Flags: superreview?(pavlov)
Attachment #349493 - Flags: review?(bzbarsky)
Somehow I missed that the previous patch didn't actually fix this bug. This patch fixes that.
Attachment #349493 - Attachment is obsolete: true
Attachment #349493 - Flags: superreview?(pavlov)
Attachment #349493 - Flags: review?(bzbarsky)
Attachment #349497 - Flags: superreview?(pavlov)
Attachment #349497 - Flags: review?(bzbarsky)
Keywords: fixed1.9.0.5
Attachment #349497 - Flags: review?(bzbarsky) → review-
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.
At this point this is a regression from 1.8.1.x and 1.9.0.x.
Flags: blocking1.9.1?
Keywords: regression
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.
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.
Al, this is a regression on trunk.
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.
This is equivalent to what landed on 1.9.0 in bug 355126.
Attachment #349497 - Attachment is obsolete: true
Attachment #349497 - Flags: superreview?(pavlov)
Attachment #350387 - Flags: superreview?(pavlov)
Attachment #350387 - Flags: review?(bzbarsky)
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.
Attachment #350387 - Flags: review?(bzbarsky) → review+
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.
Flags: blocking1.9.1? → blocking1.9.1+
Target Milestone: mozilla1.9alpha1 → mozilla1.9.1b3
Status is steel new. It is fixed?
Tomas, please see comment 99.
Summary: Caching of images loaded from a 302 is broken → [PATCH] Caching of images loaded from a 302 is broken
Comment on attachment 350387 [details] [diff] [review]
Fix reftest failures found in bug 355126

sr=me
Attachment #350387 - Flags: superreview?(pavlov) → superreview+
Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/rev/74a85a545f0a

After some baking in m-c, will push to 1.9.1.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
had to back this out due to leaks on tinderbox
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #350387 - Attachment is obsolete: true
Attachment #353744 - Flags: superreview?(vladimir)
Attachment #353744 - Flags: review?(bzbarsky)
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.
Attachment #353744 - Flags: superreview?(vladimir) → superreview+
So the issue here is that this code is actually running before asyncOpen is called on mChannel?
No - asyncOpen is called, but fails. imgLoader then calls Cancel on the imgRequest because it can't progress any further.
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.
Attachment #353744 - Flags: review?(bzbarsky) → review+
Made those changes and pushed in http://hg.mozilla.org/mozilla-central/rev/f309ea52af68

After some baking, will push to 1.9.1.
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
Blocks: 470838
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.
Depends on: 473161
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?
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Target Milestone: mozilla1.9.1b3 → mozilla1.9.2a1
Yes - that's bug 470838. Already written, waiting for review/checkin.
Depends on: 479328
You need to log in before you can comment on or make changes to this bug.