Cannot use bouncer to server mar files The incremental downloader cannot handle HTTP redirects. The problem is that when nsHttpChannel.cpp follows a redirect, it does not propogate any user-defined headers. In this case, it is the Range header that is so crucial. Without it, we get a 200 response, which triggers bug 306170. An easy solution would be to intercept OnChannelRedirect, and then add the Range header on the newly created channel. It might also make sense to just make nsHttpChannel.cpp do this by default since this problem also likely impacts the Adobe Acrobat plugin as well as XMLHttpRequest consumers that need to set request headers.

Unfortunately, there is nothing in RFC 2616 (that I could find) that says that request headers should be forwarded to the new HTTP request when following a redirect. So, I think it is best to fix this bug by patching nsIncrementalDownload.cpp. We'll probably want to apply a similar patch to the plugin code that deals with range headers for plugins.

I filed bug 311599 for doing something similar for nsPluginHostImpl.cpp to help out Adobe Acrobat.

Comment on attachment 198864 [details] [diff] [review] v1 patch seems to me like you should forward OnChannelRedirect even if there is no Range header...

Also, doesn't this return error codes from OnChannelRedirect way too often? They cancel the redirect, don't they?

I'm not sure that RFC 2616 is too relevant here. It seems to me like this is mostly an issue of what the code calling setRequestHeader expects. I would assume that most callers expect that their explicitly set request headers are carried on to the redirected requests. note also bug 238144 comment 1. Seems like we need to patch plugins code, incremental download, and xmlhttprequest; patching httpchannel may be the better choice...

> seems to me like you should forward OnChannelRedirect even if there is no > Range header... ... > Also, doesn't this return error codes from OnChannelRedirect way too often? > They cancel the redirect, don't they? If there is no Range header in the request, then something has gone terribly wrong, and it is time to error out. This code wants to fail if something goes wrong with the Range header. > I'm not sure that RFC 2616 is too relevant here. It seems to me like this is > mostly an issue of what the code calling setRequestHeader expects. I would > assume that most callers expect that their explicitly set request headers are > carried on to the redirected requests. > > note also bug 238144 comment 1. > > Seems like we need to patch plugins code, incremental download, and > xmlhttprequest; patching httpchannel may be the better choice... Right, but... in general, though, it's hard to know if a range request makes sense on the URI that we've been redirected to. The range values apply to the entity being initially requested. The redirect may result in an entirely different entity such that the range request might make no sense. We should definitely test what IE, Safari, and Opera do when their XMLHttpReqest objects encounter a redirect on a request containing user defined headers. I'd be surprised if they automatically forward all user-defined headers to the redirect request. Maybe they only forward select headers? If they have the same "bug", then I'd think it might make sense for us to introduce an "onredirect" event for XMLHttpRequest. At any rate, fixing this in HTTP channel seems more risky to me, and for firefox 1.5, I think we want to go the safer route.

Comment on attachment 198864 [details] [diff] [review] v1 patch OK... may want to add an NS_ASSERTION on that, then

however, I want to reply to this too: (In reply to comment #6) > Right, but... in general, though, it's hard to know if a range request makes > sense on the URI that we've been redirected to. The range values apply to the > entity being initially requested. The redirect may result in an entirely > different entity such that the range request might make no sense. I am unconvinced that a 301/302 response can redirect to a different resource. Consider the RFC 2616 text: 10.3.2 301 Moved Permanently The requested resource has been assigned a new permanent URI and any future references to this resource SHOULD use one of the returned URIs. So the resource is identical, just its address changed. Similar for 302: 10.3.3 302 Found The requested resource resides temporarily under a different URI. > We should definitely test what IE, Safari, and Opera do when their XMLHttpReqest > objects encounter a redirect on a request containing user defined headers. I'd > be surprised if they automatically forward all user-defined headers to the > redirect request. Maybe they only forward select headers? If they have the > same "bug", then I'd think it might make sense for us to introduce an > "onredirect" event for XMLHttpRequest. Yeah, that does need testing... > At any rate, fixing this in HTTP channel seems more risky to me, and for firefox > 1.5, I think we want to go the safer route. Yes, indeed, but for trunk we should reconsider this.

> I am unconvinced that a 301/302 response can redirect to a different > resource. > Consider the RFC 2616 text: > 10.3.2 301 Moved Permanently > The requested resource has been assigned a new permanent URI and any > future references to this resource SHOULD use one of the returned > URIs. > > So the resource is identical, just its address changed. Similar for 302: > 10.3.3 302 Found > The requested resource resides temporarily under a different URI. It's hard to know why a server would issue a redirect. What if a POST request is redirected? Do we forward headers in that case? Maybe the server wants to redirect every request for a resource to a login page when the request is lacking the right cookie. In that case, the entity being requested originally is not the entity resulting from the redirect.

Can we get this into the trunk and nominated for RC1?

Comment on attachment 198864 [details] [diff] [review] v1 patch bring it!

fixed-on-trunk

Comment on attachment 198864 [details] [diff] [review] v1 patch >+ // In response to a redirect, we need to propogate the Range header. See bug spelling nit: propagate (brendan called me (and waterson) on that one a few times)

fixed1.8 w/ spelling correction spelling correction applied to the trunk as well.