there's no reason for redirection to be a HTTP specific thing. we should allow any protocol handler to redirect a channel to another protocol. this simply involves moving the onRedirect method from nsIHttpEventSink to some more generic interface. the resulting interface might be a good interface to consider freezing.
-> me I'll basically move onRedirect from nsIHttpEventSink to nsIRedirectObserver and change the argument to nsIChannel (rather than nsIHttpChannel). The question is.. Should it instead have a more generic nsIProtocolEventSink in case we want other kinds of notifications? I guess if we want to freeze it sometime soon, then it won't matter anyway.
how about nsIChannelEventSink? and maybe we should continue to support nsIHttpEventSink for a while longer.
nsIChannelEventSink::onChannelRedirect having "channel" in the method name helps when a class is implementing a dozen or so interfaces. also, we should freeze this for 1.8. i'm fine with it only having this one method unless someone can think of other events that might be useful. we should also freeze nsIProgressEventSink :)
Created attachment 176093 [details] [diff] [review] patch
So, it just occured to me this morning that we may want to add a flags parameter to onChannelRedirect that would indicate whether this is a temporary redirect (302, etc.) or a permanent redirect (301).
Comment on attachment 176093 [details] [diff] [review] patch It's important to document that throwing an exception from onChannelRedirect aborts the redirect. r=darin otherwise. if you want to just post a diffdiff for the revisions that'd be great.
Created attachment 176193 [details] [diff] [review] patch v2 great idea about adding the type of the redirect. I'm using a "flags" argument in this patch; this is probably a bit more extensible, but is it needed? Maybe a "type" argument would be easier to use. that failure of this method cancelled the load was mentioned in the last patch; I made that a bit clearer.
Even HTTP can do redirects back to the same URI. That happens when for example a site wanted a cookie to be set before proceeding. That scenario is what leads to that dialog about infinite redirect looping with a warning to the user that it may be due to their cookie blocking prefs. I would also prefer to see an explicit constant for REDIRECT_TEMPORARY (=0). Why flag internal redirects? Why not lump those with temporary redirects? I'm curious if you had something specific in mind. Or, is it just a matter of adding that because we can?
Comment on attachment 176193 [details] [diff] [review] patch v2 minusing in favor of my proposed changes
> Even HTTP can do redirects back to the same URI. oh, true, that's a good point. > I would also prefer to see an explicit constant for REDIRECT_TEMPORARY (=0). Well, I made this a flag (to be tested with &, not ==); so I didn't think a constant for TEMPORARY made sense. that said. changing this to just an enum would be fine with me - I'm not sure if a flag actually buys us anything. > Why flag internal redirects? I was thinking that implementors might want to differentiate between server-initiated redirects and those that are just implementation details. I did not think of a specific use case. should I remove it?
We already have a convention for zero-valued constants being defined for values that are a bit-field. See nsIRequest::LOAD_NORMAL. Ultimately, I'm hoping that we can tie the redirection type into session history, to affect URL bar rewriting. It might be useful to leverage that mechanism even in cases where there aren't server initiated redirects but rather implementation details. It'll give us more flexibility. So, that argues for either getting rid of the internal flag or allowing that bit to be set in conjunction with temporary or permanent. Maybe temporary should just have its own bit. And, let a value of 0 have no meaning.
well, the interface in this patch doesn't prohibit setting both REDIRECT_INTERNAL and REDIRECT_PERMANENT... the docs do assume that internal redirects only change the channel instance, not its URI. maybe that's a bad assumption. Hm... a real flag for TEMPORARY would probably make code clearer which tests for that condition. > Ultimately, I'm hoping that we can tie the redirection type into session > history, to affect URL bar rewriting. so temporary redirects would not affect the displayed URL? that might have security implications... (displayed url is not what actually got displayed)
Created attachment 176523 [details] [diff] [review] patch v3 - add a define for REDIRECT_TEMPORARY (defined to 1) - change REDIRECT_PERMANENT to 2 - document that one of them will always be set - let REDIRECT_INTERNAL change the URI if desired
(In reply to comment #13) > well, the interface in this patch doesn't prohibit setting both > REDIRECT_INTERNAL and REDIRECT_PERMANENT... the docs do assume that internal > redirects only change the channel instance, not its URI. maybe that's a bad > assumption. Yeah, I don't see this being all that valuable. Someone could always check the URIs if needed. > Hm... a real flag for TEMPORARY would probably make code clearer which tests for > that condition. Agreed. > > Ultimately, I'm hoping that we can tie the redirection type into session > > history, to affect URL bar rewriting. > > so temporary redirects would not affect the displayed URL? that might have > security implications... (displayed url is not what actually got displayed) The idea is that the site you think you are going to (the one displayed in the URL) is the one controlling the redirect. So if you trust the site you think you're at then you trust whereever it sends you. Afterall, the site could achieve the same result using a <frameset>. So, I think it would be better to fix the long standing bug that browser history systems should not reflect the result of a temporary redirect. I think there is a bug on this somewhere. For example, have you ever noticed that navigating to www.hotmail.com results in some weird hostname showing up in the URL bar? That's because of this problem. It'd be nicer for users if it still said www.hotmail.com :-)
Comment on attachment 176523 [details] [diff] [review] patch v3 r=darin
Comment on attachment 176523 [details] [diff] [review] patch v3 sr=bzbarsky. Should we change chrome/jar channels to fire such "internal" redirects when they open the "real" channel?
checked in. Do they change their URI when they do that? The instance stays the same, right?
> Do they change their URI when they do that? Absolutely. I meant chrome/resource channels. And they change the URI to a jar URI. But yes, the channel instance does not change.... The resource: protocol handler just resolves the URI and then calls newChannel() on that URI. chrome: does the same, unless it's a cached chrome load; then it uses a custom channel impl that doesn't actually implement nsIChannel right... ;) Perhaps this code should in fact implement custom channels? More on this below. To be more clear, I want to eliminate the code at http://lxr.mozilla.org/seamonkey/source/content/base/src/nsDocument.cpp#704 (which basically special-cases some schemes so that we don't get the post-redirect URI for them). It seems to me this could be addressed by listening for redirects in the URILoader and then passing whatever data we garnered from that listener to DoContent (or its replacement on nsIURIContentListener2?). I guess I should file a new bug on this, eh? ;)
This might be a bigger change then we want to do, or it might be a bad idea for some other reason, but i've always thought that our current redirect-blocking is a bit reversed. Wouldn't it be better to block redirection by default, and allow some way (through nsIChannelEventSink or otherwise) to turn it on? That way we don't have to constantly be on the lookout for security holes...
I filed Bug 285192 on sending onChannelRedirect for chrome/resource