83471 - Redirection loops

Status: VERIFIED FIXED

(Core :: Networking: HTTP, defect, P1)

Description

[tenthumbs]

They sometimes happen. Mozilla just blindly follows each redirection until the
user gives up on it.

I tried Opera on Linux and it just goes around a loop once but then seems to get
stuck in some internal loop of its own and just sits there. That's worse than
mozilla. 

Lynx does best. It follows the loop for a while and then says:

  Alert!: Redirection limit of 10 URL's reached.

I think mozilla should act like Lynx.

Comment 1

[gabriel]

Could be a dupe of bug 40072.

Comment 2

[neeti]

Is there a testcase for this?

Comment 3

[tenthumbs]

Actually, I think bug 40072 and bug 76485 demonstrate this one.

I'll attach some simple shell scripts. They work nicely on Linux running an
Apache server. Just point mozilla at one of them and watch it go back and forth.

Comment 4

[tenthumbs]

Attachment: ZIP file with two Unix shell CGI scripts

Comment 5

[neeti]

tever: could you setup these scripts on a server?

thanks,
Neeti

Comment 6

[neeti]

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

Comment 7

[John Morrison]

Reopening. The other bug does load infinitely, but it is a specific case
of a meta refresh request not being cancelled. It's not the general case 
bug for "redirect loops" are never limited. 

There used to be code in place that would detect a redirect loop (bug 24884).
Has that been disabled/broken/removed?

Comment 8

[neeti]

gagan: let me investigate this one as well...

Comment 9

[Gagan]

Neeti: If I understand correctly this is only suggesting a way to prevent badly
written loops with a certain limit. So there isn't much more investigation
left.. but we need to decide on that limit. My suggestion is that we put a pref
(defaulting to say 16) and just increment that count on the nsHttpChannel each
time we redirect. If it gets > 16 then we load the page without following the
redirect and alert the user with a likewise message. We'd need to add the
correcly localized dialog for that alert. All this work seems like this is going
to have to be a later milestone.

Comment 10

[tenthumbs]

Yes, that's exactly right. Mozilla is behaving correctly but
irrationally. I am suggesting that mozilla should be more clever,
behaving as SMTP daemons do when they detect mail loops.

Comment 11

[benc]

It's easier in SMTP, you just parse the Received headers to get the audit trail...

Comment 12

[tenthumbs]

SMTP daemons need headers because they don't control every connection.
Mozilla does. It knows whenever it redirects; it just has to remember
that long enough to be useful.

Comment 13

[Boris Zbarsky [:bzbarsky]]

*** Bug 99471 has been marked as a duplicate of this bug. ***

Comment 14

[gabriel]

Any progress on this yet ?

Comment 15

[Bradley Baetz (:bbaetz)]

*** Bug 105720 has been marked as a duplicate of this bug. ***

Comment 16

[Darin Fisher]

-> me

Comment 17

[Eric Hedström]

I just ran into this problem. If you go to www.stac.com and follow the link at
the bottom to www.previo.com it will put you into an endless loop. I let it go
for a while, and then tried to stop it. It wouldn't stop by hitting Escape, so I
tried to close that browser window. Crash!

Talkback ID is TB37820199E, build is 2001101909 on Win2k.

Platform/OS should be all/all.

Comment 18

[Darin Fisher]

Attachment: v1.0 patch

this patch adds a limit to the number of times a channel can be redirected.

since a new channel is created for each redirect, i had to add a method on
nsIHttpChannel to convey the current redirect count.

Comment 19

[Darin Fisher]

some comments from bbaetz:

1- nsHttpChannel::mRedirectionCount could be 8-bit

2- failure should trigger a special error code that could cause the uriloader
(or docshell) to pop up an alert.

Comment 20

[tenthumbs]

Some comments.

Should there be separate limits for a true loop as opposed to a long 
chain? ASP pages tend to do a lot of stupid things.

Does this apply to all URI's sent to a proxy server?

Wouldn't it be somewhat faster to set the redirection count to the 
current limit and then count down towards zero? Fewer function calls.

Comment 21

[Darin Fisher]

yeah, counting down sounds like a good idea.

this has nothing to do with proxy servers.  if a URL results in a redirect, then
the redirect count will be incremented (or decremented if we make the change you
suggested).  when the limit is reached, then we simply do not follow the redirect.

its tricky to detect true infinite redirects.  this would require tracking
cookies (and other perhaps unknown extensions to http) in the http code, which
the http code currently knows nothing about.

having a hard limit on the number of redirects just seems like a simple solution
to this problem... it avoids situations that would peg the users CPU, and i
think that's the real thing we're trying to do with this patch.

perhaps the limit of 10 redirects is too aggresive.  how about 20?  or 200?
it doesn't really matter.

Comment 22

[tenthumbs]

Proxy servers can siletly rewrite URIs so I was worried about what 
apepars to be an ftp URI receiving a redirection from some http server. 
If redirection limits apply to all URIs then it's not an issue.

As for the limit, lynx goes around the www.stac.com loop  twice before 
it notices. It would be nice if mozilla only did it once. However, 
setting the limit lower tahn 10 will probably catch dump ASP pages. No 
one seems to object to lynx's limit of 10. I would say keep it at that 
and see what happens.

Comment 23

[Darin Fisher]

when using FTP via a proxy we're actually speaking HTTP, so we'd still get the
redirection counting.

also, glad to hear that there is somewhat of a standard for limiting the number
of redirects to 10 :)

Comment 24

[Stephen Moehle]

*** Bug 112200 has been marked as a duplicate of this bug. ***

Comment 25

[Darin Fisher]

*** Bug 112310 has been marked as a duplicate of this bug. ***

Comment 26

[Darin Fisher]

Attachment: v1.1 patch

- added NS_ERROR_REDIRECT_LOOP to set of necko error codes.
- added code in nsWebShell.cpp for popping up an alert on
NS_ERROR_REDIRECT_LOOP
- made mRedirectionLimit 8-bit
- made mRedirectionLimit count down
- plus some other bits of cleanup

Comment 27

[Darin Fisher]

bbaetz: can you review this new patch?

Comment 28

[tenthumbs]

I suggest changing "cannot be loaded" to "will not be loaded". It's an 
administrative decision. 

I'm not opposed to a dialog box but I'm wondering if displaying a 
message in the status bar instead of/in addition to a dialog box might 
not be good; provided the status bar ever works.

Comment 29

[Darin Fisher]

"will not" almost sounds like a deficiency on our side, when in fact this is
likely a problem with the server.  a problem with the server is something we
"cannot" do anything about.  so, i think i prefer "cannot" though i'm open to
arguments against it and/or other suggestions :)

i agree that it might be nice to report the problem in the status bar... hmm...
actually, seems like it'd be nice to report any load failure in the status bar
instead of just "Document Done".  that's probably a separate bug.

see bug 112366, which has a fix for the status bar updating problems.

Comment 30

[Darin Fisher]

*** Bug 112906 has been marked as a duplicate of this bug. ***

Comment 31

[tenthumbs]

Well, I see "cannot" as meaning mozilla is unable to do something which 
isn't really true. Mozilla just reaches a limit. Maybe something like:
 "Redirection limit for this URL exceeded. Loading aborted."
but I like terse messages. :-)  In any event, I'm not violently opposed 
to the current message. It's also true that terse messages fit better 
into the status bar.

Comment 32

[Darin Fisher]

yeah i kinda like "Redirection limit for this URL exceeded. Loading aborted." too.

i'm not sure that "The URL has been redirected too many times and cannot be
loaded." is any clearer or less confusing for a novice user.

i tested IE6 and noticed that it has this bug as well... it totally clobbered my
webserver that hosts an infinite redirect testcase.  oddly enough however it
didn't seem to hurt the windows box too badly.

Comment 33

[Bradley Baetz (:bbaetz)]

Comment on attachment 59675 [details] [diff] [review]
v1.1 patch

>+      nsXPIDLString messageStr;
>+      rv = stringBundle->GetStringFromName(NS_LITERAL_STRING("redirectLoop").get(),
>+                                           getter_Copies(messageStr));
>+      if (NS_FAILED(rv)) return rv;
>+
>+      PRUnichar *msg = nsTextFormatter::smprintf(messageStr, (const char*)host);

What does this do? messageStr doesn't contain any format specifiers. Even if it
did, you
should be using formatStringFromName instead, which "uses
nsTextFormatter::smprintf to do the dirty work"
(This would then mean using the %S notation instead, in the format string)

Do we really need to print the url?

>+      if (!msg) return NS_ERROR_OUT_OF_MEMORY;
>+
>+      prompter->Alert(nsnull, msg);
>+      nsTextFormatter::smprintf_free(msg);
>+    }

<snip>

>     case 302:
>     case 307:
>         // these redirects can be cached (don't store the response body)
>-        if (mCacheEntry) {
>-            rv = InitCacheEntry();
>-            if (NS_SUCCEEDED(rv) && mCacheEntry) {
>-                // XXX we must open an output stream to force the cache service to
>-                // select a cache device for our entry -- bad cache service!!
>-                rv = mCacheEntry->GetTransport(getter_AddRefs(mCacheTransport));
>-                if (NS_SUCCEEDED(rv)) {
>-                    nsCOMPtr<nsIOutputStream> out;
>-                    rv = mCacheTransport->OpenOutputStream(0, PRUint32(-1), 0, getter_AddRefs(out));
>-                }
>-            }
>-            CloseCacheEntry(rv);
>-        }
>+        if (mCacheEntry)
>+            CloseCacheEntry(InitCacheEntry());
>+

This bit is related to that cache service bug which wsa fixed a while back,
isn't it? Have you checked
that these are still being cached?

It looks OK apart from that, but I haven't had time to test it.

Comment 34

[tenthumbs]

I think you really must show the URI in question (at least in a dialog 
box). It could be just one object of many on a page.

Comment 35

[Darin Fisher]

bbaetz: oops!  i just blindly copied the NS_ERROR_UNKNOWN_HOST case :-/

and, yeah gordon fixed the cache bug that required the extra code in
nsHttpChannel.  it works now.

tenthumbs: i agree that it would be nice to give some context to the URL, but
displaying an URL in an alert box could be problematic... what happens if the
URL is really long?  also, there could be multiple URLs involved in the infinite
redirect.  i didn't want to deal with these problems, and because it seems like
these kinds of problems occur mostly with toplevel documents, i figured there'd
be enough context.  i'm sure this is somewhat flawed but i really can't think of
an easy solution to the problem.  suggestions?

Comment 36

[Darin Fisher]

Attachment: v1.2 patch

revised per comments from bbaetz

Comment 37

[Bradley Baetz (:bbaetz)]

Comment on attachment 60236 [details] [diff] [review]
v1.2 patch

This looks fine to me. The error text may be a bit too technical, but I can't
think of another way to put it.

r=bbaetz

Comment 38

[tenthumbs]

darin: I don't think there is any good way to get all the context into 
the available places so it's doesn't seem worth the trouble of trying. 
What mozilla needs is a log window where all this stuff could be dumped, 
but that's not going to happen any time soon.

One small suggestion, though. In nsHttpChannel::ProcessResponse, a 
little after line 1145, where you have 
 LOG(("redirection limit reached!\n"));
I think you should include the URL here. At least someone might get to 
see it. :-)

Comment 39

[Darin Fisher]

tenthumbs: the LOG file already contains the URL for the channel, so no need to
print it out again.  also, i thought about this bug a bit more and i suspect
that we'll only get the dialog if it's the toplevel document that redirects
infinitely.  otherwise, we'll probably just get a blank frame, image, or
whatever without an alert dialog.  i think this is ok, cuz you wouldn't want to
put up tons of alerts all at once or all in sequence... that would just annoy
the user.

Comment 40

[tenthumbs]

darin: I generally find that the interesting parts of a log file are 
separated by hundreds of lines of junk so I like repetition. :-)

If toplevel document means resulf of user-initiated action then I agree 
that supressing some messages is ok (at least for dialog boxes, it would 
be different for the status bar). For example, if a user clicks on a 
link that says "click here for a larger image" then I would expect 
mozilla to put up an error if that URI redirected too much, no matter 
what the eventual mime type was. That wouldn't be necessary if the image 
was embedded in a page.

Comment 41

[rpotts (gone)]

hey darin,

this patch looks good to me... 

my only thought is whether 'redirectionLimit' really needs to be exposed through
nsIHTTPChannel...  i wonder if it wouldn't be enough to expose it via
nsIHttpProtocolHandler and just not allow an individual channel to have it's
'own' redirect limit...

-- rick

Comment 42

[rpotts (gone)]

Comment on attachment 60236 [details] [diff] [review]
v1.2 patch

sr=[e-mail]

Comment 43

[Darin Fisher]

rick: yeah, i don't like have redirectionLimit on nsIHttpChannel per se, but it
does need to be a method implemented by nsHttpChannel.  this is because each
nsHttpChannel can have a different redirect limit.  that is, each time we
redirect, the redirected channel gets a redirect limit that is one less than the
previous channels redirect limit.  if a channel's redirect limit is zero then it
cannot redirect.

Comment 44

[Darin Fisher]

rick: didn't realize you weren't cc'd... see comment #43.

also, i meant to add that i put redirectLimit on nsIHttpChannel because when we
redirect we don't know if the new channel is actually a http channel... it could
be a ftp channel for all we know.  so the new channel has to be QI'd to
nsIHttpChannel so we can set the new redirect limit.

Comment 45

[Darin Fisher]

fixed-on-trunk

Comment 46

[John Unruh]

Verified.

Comment 47

[benc]

john: did you use a test server for this? can you include a link or send me an
internal URL?

Comment 48

[John Unruh]

Test URl used from bug 99471 added to the test URL -
http://pytheas.dmw.ca/cgi-bin/redirectself