133973 - URL redirection limit exceeded

Status: VERIFIED FIXED

(Core :: Networking: HTTP, defect)

Description

[Jason Huebel]

From Bugzilla Helper:
User-Agent: Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; DigExt; Q312461)
BuildID:    2002031104



Reproducible: Always
Steps to Reproduce:
Visit specified URL.

Actual Results:  Recieved an error that the URL redirection limit was exceeded.

Expected Results:  Should have browsed to the appropriate web page.

Comment 1

[Paul Wyskoczka]

*** This bug has been marked as a duplicate of 128443 ***

Comment 2

[Matthias Versen [:Matti]]

This can't be a dupe of that bug because:
a) The URL is different
b) bug 128443 is in Tech Evangelism and even if this bugs is the same problem it
would be not a dupe.

Reporter: Do you accept all cookies ?

Comment 3

[Matthias Versen [:Matti]]

-> Networking:http

and confirming with win2k and 0.9.9 
(my current build is horked due to the gmake change)

Comment 4

[Jason Huebel]

"Enable all cookies" is selected in the Edit/Preferences/Privacy & 
Security/Cookies.

Comment 5

[Darin Fisher]

-> badami for investigation

Comment 6

[Vinay Badami]

Attachment: version 1

1. In nsHttpResponseHead::ExpiresInPast(), assume that the entry has expired
and then check if it actually is going to expire in the future.
2. In nsHttpChannel two changes :
   a. Some more debug logging
   b. Do not assume cache entry as valid if we got only READ access unless we
are offline.

Comment 7

[Vinay Badami]

wrt comment 6, I need to mention that for this particular case we only need the
fix  in nsHttpResponseHead. However, I think that it is better to get both the
fixes in.

Comment 8

[Darin Fisher]

vinay:

as i said via email, if we get READ ONLY access to the cache then we should not
go out to fetch the document from the network.  this can only mean that we are
offline already.  so, there is no value in adding the first part of this patch
(a debug assertion might be good, but otherwise i don't see the point).

the changes to ExpiresInPast are going to cause regressions elsewhere.  we use
that function to implement nsIHttpChannel::isNoCacheResponse().  ExpiresInPast
is only TRUE if the server sends an Expires header w/ a value less than the
value of the Date header.  we treat this case similarly to 'Pragma: no-cache'
whereas a response sent w/o an Expires header should not be treated as 'Pragma:
no-cache'.

seems to me that your patch for ExpiresInPast works because it changes the
behavior of nsHttpResponseHead::MustValidate() to always return TRUE when an
Expires header is not present.  this is bad because it'll prevent caching in a
lot of cases. 

perhaps nsHttpResponseHead::MustValidateIfExpired could be modified to return
TRUE for redirects.  but that still seems like a hack.  we need to better
understand why we're hitting this condition.  is there a '<=' that should be a '<'??

Comment 9

[Gagan]

Comment on attachment 77028 [details] [diff] [review]
version 1

r=gagan

Comment 10

[Darin Fisher]

Comment on attachment 77028 [details] [diff] [review]
version 1

needs-work

Comment 11

[Gagan]

Comment on attachment 77028 [details] [diff] [review]
version 1

revoking r= per darin's comments. I am clearly getting way old for this stuff!
:)

Comment 12

[Vinay Badami]

Attachment: http log at level 2 logging

Summary of log
/ causes redirect to /all/public.
We then have a series of redirects resulting in setting of a session cookie and
then a redirect back to /all/public. We are then picking up the cached
response.

The Cache-Control directive should have been no-cache as opposed to private
when we got the response for /all/public. We are not getting an expires header
for the request to /all/public.

Comment 13

[Vinay Badami]

Attachment: revalidate if redirect and cookie is present

Comment 14

[Darin Fisher]

vinay: good work isolating the real problem here.  your patch looks like a good
work around for this broken server.

unfortunately, i think your patch would hurt performance on sites that follow
the RFC to the letter.  some redirects are supposed to be cached... even
requests that have Cookie headers that result in redirects.  you could argue
that since the first request didn't have cookies and the second one does that
they are different.  however, there is no provision for such in the RFC. 
perhaps if the website sent a "Vary: Cookie" response header, then we'd be just
in landing something similar to your patch.  of course, we currently don't
handle the "Vary" header correctly, but that should change ;-)

i'm torn between landing this patch and evangelizing the website.  they are
clearly violating RFC2616.  301 redirects are explicitly cacheable, and there's
nothing in this response that would dictate otherwise:

  HTTP/1.1 301 Moved Permanently
  Server: Microsoft-IIS/4.0
  Date: Tue, 02 Apr 2002 17:30:29 GMT
  Location: http://www.adtran.com/all/public
  Content-Length: 0
  Content-Type: text/html
  Cache-Control: private

the site should be using a 302, not a 301.  no matter what we decide to do on
our end, i want this bug to eventually end up in the hands of our evangelism team.

Comment 15

[Vinay Badami]

Darin
I agree and understand. I think we should lean on getting this checked in. There
are too many sites that have problems of this nature. No point tracking down
these problems again on other sites. Just a waste of $$.
Given the fact that it works with IE, it looks like they must have some such
form a similar hack.

Comment 16

[Susie Wyshak]

You get this error message on MSN too.
1. go to http://communities.msn.com/people
2. click a chat

Result: you get the "Redirection limit ..." error

It *is* expected that this site doesn't work (b/c they need to fix it) but that
message was quite unexpected. 
Could it at least be reworded to something more user friendly if the error needs
to stay?

Comment 17

[Susie Wyshak]

FYI behavior in 9.4 on the MSN Chat site is an incessant "Transferring -
Connecting" message in the bottom status bar. 

Call me an evangelista but I like that a user might be more prone to report the
problem to the website's customer support, based on that behavior.

Comment 18

[Vinay Badami]

Susie
What is wrong with this site ? Is there any info ? Seems to work with IE.

Comment 19

[Darin Fisher]

gagan, bbaetz: what are your thoughts on this issue?  see comment #14.  thx!

Comment 20

[Darin Fisher]

Susie: we added a limit on the number of times an URL could be redirected to
another URL after mozilla 0.9.4.  this was done to prevent needlessly spamming
the internet and to let the user know that mozilla discovered that something is
wrong.

originally, i wanted the message to say that there is something wrong with the
website, but in reality it is usually a combination of mozilla and the website
that leads to infinite redirects.  we've nailed a lot of the problems in mozilla
that could lead to an infinite redirect, and what were seeing now are sites that
don't follow the standards, but instead rely on IE's behavior to dictate
expected behavior :-/  as is the case for chat.msn.com, iirc.

Comment 21

[Bradley Baetz (:bbaetz)]

I think that no Vary header should imply:

Vary: Cookie

Optionally, only do this if we are actualy going to accept the cookie (based on
cookie prefs), but that may not be possible from http.

Do other browsers cache 301 responses permenantly, and what do thay do in this case?

Comment 22

[Bradley Baetz (:bbaetz)]

Oh, and only check on the cookie's value, not expiration date, if possible.

OTOH, this is a redirection, so there really is no data. Maybe lets just not
cache 301s which set a cookie we didn't already have.

Comment 23

[Gagan]

I am inclining towards bbaetz suggestion. IIRC that's similar to what we did for
4.x too. 

Comment 24

[Vinay Badami]

Doesnt the patch do exactly what u folks are suggesting ?
We check for a cookie and if it is a redirect. 
R u suggesting that in addition we check whther there was a cookie on the
original request etc. I did not want to do that since we then start comparing
the cookie value etc, existence of a new cookie etc.
I think the fix in hand will suffice.

Comment 25

[Darin Fisher]

it'd be difficult to solve this problem by observing when cookies are set
because we'd have to "be the cookie manager" to know when cookies that are
received should be sent.  i think vinay's patch is the best solution.  it is
effectively implementing a solution that assumes "Vary: Cookie" on any redirect
response.  since we don't really have much in the way of Vary header support
right now, i think vinay's patch should be checked in as is.

Comment 26

[Darin Fisher]

Comment on attachment 77180 [details] [diff] [review]
revalidate if redirect and cookie is present

sr=darin

Comment 27

[Bradley Baetz (:bbaetz)]

Comment on attachment 77180 [details] [diff] [review]
revalidate if redirect and cookie is present

r=bbaetz

Comment 28

[Asa Dotzler [:asa]]

Comment on attachment 77180 [details] [diff] [review]
revalidate if redirect and cookie is present

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 29

[Vinay Badami]

fixed with checkin

D:\mozilla\netwerk\protocol\http\src>cvs commit nsHttpChannel.cpp
Checking in nsHttpChannel.cpp;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp,v  <-- 
nsHttpChannel.cpp
new revision: 1.108; previous revision: 1.107
done

Comment 30

[Susie Wyshak]

In answer to Comment #18 MSN needs to do some server side work on Chat, for
Gecko compatibility. If it didn't work in IE either that would be pretty funny!

Comment 31

[John Unruh]

Verified http://www.adtran.com on Win2000 and Linux. MSN chat works only on
Win32, as designed.