311595 - Cannot use bouncer to serve mar files

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Darin Fisher]

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.

Comment 1

[Darin Fisher]

Attachment: v1 patch

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.

Comment 2

[Darin Fisher]

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

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 198864 [details] [diff] [review]
v1 patch

seems to me like you should forward OnChannelRedirect even if there is no Range
header...

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

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...

Comment 6

[Darin Fisher]

> 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 7

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 198864 [details] [diff] [review]
v1 patch

OK... may want to add an NS_ASSERTION on that, then

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

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.

Comment 9

[Darin Fisher]

> 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.

Comment 10

[Scott MacGregor]

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

Comment 11

[Darin Fisher]

Comment on attachment 198864 [details] [diff] [review]
v1 patch

bring it!

Comment 12

[Darin Fisher]

fixed-on-trunk

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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)

Comment 14

[Darin Fisher]

fixed1.8 w/ spelling correction

spelling correction applied to the trunk as well.