This has been extracted from bug 45747. There is a patch for this submitted by ruslan: http://bugzilla.mozilla.org/showattachment.cgi?attach_id=15560 It is a little old, and so it may require some revision.

changed platform to all.

added RFE to summary.

http bugs to "Networking::HTTP"

nominating for moz 0.9

added keyword nsbeta1 (we have a patch that just needs to be resurected).

-> 0.9.1

my changes for bug 76866 will fix this bug as well.

-> 0.9.2 in favor of fixing other more important bugs for 0.9.1

-> moz1.0

nominating for 0.9.5, given the number of people using broken proxies (aka junkbuster)

-> 0.9.5

removing incorrect keywords.

-> future (this is only important to users who connect both via a proxy server and directly to web servers)

would love to squeeze this in for mozilla 1.0

updated to work with today's trunk :-)

Comment on attachment 79481 [details] [diff] [review] v2 patch r=brade tho I wish we had an answer for the XXX comment (ssl tunnel with proxy)

Comment on attachment 79481 [details] [diff] [review] v2 patch >Index: netwerk/protocol/http/src/nsHttpHandler.cpp >=================================================================== >RCS file: /cvsroot/mozilla/netwerk/protocol/http/src/nsHttpHandler.cpp,v >retrieving revision 1.54 >diff -u -r1.54 nsHttpHandler.cpp >--- netwerk/protocol/http/src/nsHttpHandler.cpp 22 Mar 2002 21:25:58 -0000 1.54 >+++ netwerk/protocol/http/src/nsHttpHandler.cpp 16 Apr 2002 19:11:52 -0000 >@@ -1292,14 +1292,17 @@ > else > mHttpVersion = NS_HTTP_VERSION_1_0; > } >+ } > >- if (mHttpVersion == NS_HTTP_VERSION_1_1) { >- mCapabilities = NS_HTTP_ALLOW_KEEPALIVE; >- mProxyCapabilities = NS_HTTP_ALLOW_KEEPALIVE; >- } >- else { >- mCapabilities = 0; >- mProxyCapabilities = 0; >+ if (PREF_CHANGED(HTTP_PREF("proxy.version"))) { >+ nsXPIDLCString httpVersion; >+ prefs->GetCharPref(HTTP_PREF("proxy.version"), getter_Copies(httpVersion)); >+ if (httpVersion) { >+ if (!PL_strcmp(httpVersion, "1.1")) >+ mProxyHttpVersion = NS_HTTP_VERSION_1_1; >+ else >+ mProxyHttpVersion = NS_HTTP_VERSION_1_0; >+ // it does not make sense to issue a HTTP/0.9 request to a proxy server > } > } where do you end up setting mCompabilities and mProxyCapabilities any more? > >@@ -1787,9 +1790,12 @@ > return NS_ERROR_OUT_OF_MEMORY; > NS_ADDREF(httpChannel); > >- nsresult rv = httpChannel->Init(uri, >- mCapabilities, >- proxyInfo); >+ // select proxy caps if using a non-transparent proxy or ssl tunnel >+ // XXX is this correct for ssl tunnel? >+ PRInt8 caps = (proxyInfo && !nsCRT::strcmp(proxyInfo->Type(), "http")) ? >+ mProxyCapabilities : mCapabilities; >+ >+ nsresult rv = httpChannel->Init(uri, caps, proxyInfo); > > if (NS_SUCCEEDED(rv)) > rv = httpChannel-> This is wrong for ssl tunnels - the CONNECT request will end up using mCapabilities instead od mProxyCapabilities. However, since we can't keep alive or pipeline the CONNECT request, I don't know if this matters.

bbaetz: mCapabilities and mProxyCapabilities are set when the corresponding prefs are read in and/or changed. these include "network.http.keep-alive" and friends. for SSL tunneling, the proxy info's type has a value of "http", as set via ExamineForProxy, so for SSL tunneling, we would use mProxyCapabilities, not mCapabilities. my XXX comment in the patch is to indicate that perhaps we shouldn't be using proxy capabilities when speaking to a SSL tunnel since after the first transaction it is going to be treated as a transparent proxy.

/me makes mental note to wake up before reviewing patches I misread the arguments for proxies, but if you add a SchemeIs("https") check, then my argument does hold, and I think it is OK. The overhead for CONNECT + ssl setup isn't low, and we should try to minimise this.

i agree.. we probably could trim the fat a bit on our CONNECT message, but let's save that for another bug ;-)

revised to use direct connection preferences for SSL tunneling. bbaetz convinced me that this makes perfect sense. also, i checked and the SSL CONNECT request is pretty trimmed down already.

Comment on attachment 79574 [details] [diff] [review] v3 patch This looks fine; I assume you've checked that this is correct, via packet sniffing. r=bbaetz

Comment on attachment 79658 [details] [diff] [review] v3.1 same thing + a little bit of cleanup i tested this patch w/ brade's latest patch for bug 136956 against a squid proxy. everything checks out :-) carrying forward r=brade,bbaetz

Comment on attachment 79481 [details] [diff] [review] v2 patch sr=rpotts@netscape.com

Comment on attachment 79658 [details] [diff] [review] v3.1 same thing + a little bit of cleanup carrying forward sr=rpotts

fixed-on-trunk

Comment on attachment 79658 [details] [diff] [review] v3.1 same thing + a little bit of cleanup a=chofmann for 1.0.1

checked with packet sniffer, was able to switch from HTTP/1.0 to HTTP/1.1 for all proxy transactions verified trunk - 06/04/02 builds - winNT4, linux rh6, mac osX

Changing to Mozilla1.0.1+ per comment #30 From chris hofmann. Resolving as fixedbecause it has been checked, and verified into the trunk adt1.0.1+ (on ADT's behalf) approval for checkin to the 1.0 branch. pls check this in asap, then add the "fixed1.0.1" keyword.

fixed

verified per comment #31

verified branch - 06/10/02 builds - win NT4, linux rh6, mac osX

removing fixed1.0.1 keyword