User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2) Gecko/20100115 Firefox/3.6 GTB6 (.NET CLR 3.5.30729) Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2) Gecko/20100115 Firefox/3.6 GTB6 (.NET CLR 3.5.30729) Dear team, We are in the ad technology department at Yahoo!, we noticed that FF3.6 have a misbehavior when loading a image tag with redirect url inside. The misbehaviors will cause loading the redirect url twice e.g when FF3.6 load the page contains this tag. <img src="http://ad-apac.doubleclick.net/imp;v1;f;221775763;0-0;0;45396802;1%7c1;35207401%7c35225219%7c1;;cs=r%3fhttp://ad.hk.doubleclick.net/dot.gif?%times%" width=1 height=1 border=0> FF3.6 will call the url "ad-apac.doubleclick.net/im....." twice. the impact is double count those ads impression when loading the tracking beacon twice. As the same issue does not occur at previous Firefox version, Please help to investigate and reply ASAP. Please feel free to contact kylec@yahoo-inc.com Thanks Kyle Reproducible: Always Steps to Reproduce: 1. open http parser tools like fiddler or firebug Net to sniff the http calls 2. go to the page: http://hk.sports.yahoo.com/test/clone/index.html 3. Actual Results: you will find the redirect pixel for "ad-apac.doubleclick.net/im....." will call twice Expected Results: the redirect pixel should call one times per page view This issue will cause a huge discrepancy for counting impression on ads banner. There are no such issue in the version before FF3.6

here is another example happened at UK yahoo site here: http://uk.dir.yahoo.com/Computers_and_Internet/Communications_and_Networking/ the redirect pixel appended at the banner is as <img src="http://ad-apac.doubleclick.net/imp;v1;f;221775763;0-0;0;45396802;1%7c1;35207401%7c35225219%7c1;;cs=r%3fhttp://ad.hk.doubleclick.net/dot.gif?%5btimestamp%5d" width=1 height=1 border=0> the redirec pixel url ""http://ad-apac.doubleclick.net/imp;v1;f..." is double call as the screencap

Not a security-sensitive bug.

Hi teams, any update on the bugs? because this may cause huge discrepancy on ads impression counting. Looking forward to your prompt reply cheers Kyle

Just to be clear, are you saying that this is a change in behavior that you're seeing in Firefox 3.6 vs. Firefox 3.5? If so, a regression range would be most helpful to pinpointing the problem. You could start by finding regression range using binary search amongst 3.6 alphas/betas. We can pinpoint further using nightlies once that's done. Alphas/betas are here: http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/namoroka/ http://releases.mozilla.org/pub/mozilla.org/firefox/releases/3.6b1/ http://releases.mozilla.org/pub/mozilla.org/firefox/releases/3.6b2/ http://releases.mozilla.org/pub/mozilla.org/firefox/releases/3.6b3/ http://releases.mozilla.org/pub/mozilla.org/firefox/releases/3.6b4/ http://releases.mozilla.org/pub/mozilla.org/firefox/releases/3.6b5/

I just tried the steps from comment 0 in a Mac build of Firefox 3.6 while logging the HTTP requests Firefox makes and I cannot reproduce the bug. I see us sending exactly one HTTP GET for that GIF. The url from comment 1 doesn't seem to have an ad-apac gif on it at all (at least not just now when I loaded it). Kyle, what do I have to do to reproduce this bug?

Hi Gavin, Thank you for your information. But i am not quite sure how to make use of the code in the ftp server. Do i need to download them one by one and installed it separately for testing? Really appreciate if you can us more suggestion cheers Kyle

> Do i need to download them one by one and installed it separately for testing? Yes. Or give me a way to reproduce the bug, of course...

Attach is the fiddler log when loading the page http://hk.sports.yahoo.com/test/clone/index.html at FF3.6 u can noticed the double count issue for this url http://ad-apac.doubleclick.net/imp;v1;f;221775763;0-0;0;45396802;1%7c1;35207401%7c35225219%7c1;;cs=r%3fhttp://ad.hk.doubleclick.net/dot.gif?1269239748 Btw, please suggest which http request logging tools are you using? thanks Kyle

screencap of the html request logs, FYI, kyle

Hi Boris, To reproduce the doublecount issue, please follow comment 8. thanks in advance for your prompt reply cheers kyle

> Btw, please suggest which http request logging tools are you using? https://developer.mozilla.org/en/HTTP_Logging which is basically just printfs in the Mozilla HTTP code that can be used to see exactly what it's doing. > To reproduce the doublecount issue, please follow comment 8. As I said in comment 5, loading the url listed in comment 8 (which is the same as the url in comment 0) does NOT show the issue for me in 3.6 on Mac. Is the issue Windows-specific? I'll try Windows 7 tomorrow.

Can't reproduce the problem on Windows 7 either. Just to make sure... you're testing in safe mode, or at least with all add-ons disabled?

my testing is in windowsXP

Yes, I gathered that from the OS field on this bug. I would be very surprised if this is affected in any way by WinXP vs Win7. Much less surprising would be some extension you have installed affecting results, or a timing issue that causes us to see different request patterns in the face of resource prefetch or some such.

Hi Boris, it may be the timing issue make you cannot reproduce the issue. Now i have setup an exclusive testing ads. please access the url again to reproduce the duplicate call issue http://hk.sports.yahoo.com/test/clone/index.html cheers Kyle

Hi boris, Now i have remove all the firefox addon and update to Firefox3.6.2. But the issue still occurred. i have logged the http request by the addon -live HTTP headers. Here attached the logs for your reference. Please help to reproduce the problem again and check on the issue cheers Kyle

Ok. With the page in comment 15, I can reproduce the issue, but only if I start with a clean profile that has never loaded the page before. Once it's been loaded once, the issue doesn't happen. Looking into it.

Alright. As expected, the first load is the image preload. The second load is the actual image load. For the second load, the HTTP 302 response is cached, but there is no Expires header or Max-Age header and there _is_ a query part to the url, so RFC 2616 section 13.9 applies and we do an If-Modified-Since GET request to make sure our cached redirect is still valid. The new profile is probably needed to make sure the scripts and such are not cached. That's the timing dependency I mentioned. Using a no-cache/no-store script which is very slow to respond should make this reproducible every time. So it looks like the real issue is that loadImage is not coalescing the preload and the real load. As I recall, this is because the image cache stores things based on the final (post-redirect) URI, so we get a cache miss on the load of the pre-redirect URI. Joe, that seems somewhat suboptimal. Can we fix that? I should note, though, that GET requests are supposed to be idempotent. Proxies and various other things (page info, view-source, etc) can repeat them as needed. So relying on counts of GET requests for something is pretty broken. The count has little to do with what users actually see.

Hi Boris, thanks for your investigation, Would you mind please follow up with joe when will be the fix done? any ETA? Appreciate to your support! cheers kyle

No idea. This doesn't seem like a high-priority problem compared to the other things Joe's working on, as far as I can tell... It's not a spec violation on our part; mostly a performance bug. From my point of view, of course. I understand the problems it's causing you guys, but those seem to be largely based on faulty assumptions about GET requests as far as I can tell.

It also doesn't help that imagelib is perennially understaffed, of course.

So will there be any plan for fixing the performance bug? or can you suggest a workaround solution to prevent double loading a redirect-URI? thanks kyle

Kyle, please stop modifying fields in this bug. I assume you are getting a "mid-air" screen showing you the fields changed. Please completely reload the bug before adding a comment.

> So will there be any plan for fixing the performance bug? I'd like there to be, yes. But Joe's the module owner, so it's his call. > can you suggest a workaround solution to prevent double loading a > redirect-URI? Send Max-Age headers or Expires headers that would allow the redirect response to actually be cached without needing constant revalidation. Setting Max-Age to a few minutes should do the trick, I would think. Or do you reuse the same pre-redirect URI on different pages or across loads of the same page? One other thing that might help with getting this bug fixed is if you stop removing the blocking flags I set on it and moving it back out of the imagelib component (presumably due to reloading the bug without clearing your saved form state)...

You'll need to shift-reload in Firefox to ensure the flags aren't reset. This is something that I think we should fix, but I don't think we should block on it. I'm also making the call that 1.9.2 drivers won't block on it either. I'm not certain when I'll be able to work on it, but Bobby will be available to work on Imagelib stuff as of June-ish.

Thanks Boris and Joe, nice to hear that bobby will be available on the fix, please feel free to let us know the fixation timeline cheers Kyle

I have been able to mitigate this problem somewhat by using Cache-Control and Expires headers, however it seems that 3.6.x is only willing to use a cached redirect in the case when a Cookie header is NOT passed with the request. I am able to reproduce this consistently. By turning cookies off, I can see that 302s are pulled from the cache; when cookies are on, 3.6.x ignores the cache a re-requests the image redirects from the server.

If you're sending a "Vary" header that includes "cookie", then that's expected.

No Vary header is being sent with responses. My assumption is that in the absence of a Vary header, the cache assumes there is only one representation of the content returned for that URL, no matter what headers are sent along with the request.

Then the Cookie dependence is odd. Can you please file a separate bug on that and point me to a testcase that shows that issue?

Logged bug 561276 to track issue. Thanks.

This issue is negatively impacting ad servers that use pixel beacons to track impressions. When Firefox pre-fetches then re-fetches this image redirect, that causes a duplicate ad impression when the ad has only been seen once. The net effect of this is that ad servers are over-counting impressions for ad campaigns without the knowledge of publishers or advertisers that are not aware of this browser bug. Section 2 of the IAB Rich Media Measurement Guidelines states that use of redirects as beacons is an acceptable way of counting impressions post-caching; it has been implemented in this way for over a decade by most ad servers. Please let me what I can do to get this bug resolved as soon as possible. If that means dedicating a team of Google engineers, I'll see what I can do. See IAB Rich Media Measurement Guidelines here: http://www.iab.net/iab_products_and_industry_services/508676/guidelines/Rich_Media_Measurement

Boris, Joe: Any update on the ETA for the fix for this? Are we still shooting for June?

That's not really an ETA, just an "earliest possible" date. But yes, that's still the earliest possible date. :)

Note that this bug also breaks other consumers that rely on the image cache working correctly (like CSS; see bug 552538).

Given bug 552538, I'm renominating for 1.9.3 blocking.

Assigning this back to joe because he's much more familiar with the necko-facing corner of ImageLib. Note that I'll happily work on this if there comes a point when I have no more blockers and joe still does. But as long as we're arbitrarily assigning blocker owners, I think this is better suited for his queue. ;-)

hi joe, any update from your side? could you please pump up this ticket to higher priority? looking forward to your reply cheers Kyle

hi firefox team, i got reported the issue not only occur in AU, EURO team also noticed this also happening in their side. you may reference the following demo page in order to reproduce the issue http://example.admedia.yahoo.net/firefoxPrefetchBug/ we also raise the severity because of the issue coverage area is broaden cheers Kyle

Bug 89419 specifically made us change from caching pre-redirect to post-redirect URI. Is the thought that we switch back? Cache both URIs?

I think the most straightforward way to get "correct" behavior is to cache both URIs, so that the 3xx cache entry will know it was no-cache. In particular, that's the obvious way to handle the fact that the 3xx and the final 200 can have different cache metadata.

One other thing: I think the current image cache setup, where we have an object cache with HTTP-like semantics on top of it, is sort of funky. What would work better, perhaps, is a straight-on per-document object cache (with no HTTP semantics in sight), with a second cache that has HTTP semantics and maybe caches objects too....

I'm getting confused, so let's go back to first principles. Call URL-pre the URL we're requesting, URL-post the URL we're getting redirected to. 1. Should we key a cache entry on URL-pre? Why? 2. Should we key a cache entry on URL-post? Why? 3. Should those cache entries necessarily be the same? Why? In particular, I'm wondering if just going back to our pre-bug 89419 behaviour would be considered conformant. I'm presuming not.

> 1. Should we key a cache entry on URL-pre? Why? Imo, yes; this is what the page is actually requesting. But see below. > 2. Should we key a cache entry on URL-post? Why? Not necessarily, but see below. > 3. Should those cache entries necessarily be the same? Why? They can't be the same. The reason is that we're trying to pretend like we have an HTTP cache, while we actually have an object cache. An HTTP cache has to obey the HTTP headers for its cached content. The headers for URL-pre and URL-post are different. In particular, consider two cases: 1) URL-pre redirects to a different URL-post every 1 second and asks to not be cached, but each URL-post is cacheable. 2) URL-pre is cachable and always redirects to the same URL-post, but URL-post returns different content on each load and is not cacheable. Bug 89419 was more or less about case 1 above. This bug is more or less about case 2 above. The only way to handle both is to store the cache header information for URL-pre and URL-post separately ... or something. Note that there can be multiple redirects in the chain, by the way. Perhaps what we should do for redirects is to have a separate image-redirect cache or something? So basically walk the redirect chain when doing the object lookup to find the final URI to actually use for the image, but revalidate any steps that need revalidating (the way we revalidate the non-redirect URI right now in the simple case).

(Whoever gets to this first out of bholley/jrmuizel should review it.) After a lot of back & forth with Boris and Jeff on irc, we finally decided what to do here: don't change the image cache when we're redirected. We correctly revalidate images on redirect (now, at least), so the cache logic I added in bug 89419 doesn't seem to be necessary any more, and actively breaks this usecase.

This test ensures that we don't break the intent of bug 89419.

I'll write and upload a test for *this* bug tomorrow.

Comment on attachment 539921 [details] [diff] [review] test: make sure we hit the server with Cache-control: no-cache on redirects Please just use the utility functions in WindowSnapshot.js (which incidentally mean you don't have to do expensive conversion to data urls unless the test fails) instead of adding the new API? Also, I'd like a version of this test that uses iframeelem.contentWindow.location.href = iframeelem.contentWindow.location.href; instead of reload(), since reload() does extra validation in general that navigation does not do. Does that version also pass?

OK, this implements the test changes you asked for. Unfortunately, the second test you asked for (location.href = location.href) fails. I did some debugging, and this is because when we call contentWindow.reload(false), "something" passes in nsIRequest::VALIDATE_ALWAYS as aFlags on LoadImage. I think this is a side-effect of us not properly handling uncacheable channels. Right now, images loaded with Cache-control: no-cache only revalidate when the loaded entry hasn't expired, which seems wrong according to the HTTP spec.

> "something" passes in nsIRequest::VALIDATE_ALWAYS as aFlags on LoadImage. Right, that's the "does extra validation" bit. ;) Last paragraph of comment 51 sounds about right to me based on my vague fuzzy memories.

Comment on attachment 539985 [details] [diff] [review] tests v2: make sure we hit the server with Cache-control: no-cache on redirects >+ ok(correct, "We got the same images after a reload."); How about "Image should have changed after a reload."? >+ ok(correct, "We got the same images after a reload."); Similar change here. Ideally with a slightly different message so it's easy to tell which one failed. Maybe "first reload" and "second reload"? >+ ok(correct, "We got the different images instead of looping back to the first."); "Third image should match first image." Similar for the other file. And for symmetry, test_bug89419-1.html. r=me with those nits picked. Thank you for getting tests up for this stuff!

Huh. The first OnStartRequest we get for the image has mOriginalURI set to the pre-redirect URI, mURI set to the post-redirect URI, and no Cache-Control response header. IsNoCacheResponse also returns false. Ah, because apparently OnRedirectVerifyCallback is called _before_ OnStartRequest. I guess we have to listen to the cache headers there as well.

Comment on attachment 539920 [details] [diff] [review] part 0.99999: don't change the cache on redirect Looks pleasant to me.

This is one part of the change that's necessary to fix the tests I wrote earlier. Basically, it makes it so we set the cache expiry and must-revalidate bit to the union of all the redirections. This means that if any of the redirection chain is set to Cache-Control: no-cache, the entry as a whole will always be validated; similarly, expiry time will be set to the first cache entry set in the redirect chain. Boris, is this a reasonable approach?

Comment on attachment 540099 [details] [diff] [review] part 1: Set cache expiry, validation on both OnStartRequest and OnRedirectVerifyCallback In OnRedirectVerifyCallback, shouldn't you call SetCacheValidation on the pre-redirect channel, not the post-redirect one? Past that, what exactly does the cache validator do? The answer to that question determines how to handle cache information from multiple channels...

(In reply to comment #57) > In OnRedirectVerifyCallback, shouldn't you call SetCacheValidation on the > pre-redirect channel, not the post-redirect one? Uh, yes. That would explain why it doesn't work! ;) > Past that, what exactly does the cache validator do? The answer to that > question determines how to handle cache information from multiple channels... If-Modified-Since (nsICachingChannel::LOAD_ONLY_IF_MODIFIED).

So it does a single request to the cached URI (in this case that would be the original image src) with that flag, right? Then imgCacheValidator::OnStartRequest will just see whether necko has that first URI still validly in cache; necko will just call it directly if that first URI is cached. Now consider this situation. <img src="uri1"> where uri1 redirects to uri2. uri1 has a freshness lifetime of 1000s; uri2 has a lifetime of 1s. After 1s passes, if an image is loaded as uri1 we'll try to validate uri1 (and even that only if 1000s have passed), this will return that it's still cached.... and we'll never do the new load of uri2 and get the actual new image data. Unless I'm missing something?

I believe that to be the case. What should we do in that case, though? Set it to the minimum of all the lifetimes?

Setting it to the minimum is not good enough, if I read right. In the example from comment 59 you would set expiry to 1s, so after 1s you would revalidate uri1 and necko would tell you "valid". Then what? One thought is to have each cache entry store a list of (uri, expiry, must-validate) triples? And then the validator would walk the list, validating them one after the other until it either finds that all are valid (and then we're done) or one is invalid (and then we need to reload the list from that point on). Would that work?

Crap - the reason my fixes still don't fix the location.href = location.href case in general is because we don't pay attention to redirects at all in our if-modified-since validator. At least now I know how to fix it. Is it worth doing all this work for what is really an edge case?

Which is an edge case? Performance problems due to images going through redirects? Or images being cached when they shouldn't be? (Fwiw, I think doing this work is worth it for either one....)

Comment on attachment 540099 [details] [diff] [review] part 1: Set cache expiry, validation on both OnStartRequest and OnRedirectVerifyCallback Joe convinced me this was reasonable out of band.

Future patches will need to be able to call this without an associated imgRequest. Since it's just as easy to pass an imgCacheEntry as to use mCacheEntry, make this function static.

When a server tells us Cache-Control: no-cache, they don't mean "you can use it if it hasn't expired," they mean "revalidate this before using it." (At least according to my reading of http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.9.1) So, let's honour that.

This is related to a future patch. There's no reason why we should depend on the URL being available at imgCacheEntry construction time. Instead, we can handle it as part of the rest of the validation process, and simplify the constructor a little.

When we validate cache entries, we need to follow redirects and use their cache headers as part of the new cache entry. The cleanest way I could see to do this was to pre-create a replacement imgRequest and imgCacheEntry to store the cache metadata from the redirect chain, then use them if and when we need to insert our new entry into the image cache. This patch makes us pass the tests in attachment 539985 [details] [diff] [review]. It still doesn't address the Necko redirect cache requirements that Boris has mentioned in this bug.

Boris, how would a "dumb" resource loader deal with the differing cache directives you explained in comment 59? If we run into a web page in this situation, for example, how do we make sure we've gotten Necko to reload "all the way"?

> Boris, how would a "dumb" resource loader deal In the comment 59 situation, for a bog-standard necko load, we'd ask necko to load uri1. It would check the cache, find a cached version of uri1, decide whether that cached version is ok to use based on its expiration date, etc. If it's not, it would make another request for uri1 to the server. If it's ok to use, it would pull the data from the cache. Either way, it would then look at the resulting Location header (from the server or from the cache), and discover that we need to redirect to uri2. Then we will do a load of uri2. We'll look for uri2 in the cache, find a cached version, check its expiration date, etc. The point is that there is a separate cache entry in necko for every resource, and all of them have distinct expiration information. When doing a load that involves a redirect chain, every link in the chain is checked against the cache, and the cached version is used if that's ok. Hence my suggestion in comment 43.

Comment on attachment 541203 [details] [diff] [review] part 2: Change "must validate if expired" to "must validate" to match the HTTP spec This probably makes us reload no-cache images on back navigation to the page containing the image when that page is not bfcached... or do we do that even without this patch?

(In reply to comment #71) > The point is that there is a separate cache entry in necko for every > resource, and all of them have distinct expiration information. Right - totally understood. I'm just faced with writing a bunch of code for what seems like a common situation, and I'm wondering whether, for example, web pages or stylesheets need to do the same thing.

Ah. Yeah, web pages and stylesheets just use the necko cache so get things working right for free. We do have an object cache for stylesheets, but it's per-document, so doesn't affect new document loads.

OK, so the problem you're describing only comes up when we ask for nsICachingChannel::LOAD_ONLY_IF_MODIFIED, then?

Yes (but of course that's the default value, right?).

(In reply to comment #72) > Comment on attachment 541203 [details] [diff] [review] [review] > part 2: Change "must validate if expired" to "must validate" to match the > HTTP spec > > This probably makes us reload no-cache images on back navigation to the page > containing the image when that page is not bfcached... or do we do that even > without this patch? It seems (if onunload="alert('hi')" is enough to not go into the bfcache) that we don't run into this problem, either in m-c or with my patches.

(In reply to comment #76) > Yes (but of course that's the default value, right?). What do you mean by the default? If it's the default, wouldn't "simple" necko consumers automatically run into this bug?

> It seems (if onunload="alert('hi')" is enough to not go into the bfcache) You can test by seeing what the event.persisted is for the pagehide event for the page.... > What do you mean by the default? Oh, sorry. I misread above. Yes, the problem only comes up if LOAD_ONLY_IF_MODIFIED is set, since that's the only place in which necko's normal redirect handling is short-circuited.

(In reply to comment #79) > You can test by seeing what the event.persisted is for the pagehide event > for the page.... It is, indeed, false. > Yes, the problem only comes up if LOAD_ONLY_IF_MODIFIED is set, since that's > the only place in which necko's normal redirect handling is short-circuited. So, I've been thinking - there's not really any reason to LOAD_ONLY_IF_MODIFIED, is there? If the Necko cache will implement HTTP semantics for us already, maybe we can just AsyncOpen the channel and be done with it?

Well, the idea is that you're using this as a way to test whether you can use your already-cached object, no? If you don't use LOAD_ONLY_IF_MODIFIED, how would you use the resulting necko callbacks to determine the answer to that question?

Would the channel passed to OnStartRequest not support nsICacheInfoChannel, or return false to nsICacheInfoChannel::isFromCache()? My idea relies on Necko filling in those details, it's true.

> Would the channel passed to OnStartRequest not support nsICacheInfoChannel It would, in general... So are you thinking just cancel the load at that point?

Precisely!

That might work, yes. It's more expensive performance-wise than what we do now, but might be nice and simple....

But one important note is that we'll need to make sure that the cached image object we have corresponds to the URI of that last necko channel.

I've implemented all of this, and I've got a pile of patches to upload. But unfortunately I've run into a very strange problem. It seems that the following hunk: --- a/modules/libpr0n/src/imgLoader.cpp +++ b/modules/libpr0n/src/imgLoader.cpp @@ -408,16 +408,19 @@ static PRBool ShouldRevalidateEntry(imgC PRBool bValidateEntry = PR_FALSE; if (aFlags & nsIRequest::LOAD_BYPASS_CACHE) return PR_FALSE; if (aFlags & nsIRequest::VALIDATE_ALWAYS) { bValidateEntry = PR_TRUE; } + if (aEntry->GetMustValidateIfExpired()) { + bValidateEntry = PR_TRUE; + } // // The cache entry has expired... Determine whether the stale cache // entry can be used without validation... // else if (aHasExpired) { // // VALIDATE_NEVER and VALIDATE_ONCE_PER_SESSION allow stale cache // entries to be used unless they have been explicitly marked to against m-c old (will recheck against tip) cause imgLoader itself to leak when running certain crash tests. It seems that, and I haven't yet confirmed this with a debugger, but I have compared refcnt trees, nsContentUtils::Shutdown isn't called. In the "working" (unpatched) case, nsContentUtils::Shutdown is called from a GC. -2 nsGlobalChromeWindow::Release() -2 nsGlobalWindow::Release() -2 nsGlobalChromeWindow::~nsGlobalChromeWindow() -2 nsGlobalWindow::~nsGlobalWindow() -2 nsLayoutStatics::Release() -2 nsLayoutStatics::Shutdown() -1 nsContentUtils::Shutdown() -1 imgLoader::Release() -1 nsContentUtils::Shutdown() -1 imgLoader::Release() If anyone has ideas, I'd love to hear them!

Huh, ignore that. Updating seems to have fixed this problem!

I've integrated Boris' review comments. I also wrote a docshell test to ensure that the "no bfcache go-back" situation Boris mentioned in comment 72 keeps working.

Comment on attachment 542263 [details] [diff] [review] tests v3: add non-bfcache go-back test r=me

These are tests for *this* bug. Specifically, I test that loading a redirected image with cache-control: no-cache is always the same on the same document, in both the dynamically-added and two <img> elements case.

Comment on attachment 539920 [details] [diff] [review] part 0.99999: don't change the cache on redirect This patch goes in-between part 0 and part 1, and actually fixes this bug.

imgRequest::mKeyURI and imgRequest::mURI are the same now that we don't change the cache on redirect and re-insert to the image cache on original URI instead of current URI. So, we should keep track of current URI instead of key uri!

In comment 59, Boris outlined a disaster situation that would require me to write a bunch of code. Then, through some conversation, I came to the realization in comment 75 that if we just removed our use of LOAD_ONLY_IF_MODIFIED, and instead relied upon the Necko cache to be transparent, we'd win. The only added code here is to make sure that the final URI is the same as the final URI we were expecting, because redirects can change where they redirect to!

Comment on attachment 542292 [details] [diff] [review] part 4 v2: put the redirect from if-modified-since validation into the cache keyed on original URI, not current URI Review of attachment 542292 [details] [diff] [review]: ----------------------------------------------------------------- ::: modules/libpr0n/src/imgLoader.cpp @@ +2004,1 @@ > A comment about why we need to inherit from all these guys would be nice. @@ +2194,5 @@ > + return mProgressProxy->AsyncOnChannelRedirect(oldChannel, newChannel, flags, this); > +} > + > +NS_IMETHODIMP imgCacheValidator::OnRedirectVerifyCallback(nsresult aResult) > +{ Add a comment about how this code is similar to the imgRequests version.

Comment on attachment 542300 [details] [diff] [review] part 6: rely on the necko cache to validate all cache-control headers rather than trying to do it ourselves Review of attachment 542300 [details] [diff] [review]: ----------------------------------------------------------------- ::: modules/libpr0n/src/imgLoader.cpp @@ +2038,5 @@ > + PRBool sameURI; > + if (NS_SUCCEEDED(cacheChan->IsFromCache(&isFromCache)) && isFromCache && > + NS_SUCCEEDED(channel->GetURI(getter_AddRefs(channelURI))) && > + channelURI && > + NS_SUCCEEDED(channelURI->Equals(mRequest->mCurrentURI, &sameURI)) && Split out the initialization of these booleans.

Looks green on Try. bz, review ping on the last remaining tests?

Comment on attachment 541203 [details] [diff] [review] part 2: Change "must validate if expired" to "must validate" to match the HTTP spec I guess this works. I think. Slightly lost here, to be honest. :(

Comment on attachment 542273 [details] [diff] [review] test for this bug: ensure 302 Cache-Control: no-cache images are always the same for the same document r=me

http://hg.mozilla.org/integration/mozilla-inbound/rev/872a641c176c http://hg.mozilla.org/integration/mozilla-inbound/rev/9fe9fb1368e3 http://hg.mozilla.org/integration/mozilla-inbound/rev/2809dd5b87f1 http://hg.mozilla.org/integration/mozilla-inbound/rev/86258998da39 http://hg.mozilla.org/integration/mozilla-inbound/rev/f32b08607c6d http://hg.mozilla.org/integration/mozilla-inbound/rev/3e75509585ad http://hg.mozilla.org/integration/mozilla-inbound/rev/b6578040f51f http://hg.mozilla.org/integration/mozilla-inbound/rev/d7411121343b http://hg.mozilla.org/integration/mozilla-inbound/rev/2259a22fe32c http://hg.mozilla.org/integration/mozilla-inbound/rev/10046ea8137b

this push, along with bug 619048, greatly increased random oranges in the followint reftest: layout/reftests/svg/as-image/img-and-image-1.html

backed out from inbound since the reftests failures were not something I'd love to merge to central. fixing the above one or marking as random may be enough, but I don't know what this code does and what the test is supposed to do.

(In reply to comment #103) > this push, along with bug 619048, greatly increased random oranges in the > followint reftest: layout/reftests/svg/as-image/img-and-image-1.html (that randomorange is bug 645267) That's kind of great, actually -- I've been wanting to debug that randomorange, but I've never been able to reproduce. (have never been able to repro locally or in hundreds of tryserver jobs & re-triggers) Now maybe I can figure out what's going wrong... In the meantime, I'd be ok with marking that test as random so that this can re-land.

OK, I've marked it as random. http://hg.mozilla.org/integration/mozilla-inbound/rev/5202f8aa6642 http://hg.mozilla.org/integration/mozilla-inbound/rev/e8f263792437 http://hg.mozilla.org/integration/mozilla-inbound/rev/3a7f41ccc226 http://hg.mozilla.org/integration/mozilla-inbound/rev/35aa54f107ac http://hg.mozilla.org/integration/mozilla-inbound/rev/188a959fc629 http://hg.mozilla.org/integration/mozilla-inbound/rev/586e991940db http://hg.mozilla.org/integration/mozilla-inbound/rev/d3b396624517 http://hg.mozilla.org/integration/mozilla-inbound/rev/6b905a3845cd http://hg.mozilla.org/integration/mozilla-inbound/rev/39bd0f6314c3 http://hg.mozilla.org/integration/mozilla-inbound/rev/47e231b4821e

http://hg.mozilla.org/mozilla-central/rev/5202f8aa6642 http://hg.mozilla.org/mozilla-central/rev/e8f263792437 http://hg.mozilla.org/mozilla-central/rev/3a7f41ccc226 http://hg.mozilla.org/mozilla-central/rev/35aa54f107ac http://hg.mozilla.org/mozilla-central/rev/188a959fc629 http://hg.mozilla.org/mozilla-central/rev/586e991940db http://hg.mozilla.org/mozilla-central/rev/d3b396624517 http://hg.mozilla.org/mozilla-central/rev/6b905a3845cd http://hg.mozilla.org/mozilla-central/rev/39bd0f6314c3 http://hg.mozilla.org/mozilla-central/rev/47e231b4821e

Kyle: This bugfix will ship with Firefox 7. Thanks for the patience. :)