801826 - duplicate request headers after doAuthRetry

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Patrick McManus [:mcmanus]]

Kerberos Windows auth takes 3 requests to get, which means 2 trips through nsHttpChannel::DoAuthRetry()

You can see from this log (taken coincidentally from bug 766817) that by the end we're sending 3 copies of Connection, Pragma, and Cache-Control.

Essentially that's because we merge those headers each time. I've seen the same thing with basic auth (only 2 deep).

Its not clear to me if we should be clearing all headers or just these particular ones in between each iteration. I'm going to start with the more limited patch and if someone feels strongly (and more confidently) then they should go ahead and clear them all.


12-06-26 09:26:58.734000 UTC - 0[200f140]:   GET /?encoding=text HTTP/1.1
2012-06-26 09:26:58.734000 UTC - 0[200f140]:   Host: echo.websocket.org
2012-06-26 09:26:58.734000 UTC - 0[200f140]:   User-Agent: Mozilla/5.0 (Windows NT 5.1; rv:14.0) Gecko/20100101 Firefox/14.0
2012-06-26 09:26:58.734000 UTC - 0[200f140]:   Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
2012-06-26 09:26:58.734000 UTC - 0[200f140]:   Accept-Language: en-us,en;q=0.5
2012-06-26 09:26:58.734000 UTC - 0[200f140]:   Accept-Encoding: gzip, deflate
2012-06-26 09:26:58.734000 UTC - 0[200f140]:   Connection: keep-alive, Upgrade, Upgrade, Upgrade
2012-06-26 09:26:58.734000 UTC - 0[200f140]:   Sec-WebSocket-Version: 13
2012-06-26 09:26:58.734000 UTC - 0[200f140]:   Origin: http://www.websocket.org
2012-06-26 09:26:58.734000 UTC - 0[200f140]:   Sec-WebSocket-Key: W6ULWH9TVksxiKe+dQHwAA==
2012-06-26 09:26:58.734000 UTC - 0[200f140]:   Cookie: __utma=9925811.1631343673.1340366976.1340691151.1340701243.6; __utmz=9925811.1340366976.1.1.utmcsr=(direct)|utmccn=(direct)|utmcmd=(none); __utmb=9925811.2.10.1340701243; BCSI-CS-F45F81F5DE06A0C3=2; __utmc=9925811
2012-06-26 09:26:58.734000 UTC - 0[200f140]:   Pragma: no-cache, no-cache, no-cache
2012-06-26 09:26:58.734000 UTC - 0[200f140]:   Cache-Control: no-cache, no-cache, no-cache
2012-06-26 09:26:58.734000 UTC - 0[200f140]:   Upgrade: websocket
2012-06-26 09:26:58.734000 UTC - 0[200f140]: ]

Comment 1

[Patrick McManus [:mcmanus]]

Attachment: patch 0

Comment 2

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

Comment on attachment 671577 [details] [diff] [review]
patch 0

be careful in case they were set by the necko user...

Comment 3

[Patrick McManus [:mcmanus]]

(In reply to Christian :Biesinger (don't email me, ping me on IRC) from comment #2)
> Comment on attachment 671577 [details] [diff] [review]
> patch 0
> 
> be careful in case they were set by the necko user...

that's why I limited myself to these particular headers.. on-modify-request users will get a second bite at the apple.

Comment 4

[Jason Duell]

Comment on attachment 671577 [details] [diff] [review]
patch 0

Keep an eye out for regressions here.  The other route I suppose is to check for the header value already existing when we try to set it, and refrain from setting twice if so.  But if you think this will work let's try it.

Comment 5

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

(In reply to Patrick McManus [:mcmanus] from comment #3)
> that's why I limited myself to these particular headers.. on-modify-request
> users will get a second bite at the apple.

Yes, but people can set those headers before AsyncOpen. I'm particularly concerned about XMLHttpRequest users here.

Comment 6

[Patrick McManus [:mcmanus]]

Attachment: patch 1

alright, we'll do this the explicit way.

Comment 7

[Patrick McManus [:mcmanus]]

biesi r? ping

Comment 8

[Jason Duell]

Comment on attachment 671810 [details] [diff] [review]
patch 1

Review of attachment 671810 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/nsHttpRequestHead.h
@@ +57,5 @@
> +    // Don't allow duplicate values
> +    nsresult SetHeaderOnce(nsHttpAtom h, const char *v, bool merge = false)
> +    {
> +        if (!merge || !HasHeaderValue(h, v))
> +            return mHeaders.SetHeader(h, nsDependentCString(v), merge);

Stupid question: should the behavior here just be the default for regular SetHeader()? When exactly do we want to support merging in a strncmp-able duplicate value to an HTTP header? I'd think never. So we could do this !HasHeaderValue check for all HTTP headers and avoid future errors. The systems programmer in me balks at the cost of extra processing, but I suppose it's a drop in the bucket for a modern CPU.  What do you think?

Comment 9

[Patrick McManus [:mcmanus]]

(In reply to Jason Duell (:jduell) from comment #8)
> Comment on attachment 671810 [details] [diff] [review]
> patch 1
> 
> Review of attachment 671810 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/http/nsHttpRequestHead.h
> @@ +57,5 @@
> > +    // Don't allow duplicate values
> > +    nsresult SetHeaderOnce(nsHttpAtom h, const char *v, bool merge = false)
> > +    {
> > +        if (!merge || !HasHeaderValue(h, v))
> > +            return mHeaders.SetHeader(h, nsDependentCString(v), merge);
> 
> Stupid question: should the behavior here just be the default for regular
> SetHeader()?

can't assume it is redundant out of context

"content-encoding: gzip, gzip" is a dumb thing to do but you better remove it twice to get the right answer

"via: foo, foo" implies a loop we want to know about

etc..

Comment 10

[Jason Duell]

> content-encoding: gzip, gzip

ah, right.  Ok then :)

Comment 11

[Patrick McManus [:mcmanus]]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/0c5a474a56b2

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0c5a474a56b2

Should this have a test?