Closed Bug 767506 Opened 8 years ago Closed 4 years ago

Select proper proxy caps for non-SSL CONNECT

Categories

(Core :: Networking: HTTP, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED INCOMPLETE

People

(Reporter: jduell.mcbugs, Unassigned)

References

Details

(Whiteboard: [http-conn])

Attachments

(1 file, 1 obsolete file)

Not sure how much difference this makes: AFAICT mProxyCapabilities/mCapabilities are generally set to the same values.  Maybe Honza knows...
Attachment #635848 - Flags: review?(mcmanus)
Attachment #635848 - Flags: feedback?(honzab.moz)
Blocks: 767516
Depends on: 713026
Attachment #635848 - Flags: feedback?(honzab.moz)
Comment on attachment 635848 [details] [diff] [review]
v1: fix proxy capabilities logic for non-SSL CONNECT

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

This makes sense to me, but I'd rather honza r? it
Attachment #635848 - Flags: review?(mcmanus)
Attachment #635848 - Flags: review?(honzab.moz)
Attachment #635848 - Flags: feedback+
Whiteboard: [http-conn]
Whiteboard: [http-conn] → [http-conn]
Comment on attachment 635848 [details] [diff] [review]
v1: fix proxy capabilities logic for non-SSL CONNECT

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

This seems to be OK.

r=honzab

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ +1434,5 @@
> +    bool usingConnect = false;
> +    if (proxyInfo) {
> +        usingConnect = https;   // SSL always uses CONNECT
> +        PRUint32 resolveFlags = 0;
> +        if (NS_SUCCEEDED(proxyInfo->GetResolveFlags(&resolveFlags)) &&

Maybe don't do this when usingConnect is already true?

BTW, this effectively copies the code from nsHttpConnectionInfo ctor.  Would be nice to have this decision only on one place.  But up to you.
Attachment #635848 - Flags: review?(honzab.moz) → review+
Attached patch v2: fixes nitsSplinter Review
fixes Honza's nits.
Attachment #635848 - Attachment is obsolete: true
Attachment #638549 - Flags: review+
Unfortunately, this had to be backed out due to causing perma-orange in the websockets tests on all platforms. (Note that there are intermittent failures with the same signature, so be careful on Try when testing a new fix).
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7aa6473ce02

Example log:
https://tbpl.mozilla.org/php/getParsedLog.php?id=13175732&tree=Mozilla-Inbound
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.