Last Comment Bug 552605 - Image preloads of a URI which is redirected mis-prime the image cache
: Image preloads of a URI which is redirected mis-prime the image cache
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla7
Assigned To: Joe Drew (not getting mail)
:
Mentors:
http://hk.sports.yahoo.com/test/clone...
: 560374 568842 (view as bug list)
Depends on: 670452
Blocks: 552538
  Show dependency treegraph
 
Reported: 2010-03-15 21:16 PDT by kylec
Modified: 2011-07-09 20:34 PDT (History)
19 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.x+
wanted


Attachments
screencap for the double count issue (73.54 KB, image/jpeg)
2010-03-15 21:21 PDT, kylec
no flags Details
fiddler log (274.42 KB, application/octet-stream)
2010-03-21 23:42 PDT, kylec
no flags Details
screencap of the html request logs (432.33 KB, image/jpeg)
2010-03-21 23:45 PDT, kylec
no flags Details
logs from live http header (40.01 KB, text/plain)
2010-03-23 00:54 PDT, kylec
no flags Details
part 0.99999: don't change the cache on redirect (2.38 KB, patch)
2011-06-16 16:01 PDT, Joe Drew (not getting mail)
jmuizelaar: review+
Details | Diff | Review
test: make sure we hit the server with Cache-control: no-cache on redirects (5.56 KB, patch)
2011-06-16 16:02 PDT, Joe Drew (not getting mail)
bzbarsky: review-
Details | Diff | Review
tests v2: make sure we hit the server with Cache-control: no-cache on redirects (7.51 KB, patch)
2011-06-16 21:27 PDT, Joe Drew (not getting mail)
bzbarsky: review+
Details | Diff | Review
part 1: Set cache expiry, validation on both OnStartRequest and OnRedirectVerifyCallback (6.98 KB, patch)
2011-06-17 11:51 PDT, Joe Drew (not getting mail)
jmuizelaar: review+
bzbarsky: feedback-
Details | Diff | Review
Part 0: Fix bracing/spacing that crept in (5.79 KB, patch)
2011-06-22 15:38 PDT, Joe Drew (not getting mail)
jmuizelaar: review+
Details | Diff | Review
part 1 v2: Set cache expiry, validation on both OnStartRequest and OnRedirectVerifyCallback - make it static (7.11 KB, patch)
2011-06-22 15:40 PDT, Joe Drew (not getting mail)
jmuizelaar: review+
Details | Diff | Review
part 2: Change "must validate if expired" to "must validate" to match the HTTP spec (5.46 KB, patch)
2011-06-22 15:44 PDT, Joe Drew (not getting mail)
jmuizelaar: review+
bzbarsky: feedback+
Details | Diff | Review
part 3: check for file:// scheme in SetCacheValidation rather than on cache entry construction (5.89 KB, patch)
2011-06-22 15:48 PDT, Joe Drew (not getting mail)
no flags Details | Diff | Review
part 4: handle redirects when handling if-modified-since validation (13.24 KB, patch)
2011-06-22 15:53 PDT, Joe Drew (not getting mail)
no flags Details | Diff | Review
tests v3: add non-bfcache go-back test (17.50 KB, patch)
2011-06-27 14:09 PDT, Joe Drew (not getting mail)
bzbarsky: review+
Details | Diff | Review
test for this bug: ensure 302 Cache-Control: no-cache images are always the same for the same document (5.29 KB, patch)
2011-06-27 14:38 PDT, Joe Drew (not getting mail)
bzbarsky: review+
Details | Diff | Review
part 3 v2: fix compile error (5.89 KB, patch)
2011-06-27 15:31 PDT, Joe Drew (not getting mail)
jmuizelaar: review+
Details | Diff | Review
part 4 v2: put the redirect from if-modified-since validation into the cache keyed on original URI, not current URI (13.44 KB, patch)
2011-06-27 15:33 PDT, Joe Drew (not getting mail)
jmuizelaar: review+
Details | Diff | Review
part 5: imgRequest::mKeyURI is the same as mURI now (7.74 KB, patch)
2011-06-27 15:38 PDT, Joe Drew (not getting mail)
jmuizelaar: review+
Details | Diff | Review
part 6: rely on the necko cache to validate all cache-control headers rather than trying to do it ourselves (5.37 KB, patch)
2011-06-27 15:45 PDT, Joe Drew (not getting mail)
jmuizelaar: review+
Details | Diff | Review

Description kylec 2010-03-15 21:16:22 PDT
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
Comment 1 kylec 2010-03-15 21:21:36 PDT
Created attachment 432732 [details]
screencap for the double count issue 

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
Comment 2 Brandon Sterne (:bsterne) 2010-03-16 08:36:19 PDT
Not a security-sensitive bug.
Comment 3 kylec 2010-03-17 02:40:22 PDT
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
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-03-21 02:19:04 PDT
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/
Comment 5 Boris Zbarsky [:bz] 2010-03-21 10:27:17 PDT
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?
Comment 6 kylec 2010-03-21 21:37:14 PDT
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
Comment 7 Boris Zbarsky [:bz] 2010-03-21 23:03:43 PDT
> 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...
Comment 8 kylec 2010-03-21 23:42:56 PDT
Created attachment 433882 [details]
fiddler log

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
Comment 9 kylec 2010-03-21 23:45:18 PDT
Created attachment 433883 [details]
screencap of the html request logs

screencap of the html request logs, FYI, kyle
Comment 10 kylec 2010-03-21 23:59:10 PDT
Hi Boris,

To reproduce the doublecount issue, please follow comment 8.

thanks in advance for your prompt reply

cheers
kyle
Comment 11 Boris Zbarsky [:bz] 2010-03-22 09:24:50 PDT
> 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.
Comment 12 Boris Zbarsky [:bz] 2010-03-22 17:59:46 PDT
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?
Comment 13 kylec 2010-03-22 18:47:30 PDT
my testing is in windowsXP
Comment 14 Boris Zbarsky [:bz] 2010-03-22 18:50:35 PDT
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.
Comment 15 kylec 2010-03-23 00:46:01 PDT
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
Comment 16 kylec 2010-03-23 00:54:40 PDT
Created attachment 434181 [details]
logs from live http header

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
Comment 17 Boris Zbarsky [:bz] 2010-03-24 11:14:05 PDT
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.
Comment 18 Boris Zbarsky [:bz] 2010-03-24 11:36:00 PDT
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.
Comment 19 kylec 2010-03-24 21:05:01 PDT
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
Comment 20 Boris Zbarsky [:bz] 2010-03-24 21:26:41 PDT
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.
Comment 21 Boris Zbarsky [:bz] 2010-03-24 21:27:29 PDT
It also doesn't help that imagelib is perennially understaffed, of course.
Comment 22 kylec 2010-03-24 21:40:28 PDT
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
Comment 23 Reed Loden [:reed] (use needinfo?) 2010-03-24 21:44:49 PDT
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.
Comment 24 Boris Zbarsky [:bz] 2010-03-24 21:51:20 PDT
> 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)...
Comment 25 Joe Drew (not getting mail) 2010-03-25 09:03:24 PDT
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.
Comment 26 kylec 2010-03-25 21:39:01 PDT
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
Comment 27 Boris Zbarsky [:bz] 2010-04-20 05:39:28 PDT
*** Bug 560374 has been marked as a duplicate of this bug. ***
Comment 28 Brooks Sizemore 2010-04-22 19:58:05 PDT
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.
Comment 29 Boris Zbarsky [:bz] 2010-04-22 20:05:17 PDT
If you're sending a "Vary" header that includes "cookie", then that's expected.
Comment 30 Brooks Sizemore 2010-04-22 20:18:08 PDT
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.
Comment 31 Boris Zbarsky [:bz] 2010-04-22 20:30:49 PDT
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?
Comment 32 Brooks Sizemore 2010-04-22 21:03:51 PDT
Logged bug 561276 to track issue. Thanks.
Comment 33 Brooks Sizemore 2010-04-28 09:43:17 PDT
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
Comment 34 Brooks Sizemore 2010-05-03 17:09:08 PDT
Boris, Joe:

Any update on the ETA for the fix for this? Are we still shooting for June?
Comment 35 Joe Drew (not getting mail) 2010-05-03 17:31:00 PDT
That's not really an ETA, just an "earliest possible" date. But yes, that's still the earliest possible date. :)
Comment 36 Boris Zbarsky [:bz] 2010-06-07 12:01:55 PDT
*** Bug 568842 has been marked as a duplicate of this bug. ***
Comment 37 Boris Zbarsky [:bz] 2010-06-07 12:03:05 PDT
Note that this bug also breaks other consumers that rely on the image cache working correctly (like CSS; see bug 552538).
Comment 38 Boris Zbarsky [:bz] 2010-06-07 20:17:57 PDT
Given bug 552538, I'm renominating for 1.9.3 blocking.
Comment 39 Bobby Holley (PTO through June 13) 2010-08-02 19:08:23 PDT
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. ;-)
Comment 40 kylec 2011-02-01 02:24:33 PST
hi joe, 

any update from your side? could you please pump up this ticket to higher priority? looking forward to your reply

cheers
Kyle
Comment 41 kylec 2011-02-28 02:59:49 PST
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
Comment 42 Joe Drew (not getting mail) 2011-06-07 14:01:21 PDT
Bug 89419 specifically made us change from caching pre-redirect to post-redirect URI. Is the thought that we switch back? Cache both URIs?
Comment 43 Boris Zbarsky [:bz] 2011-06-07 14:10:32 PDT
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.
Comment 44 Boris Zbarsky [:bz] 2011-06-07 14:11:47 PDT
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....
Comment 45 Joe Drew (not getting mail) 2011-06-14 13:24:58 PDT
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.
Comment 46 Boris Zbarsky [:bz] 2011-06-14 13:40:27 PDT
> 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).
Comment 47 Joe Drew (not getting mail) 2011-06-16 16:01:47 PDT
Created attachment 539920 [details] [diff] [review]
part 0.99999: don't change the cache on redirect

(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.
Comment 48 Joe Drew (not getting mail) 2011-06-16 16:02:48 PDT
Created attachment 539921 [details] [diff] [review]
test: make sure we hit the server with Cache-control: no-cache on redirects

This test ensures that we don't break the intent of bug 89419.
Comment 49 Joe Drew (not getting mail) 2011-06-16 16:03:10 PDT
I'll write and upload a test for *this* bug tomorrow.
Comment 50 Boris Zbarsky [:bz] 2011-06-16 19:37:56 PDT
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?
Comment 51 Joe Drew (not getting mail) 2011-06-16 21:27:11 PDT
Created attachment 539985 [details] [diff] [review]
tests v2: make sure we hit the server with Cache-control: no-cache on redirects

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.
Comment 52 Boris Zbarsky [:bz] 2011-06-16 21:34:47 PDT
> "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 53 Boris Zbarsky [:bz] 2011-06-16 21:38:26 PDT
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!
Comment 54 Joe Drew (not getting mail) 2011-06-16 21:53:47 PDT
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 55 Jeff Muizelaar [:jrmuizel] 2011-06-17 06:53:42 PDT
Comment on attachment 539920 [details] [diff] [review]
part 0.99999: don't change the cache on redirect

Looks pleasant to me.
Comment 56 Joe Drew (not getting mail) 2011-06-17 11:51:35 PDT
Created attachment 540099 [details] [diff] [review]
part 1: Set cache expiry, validation on both OnStartRequest and OnRedirectVerifyCallback

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 57 Boris Zbarsky [:bz] 2011-06-17 12:00:33 PDT
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...
Comment 58 Joe Drew (not getting mail) 2011-06-17 12:09:06 PDT
(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).
Comment 59 Boris Zbarsky [:bz] 2011-06-17 12:35:47 PDT
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?
Comment 60 Joe Drew (not getting mail) 2011-06-17 13:09:19 PDT
I believe that to be the case.

What should we do in that case, though? Set it to the minimum of all the lifetimes?
Comment 61 Boris Zbarsky [:bz] 2011-06-17 13:29:31 PDT
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?
Comment 62 Joe Drew (not getting mail) 2011-06-17 15:52:03 PDT
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?
Comment 63 Boris Zbarsky [:bz] 2011-06-17 17:07:08 PDT
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 64 Jeff Muizelaar [:jrmuizel] 2011-06-20 12:22:37 PDT
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.
Comment 65 Joe Drew (not getting mail) 2011-06-22 15:38:55 PDT
Created attachment 541201 [details] [diff] [review]
Part 0: Fix bracing/spacing that crept in
Comment 66 Joe Drew (not getting mail) 2011-06-22 15:40:47 PDT
Created attachment 541202 [details] [diff] [review]
part 1 v2: Set cache expiry, validation on both OnStartRequest and OnRedirectVerifyCallback - make it static

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.
Comment 67 Joe Drew (not getting mail) 2011-06-22 15:44:30 PDT
Created attachment 541203 [details] [diff] [review]
part 2: Change "must validate if expired" to "must validate" to match the HTTP spec

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.
Comment 68 Joe Drew (not getting mail) 2011-06-22 15:48:46 PDT
Created attachment 541206 [details] [diff] [review]
part 3: check for file:// scheme in SetCacheValidation rather than on cache entry construction

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.
Comment 69 Joe Drew (not getting mail) 2011-06-22 15:53:17 PDT
Created attachment 541210 [details] [diff] [review]
part 4: handle redirects when handling if-modified-since validation

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.
Comment 70 Joe Drew (not getting mail) 2011-06-22 15:55:33 PDT
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"?
Comment 71 Boris Zbarsky [:bz] 2011-06-22 20:24:37 PDT
> 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 72 Boris Zbarsky [:bz] 2011-06-22 20:28:14 PDT
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?
Comment 73 Joe Drew (not getting mail) 2011-06-23 08:51:00 PDT
(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.
Comment 74 Boris Zbarsky [:bz] 2011-06-23 09:14:46 PDT
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.
Comment 75 Joe Drew (not getting mail) 2011-06-23 09:25:53 PDT
OK, so the problem you're describing only comes up when we ask for nsICachingChannel::LOAD_ONLY_IF_MODIFIED, then?
Comment 76 Boris Zbarsky [:bz] 2011-06-23 09:32:37 PDT
Yes (but of course that's the default value, right?).
Comment 77 Joe Drew (not getting mail) 2011-06-23 11:25:04 PDT
(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.
Comment 78 Joe Drew (not getting mail) 2011-06-23 11:26:31 PDT
(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?
Comment 79 Boris Zbarsky [:bz] 2011-06-23 11:28:37 PDT
> 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.
Comment 80 Joe Drew (not getting mail) 2011-06-23 12:05:52 PDT
(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?
Comment 81 Boris Zbarsky [:bz] 2011-06-23 12:51:46 PDT
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?
Comment 82 Joe Drew (not getting mail) 2011-06-23 13:10:36 PDT
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.
Comment 83 Boris Zbarsky [:bz] 2011-06-23 14:00:56 PDT
> Would the channel passed to OnStartRequest not support nsICacheInfoChannel

It would, in general...  So are you thinking just cancel the load at that point?
Comment 84 Joe Drew (not getting mail) 2011-06-23 14:06:47 PDT
Precisely!
Comment 85 Boris Zbarsky [:bz] 2011-06-23 14:12:15 PDT
That might work, yes.  It's more expensive performance-wise than what we do now, but might be nice and simple....
Comment 86 Boris Zbarsky [:bz] 2011-06-23 14:12:57 PDT
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.
Comment 87 Joe Drew (not getting mail) 2011-06-24 15:22:07 PDT
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!
Comment 88 Joe Drew (not getting mail) 2011-06-24 17:09:02 PDT
Huh, ignore that.  Updating seems to have fixed this problem!
Comment 89 Joe Drew (not getting mail) 2011-06-27 14:09:02 PDT
Created attachment 542263 [details] [diff] [review]
tests v3: add non-bfcache go-back test

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 90 Boris Zbarsky [:bz] 2011-06-27 14:25:18 PDT
Comment on attachment 542263 [details] [diff] [review]
tests v3: add non-bfcache go-back test

r=me
Comment 91 Joe Drew (not getting mail) 2011-06-27 14:38:22 PDT
Created attachment 542273 [details] [diff] [review]
test for this bug: ensure 302 Cache-Control: no-cache images are always the same for the same document

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 92 Joe Drew (not getting mail) 2011-06-27 15:27:29 PDT
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.
Comment 93 Joe Drew (not getting mail) 2011-06-27 15:31:19 PDT
Created attachment 542291 [details] [diff] [review]
part 3 v2: fix compile error
Comment 94 Joe Drew (not getting mail) 2011-06-27 15:33:22 PDT
Created 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
Comment 95 Joe Drew (not getting mail) 2011-06-27 15:38:53 PDT
Created attachment 542295 [details] [diff] [review]
part 5: imgRequest::mKeyURI is the same as mURI now

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!
Comment 96 Joe Drew (not getting mail) 2011-06-27 15:45:29 PDT
Created 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

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 97 Jeff Muizelaar [:jrmuizel] 2011-06-28 13:14:11 PDT
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 98 Jeff Muizelaar [:jrmuizel] 2011-06-28 13:24:56 PDT
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.
Comment 99 Joe Drew (not getting mail) 2011-06-30 11:48:30 PDT
Looks green on Try. bz, review ping on the last remaining tests?
Comment 100 Boris Zbarsky [:bz] 2011-06-30 14:29:56 PDT
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 101 Boris Zbarsky [:bz] 2011-06-30 14:31:24 PDT
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
Comment 103 Marco Bonardo [::mak] 2011-07-01 03:50:31 PDT
this push, along with bug 619048, greatly increased random oranges in the followint reftest: layout/reftests/svg/as-image/img-and-image-1.html
Comment 104 Marco Bonardo [::mak] 2011-07-01 09:11:51 PDT
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.
Comment 105 Daniel Holbert [:dholbert] 2011-07-01 09:53:04 PDT
(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.
Comment 108 Joe Drew (not getting mail) 2011-07-03 20:57:28 PDT
Kyle: This bugfix will ship with Firefox 7. Thanks for the patience. :)

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