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)
Core
Graphics: ImageLib
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+
vlad
:
superreview+
|
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.
Comment 1•23 years ago
|
||
Chances are, the URL being redirected to does not send any no-cache headers.... confirming bug.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 2•23 years ago
|
||
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.
Reporter | ||
Comment 3•23 years ago
|
||
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.
Reporter | ||
Comment 4•23 years ago
|
||
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...
Comment 6•23 years ago
|
||
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...
Reporter | ||
Comment 7•23 years ago
|
||
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?
Reporter | ||
Comment 10•23 years ago
|
||
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•23 years ago
|
||
-> darin
Comment 12•23 years ago
|
||
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
Comment 13•23 years ago
|
||
*** Bug 101502 has been marked as a duplicate of this bug. ***
Comment 14•23 years ago
|
||
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
Comment 15•23 years ago
|
||
See also bug 69486, Cannot block images which have redirects.
Updated•22 years ago
|
Target Milestone: --- → Future
Comment 16•22 years ago
|
||
Nominating sooner than to Future because it's non conformant behaviour to the HTTP RFC.
Keywords: mozilla1.0
Comment 18•22 years ago
|
||
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•22 years ago
|
||
added for the comment 70884
Reporter | ||
Comment 20•22 years ago
|
||
> 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•22 years ago
|
||
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•22 years ago
|
||
giving it back to pavlov, as he has a fix for it, as per the comment 21 from Darin.
Assignee: nivedita → pavlov
Reporter | ||
Updated•22 years ago
|
Updated•22 years ago
|
Keywords: mozilla1.0 → mozilla1.2
Comment 23•21 years ago
|
||
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•21 years ago
|
||
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
Comment 27•21 years ago
|
||
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
Comment 28•21 years ago
|
||
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.
Reporter | ||
Comment 29•21 years ago
|
||
Hmm, sorry I rearranged my websites and test cases, the testcase is now at: http://bugs.flowerday.cx/mozilla89419/
Comment 30•21 years ago
|
||
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•21 years ago
|
||
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•21 years ago
|
||
radha and adam are probably the ones most familiar with this code....
Comment 33•21 years ago
|
||
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 34•21 years ago
|
||
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-
Comment 35•21 years ago
|
||
Please don't use macros that have returns in them in imagelib code. (i.e. things like NS_ENSURE_ARG_POINTER())
Comment 36•21 years ago
|
||
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
Comment 37•21 years ago
|
||
>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 38•21 years ago
|
||
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)
Comment 39•21 years ago
|
||
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
Comment 40•21 years ago
|
||
Hermann: The patch does not have review yet.
Comment 41•21 years ago
|
||
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•21 years ago
|
||
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
Comment 43•21 years ago
|
||
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•21 years ago
|
||
*** Bug 224282 has been marked as a duplicate of this bug. ***
Comment 45•21 years ago
|
||
*** Bug 168089 has been marked as a duplicate of this bug. ***
Comment 46•21 years ago
|
||
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+
Comment 47•20 years ago
|
||
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
Comment 48•20 years ago
|
||
*** Bug 229839 has been marked as a duplicate of this bug. ***
Comment 49•20 years ago
|
||
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?
Updated•20 years ago
|
Priority: -- → P2
Target Milestone: mozilla1.7alpha → mozilla1.7beta
Updated•20 years ago
|
Target Milestone: mozilla1.7beta → mozilla1.8alpha
Updated•20 years ago
|
Target Milestone: mozilla1.8alpha1 → mozilla1.8beta
Comment 50•20 years ago
|
||
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•19 years ago
|
||
(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•19 years ago
|
||
(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•19 years ago
|
||
I've opend Bug 282198 for problem of comment #50 (redirect of IFRAME's URL case).
Comment 54•19 years ago
|
||
*** Bug 281486 has been marked as a duplicate of this bug. ***
Comment 55•19 years ago
|
||
*** Bug 281754 has been marked as a duplicate of this bug. ***
Comment 56•19 years ago
|
||
(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•19 years ago
|
||
(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•19 years ago
|
||
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•19 years ago
|
||
(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•19 years ago
|
||
*** Bug 295810 has been marked as a duplicate of this bug. ***
Updated•19 years ago
|
Flags: blocking1.8b3?
Updated•19 years ago
|
Flags: blocking1.8b3? → blocking1.8b3-
Comment 61•19 years ago
|
||
*** Bug 308252 has been marked as a duplicate of this bug. ***
Comment 62•19 years ago
|
||
*** Bug 265052 has been marked as a duplicate of this bug. ***
Comment 63•19 years ago
|
||
*** Bug 308296 has been marked as a duplicate of this bug. ***
Comment 64•19 years ago
|
||
*** Bug 257727 has been marked as a duplicate of this bug. ***
Updated•19 years ago
|
Target Milestone: mozilla1.8beta1 → mozilla1.8beta5
Updated•19 years ago
|
Flags: blocking1.8rc1?
Comment 65•19 years ago
|
||
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?
Comment 66•19 years ago
|
||
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•19 years ago
|
||
the patch should be changed to use nsIChannelEventSink...
Updated•19 years ago
|
Attachment #123325 -
Flags: approval1.8rc1?
Comment 68•19 years ago
|
||
thanks for the quick feedback darin.
Flags: blocking1.8rc1? → blocking1.8rc1-
Updated•19 years ago
|
Target Milestone: mozilla1.8beta5 → mozilla1.9alpha
Updated•18 years ago
|
Priority: P2 → P1
Comment 69•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
||
"patches welcome"
Assignee: darin.moz → nobody
Status: ASSIGNED → NEW
QA Contact: tpreston → imagelib
Comment 72•17 years ago
|
||
> 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•17 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.
Assignee | ||
Comment 74•16 years ago
|
||
I am pretty sure that this is what is causing bug 458845.
Assignee: nobody → joe
Assignee | ||
Comment 75•16 years ago
|
||
Crispin's URL no longer works, so I've put it up on my server.
Assignee | ||
Comment 76•16 years ago
|
||
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
Assignee | ||
Comment 77•16 years ago
|
||
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
Comment 78•16 years ago
|
||
(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 :)
Updated•16 years ago
|
Blocks: CVE-2008-5012
Comment 79•16 years ago
|
||
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...
Updated•16 years ago
|
Whiteboard: [patch needs r=]
Updated•16 years ago
|
Attachment #344012 -
Flags: review?(vladimir)
Comment 80•16 years ago
|
||
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.
Comment 82•16 years ago
|
||
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.
Assignee | ||
Comment 84•16 years ago
|
||
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.
Assignee | ||
Comment 85•16 years ago
|
||
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.
Assignee | ||
Comment 86•16 years ago
|
||
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•16 years ago
|
||
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.
Assignee | ||
Updated•16 years ago
|
Keywords: fixed1.8.1.18
Comment 88•16 years ago
|
||
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•16 years ago
|
||
(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•16 years ago
|
||
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•16 years ago
|
||
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.
Keywords: fixed1.8.1.18 → verified1.8.1.18
Assignee | ||
Comment 92•16 years ago
|
||
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)
Assignee | ||
Comment 93•16 years ago
|
||
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)
Assignee | ||
Updated•16 years ago
|
Keywords: fixed1.9.0.5
Updated•16 years ago
|
Attachment #349497 -
Flags: review?(bzbarsky) → review-
Comment 94•16 years ago
|
||
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•16 years ago
|
||
At this point this is a regression from 1.8.1.x and 1.9.0.x.
Flags: blocking1.9.1?
Updated•16 years ago
|
Keywords: regression
Comment 96•16 years ago
|
||
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•16 years ago
|
||
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•16 years ago
|
||
Al, this is a regression on trunk.
Assignee | ||
Comment 99•16 years ago
|
||
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.
Assignee | ||
Comment 100•16 years ago
|
||
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 101•16 years ago
|
||
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+
Comment 102•16 years ago
|
||
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.
Keywords: fixed1.9.0.5 → verified1.9.0.5
Flags: blocking1.9.1? → blocking1.9.1+
Target Milestone: mozilla1.9alpha1 → mozilla1.9.1b3
Comment 103•16 years ago
|
||
Status is steel new. It is fixed?
Assignee | ||
Comment 104•16 years ago
|
||
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+
Assignee | ||
Comment 106•16 years ago
|
||
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
Comment 107•16 years ago
|
||
had to back this out due to leaks on tinderbox
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 108•16 years ago
|
||
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)
Assignee | ||
Comment 109•16 years ago
|
||
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+
Comment 110•16 years ago
|
||
So the issue here is that this code is actually running before asyncOpen is called on mChannel?
Assignee | ||
Comment 111•16 years ago
|
||
No - asyncOpen is called, but fails. imgLoader then calls Cancel on the imgRequest because it can't progress any further.
Comment 112•16 years ago
|
||
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+
Assignee | ||
Comment 113•16 years ago
|
||
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 ago → 16 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 114•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3327b0298d1b
Keywords: fixed1.9.1
Comment 115•15 years ago
|
||
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
Comment 116•15 years ago
|
||
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?
Keywords: fixed1.9.1 → verified1.9.1
Target Milestone: mozilla1.9.1b3 → mozilla1.9.2a1
Assignee | ||
Comment 117•15 years ago
|
||
Yes - that's bug 470838. Already written, waiting for review/checkin.
You need to log in
before you can comment on or make changes to this bug.
Description
•