User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1) Gecko/20090630 Firefox/3.5 Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1) Gecko/20090630 Firefox/3.5 Logging this bug after seeing bizarre dependency on a Cookie header in a request for a cached redirect. Found as a result of testing workaround for bug 552605. When requesting a cached redirect resource, Firefox 3.6.x ignores the cache when there is a Cookie value defined in the request. Doesn't seem to matter the length of the header or number of cookies defined in the header. If cookies are disabled, then the cache is obeyed and the redirect value is pulled from the cache correctly. Also was able to reproduce this with cookies enabled, but the Cookie header filtered using the 'Modify Headers' extension. Reproducible: Always Steps to Reproduce: 1. Install some type of HTTP trace tool (HttpFox, LiveHTTPHeaders, et al), or copy the content from http://bsizemore-test.appspot.com/bugs/cookie1.html to your local webserver and monitor the access logs there. 2. Go to http://bsizemore-test.appspot.com/bugs/cookie1.html 3. Notice that redirect.png is requested, and a 302 is returned. 4. Click on 'Page 2'. The same redirect.png is referenced on cookie2.html. It is requested again from the webserver, even though it was returned originally with a Cache-Control header of "max-age=15" and a corresponding Expires header. 5. Disable cookies. 6. Repeat steps 2-4. Note differences. redirect.png is pulled from the cache when cookies are disabled. Actual Results: See above. When cookies are enabled, cached redirect URLs are ignored. When cookies are disabled, cached redirect URLs are used. Expected Results: Browser should always obey cache control headers for redirects, whether cookies are enabled or not.

This behavior doesn't seem to be specific to 3.6.x. Just reproduced this on Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1) Gecko/20090630 Firefox/3.5

I just stepped through this, and the issue seems to be this code in nsHttpChannel::CheckCache: // Sites redirect back to the original URI after setting a session/tracking // cookie. In such cases, force revalidation so that we hit the net and do not // cycle thru cached responses. if (isCachedRedirect && mRequestHead.PeekHeader(nsHttp::Cookie)) doValidation = PR_TRUE; Looks like this was added to fix bug 133973. Maybe there's a better way that could be fixed...

This code essentially says: If the request has a cookie, ignore any cached 3xx-results. Quoting bug #133973, comment #25 'it is effectively implementing a solution that assumes "Vary: Cookie" on any redirect response'. It would be interesting to remove that code-block. Assuming that servers behave better these days (it's over 8 years since the code was added) we could actually get Vary: Cookie on 3xx-responses which redirect back to themselves, and our Vary-header-code would handle this. If not, MoCo is in a better position now than 8 years ago to convince servers how to do this right. (Also, there is better support for Vary coming up in bug #468426.)

This patch removes the code-block in question. There is also a test which does what I *believe* the code was added to handle (bug #133973), and it works nicely with this code removed. If I have misunderstood the essence of bug #133973 and the test is doing the wrong thing, feel free to correct me. With the patch the test-url given in comment #0 seems to work as expected.

Ahh - I see the issue with the above test: it must redirect in a cycle. I have a test which does this and will come up with a fix.

Proposed way to break cycles in cached redirects. Seems to pass tryserver fine, and also behaves as expected with URL from comment #0.

So why did the test in that first patch work? Why an nsCStringArray and not nsTArray<nsCString>? Why do you need the explicit Clear() calls, for that matter? Does it make sense to at least drop the cache keys in OnStopRequest or OnStartRequest or something, instead of in the dtor? Fix the indentation in the test? Why does the test have |< 3| instead of |< 1|?

(In reply to comment #7) > So why did the test in that first patch work? The first test did not redirect in a cycle. > Why an nsCStringArray and not nsTArray<nsCString>? I'm open to arguments/recommendations of either. > Why do you need the explicit Clear() calls, for that matter? Probably redundant, yes, if dtor does the same job. > Does it make sense to at least drop the cache keys in OnStopRequest or > OnStartRequest or something, instead of in the dtor? Why? > Fix the indentation in the test? Sure. > Why does the test have |< 3| instead of |< 1|? Leftover from testing - not important.

> The first test did not redirect in a cycle. But it redirected back to itself, right? Did this not result in it reading from cache for some reason? > I'm open to arguments/recommendations of either. I think nsTArray would be better. Makes the Clear() issue irrelevant too. > Why? So as to not use the extra memory for a potentially long time? Or is the memory usage fairly low?

(In reply to comment #9) > > The first test did not redirect in a cycle. > > But it redirected back to itself, right? Did this not result in it reading > from cache for some reason? Not quite sure about the exact reason here, but the cache-entry for 2nd request is found before 1st has finished, making the 2nd entry wait for the OnCacheAvailable-call. And in this situation the code-block in question was not executed. I did not analyze it deeper but suddenly realized that bug #133973 mentions a number of redirects, not just one back to itself. Bottom line: when redirection goes like /a -> /b -> /a the code-block was necessary to avoid hitting the max-number of redirects. This patch breaks the cycle by keeping track of the chain. The simple case /a -> /a seems to be handled by other parts of the code. > > I'm open to arguments/recommendations of either. > > I think nsTArray would be better. Makes the Clear() issue irrelevant too. Fair enough. I'll fix it. > > Why? > > So as to not use the extra memory for a potentially long time? Or is the > memory usage fairly low? We store the cache-keys, i.e. one string per channel. Compared to everything else involved in a channel-load I'd say this is reasonable.

So several kilobytes of data per channel, in general (long URIs, multiple redirects)? Possibly hundreds of channes per page? I suppose it's ok.

If we really believe memory-usage will explode because of this it's simple to store a hash of the url instead. It will of-course impact cpu-usage but there's no free lunch, as they say. It is also simple to add cleanup-code at strategic places (for example in OnStopRequest) if we see increased memory-usage, but I'd prefer to keep it also in the dtor to make sure we don't introduce leaks.

I don't think the temporary usage is a problem. We should probably just drop this data any place we drop mListener; I seem to recall a good central location for that. I agree with also doing it in the dtor.

Changed to use nsTArray<nsCString> plus other details from the review. I don't really see the connection to mListener so I didn't code to drop the chain other places than in the dtor and at the point where we decide to hit the net. However, this is trivial to do if necessary. A central place may be DoNotifyListener(), but I'm not convinced.

> I don't really see the connection to mListener We null out mListener when our channel is "done"; it should be safe to null out the cache keys there too. The dtor might come very very late; I see no reason to hold on to the cache keys that long. + if (!mRedirectedCachekeys) { + mRedirectedCachekeys = new nsTArray<nsCString>(); + } else { + if (mRedirectedCachekeys->Contains(cacheKey)) + doValidation = PR_TRUE; + } should be: if (!mRedirectedCachekeys) mRedirectedCachekeys = new nsTArray<nsCString>(); else if (mRedirectedCachekeys->Contains(cacheKey)) doValidation = PR_TRUE; > + // Clear chain since we are not using cached responses anymore "Clear cache key chain" The rest looks good.

Clearing the url-chain at strategic places plus fixed the syntax of if-else block. This version has not been run via tryserver but I believe it's quite safe. (I can send it for a spin if needed.)

Comment on attachment 472594 [details] [diff] [review] Addressed comments >+ else if (mRedirectedCachekeys->Contains(cacheKey)) >+ doValidation = PR_TRUE; Please fix the indent. r=bzbarsky

Does this need any approval?

Yes, since it's not a blocker. Or needs to be nominated to block.

Comment on attachment 472620 [details] [diff] [review] Indent fixed... Requesting approval...

Is this something we still want to do for Firefox 4? What's the risk/benefit involved in taking this change or not taking it?

It originates from bug #552605 which is a 2.0 blocker. Bz would be the right person to state whether this one is important in the context of bug #552605.

I don't think we need this at all for bug 552605....

Approval received, requesting check-in...

Logic identical to previous version but due to changes on trunk some code had to be moved to HttpBaseChannel and the interface nsIHttpChannelInternal extended. Because of these changes I'm requesting new review and approval for 2.0

Comment on attachment 486043 [details] [diff] [review] Modified to work with recent changes to trunk Why do we do the "make sure we don't leak" cleanup from ~nsHttpChannel and not ~HttpBaseChannel? That will in fact leak if someone ever calls SetCacheKeysRedirectChain on a base channel that's not an nsHttpChannel. This looks like it won't compile on Windows, because the types of SetCacheKeysRedirectChain in HttpBaseChannel and nsIHttpChannelInteral are different (NS_IMETHOD vs nsresult, so different calling conventions). It also doesn't make sense to put "inline" on a virtual method like that. SetCacheKeysRedirectChain should probably be notxpcom as well. And put a space after the ']' in the IDL there? You need to change the IID of nsIHttpChannelInternal. And put the new method at the end of the interface. This needs to land for b7 if it's going to land for 2.0 as-is. Landing after that would mean no changing nsIHttpChannelInternal (though you could add a new interface for the new method).

Jeez... I must have been in a real hurry here... my apologies! New patch coming up.

Had to use an attribute in the idl (and explicitly disable the getter) to work around the usage of NS_FORWARD_SAFE_... in nsViewSourceChannel.h. (NS_FWD-macros doesn't seem to deal graciously with functions returning void.) If there is another preferred workaround for this, please let me know... Pushed to tryserver to be on the safe side (and to not waste too much more reviewer-time on stupid errors).

Comment on attachment 486598 [details] [diff] [review] Updated according to comments from reviewer > NS_FORWARD_SAFE_... in nsViewSourceChannel.h. Oh, right. In that case, just leave the old setup with the idl (with the method) and ignore the [notxpcom] thing. Put the NS_IMETHODIMP bit in the .cpp file, please? r=me with those changes.

(In reply to comment #29) > Pushed to tryserver to be on the safe side (and to not waste too much more > reviewer-time on stupid errors). Looks good. Some oranges, but all seem to be known issues.

Removed notxpcom, carrying over r+ from previous version, requesting approval for 2.0

Move the NS_IMETHOD bit next to the other nsIHttpChannelInternal stuff? And the .cpp should be 2-space indented in HttpBaseChannel, right?

... according to latest comments. Carrying over the r+ and requesting approval.

Comment on attachment 486870 [details] [diff] [review] Fixed positioning and indents What's the risk/reward tradeoff here? I don't really want to be taking an interface change this late in the cycle anyway.

>(In reply to comment #35) > What's the risk/reward tradeoff here? I don't really want to be taking an > interface change this late in the cycle anyway. Uhh.. didn't see this one until now... :( The reward of this patch is using cached 3xx responses for requests with cookies. I don't have statistics on this but can imagine such requests are not uncommon. Current code always validates such requests with the server, introducing unnecessary network-traffic. The main risk is IMO that we may use a cached response where we shouldn't - however this would be a bug also occurring currently for requests with no cookies, and should be identified up and fixed independently of this bug. The logic in the patch is pretty simple and should not produce regressions, and we do not run the risk of looping forever because the existing redirect-limit is also counting the cached redirects. Quoting from the IDL of the interface: "Dumping ground for http. This interface will never be frozen." Moreover, the change to the interface is adding a no-script setter, hence it's backward-compatible and it should be darn hard to mess up that value. It is not possible to request the value. So, I'd say this is a pretty low-risk patch with a (unknown) potential for reducing network-traffic. The "changing interface"-aspect is IMO irrelevant.

Comment on attachment 486870 [details] [diff] [review] Fixed positioning and indents Requesting approval again...

Comment on attachment 486870 [details] [diff] [review] Fixed positioning and indents ok, I don't think the risk is worth it for FF4 (especially for b7) and we're at API freeze after b7, so this should wait for FF5.

Comment on attachment 486870 [details] [diff] [review] Fixed positioning and indents Since we are a couple of betas later in the game I take the liberty of requesting approval again...

Comment on attachment 486870 [details] [diff] [review] Fixed positioning and indents No, really.

Comment on attachment 472620 [details] [diff] [review] Indent fixed... (bookkeeping)

Verified using Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0