1477592 - CONNECT-only HTTP channel should not make TCP connection for non-http proxies

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement, P5)

Description

[Paul Vitale [:iidebyo]]

Currently when using an HTTP channel with CONNECT-only set detection of non-http proxies is done after the initial TCP connection.  There should be no TCP connection if there a non-http proxy is being used.  non-http is inclusive of no proxy.

Comment 1

[Paul Vitale [:iidebyo]]

Attachment: Prevent TCP connection for CONNECT-only HTTP channel with non-http proxy

This moves the check for a non-http proxy from the socket transport callbacks in
nsHttpConnection to nsHttpChannel before the connection occurs.

The unit test for the no-proxy case no longer supports accepting the connection.

Comment 2

[Paul Vitale [:iidebyo]]

Created as follow-up to https://bugzilla.mozilla.org/show_bug.cgi?id=1203503#c99 (Bug 1203503 comment 99).

Comment 3

[u408661]

Thanks! Please find an appropriate reviewer and request review from them to move this patch forward. Someone you were working with on the previous bug is a good first approximation of the right reviewer.

Comment 4

[Paul Vitale [:iidebyo]]

Attachment: Prevent TCP connection for CONNECT-only HTTP channel with non-http proxy

This moves the check for a non-http proxy from the socket transport callbacks in
nsHttpConnection to nsHttpChannel before the connection occurs.

Comment 5

[Dragana Damjanovic [:dragana]]

sorry for a delay. :( This patch will need rebasing :(

Comment 6

[Paul Vitale [:iidebyo]]

This patch auto-merged for me.  It's dependent on bug 1203503 's attachment "part 1. change necko to allow CONNECT-only requests."

:mayhemer, maybe we can merge this back into the other bug since you're back and I'm waiting on EF atm?

Comment 7

[Honza Bambas (:mayhemer)]

(In reply to Paul Vitale [:iidebyo] from comment #6)
> This patch auto-merged for me.  It's dependent on bug 1203503 's attachment
> "part 1. change necko to allow CONNECT-only requests."
> 
> :mayhemer, maybe we can merge this back into the other bug since you're back
> and I'm waiting on EF atm?

Not until this patch is r+ and ready to land.  Also, there is no harm done if this remains separate.  This patch has no effect on the functionality IIUC, hence, no need to push on it.

Comment 8

[Dragana Damjanovic [:dragana]]

(In reply to Paul Vitale [:iidebyo] from comment #6)
> This patch auto-merged for me.  It's dependent on bug 1203503 's attachment
> "part 1. change necko to allow CONNECT-only requests."
> 
> :mayhemer, maybe we can merge this back into the other bug since you're back
> and I'm waiting on EF atm?

It does not compile with this patch. NS_HTTP_CONNECT_ONLY does not exist any more.

Comment 9

[Paul Vitale [:iidebyo]]

(In reply to Dragana Damjanovic [:dragana] from comment #8)
> (In reply to Paul Vitale [:iidebyo] from comment #6)
> > This patch auto-merged for me.  It's dependent on bug 1203503 's attachment
> > "part 1. change necko to allow CONNECT-only requests."
> > 
> > :mayhemer, maybe we can merge this back into the other bug since you're back
> > and I'm waiting on EF atm?
> 
> It does not compile with this patch. NS_HTTP_CONNECT_ONLY does not exist any
> more.

You'll need to import the "part 1" patch before this patch to get it to compile.  I gave it a run through try but I may have missed a platform.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=27317b7122e38bde2152e0ec6d9698e3154a3386 (part 2 is out of order but it shouldn't matter)

Comment 10

[Dragana Damjanovic [:dragana]]

(In reply to Paul Vitale [:iidebyo] from comment #9)
> (In reply to Dragana Damjanovic [:dragana] from comment #8)
> > (In reply to Paul Vitale [:iidebyo] from comment #6)
> > > This patch auto-merged for me.  It's dependent on bug 1203503 's attachment
> > > "part 1. change necko to allow CONNECT-only requests."
> > > 
> > > :mayhemer, maybe we can merge this back into the other bug since you're back
> > > and I'm waiting on EF atm?
> > 
> > It does not compile with this patch. NS_HTTP_CONNECT_ONLY does not exist any
> > more.
> 
> You'll need to import the "part 1" patch before this patch to get it to
> compile.  I gave it a run through try but I may have missed a platform.
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=27317b7122e38bde2152e0ec6d9698e3154a3386 (part 2 is
> out of order but it shouldn't matter)

ok. A note about this would have been good... :)

Comment 11

[Paul Vitale [:iidebyo]]

(In reply to Dragana Damjanovic [:dragana] from comment #10)
> (In reply to Paul Vitale [:iidebyo] from comment #9)
> > (In reply to Dragana Damjanovic [:dragana] from comment #8)
> > > (In reply to Paul Vitale [:iidebyo] from comment #6)
> > > > This patch auto-merged for me.  It's dependent on bug 1203503 's attachment
> > > > "part 1. change necko to allow CONNECT-only requests."
> > > > 
> > > > :mayhemer, maybe we can merge this back into the other bug since you're back
> > > > and I'm waiting on EF atm?
> > > 
> > > It does not compile with this patch. NS_HTTP_CONNECT_ONLY does not exist any
> > > more.
> > 
> > You'll need to import the "part 1" patch before this patch to get it to
> > compile.  I gave it a run through try but I may have missed a platform.
> > 
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=27317b7122e38bde2152e0ec6d9698e3154a3386 (part 2 is
> > out of order but it shouldn't matter)
> 
> ok. A note about this would have been good... :)

Yep, sorry about that. Any guidance on a try server run for this patch? Like, platforms/all tests?

Comment 12

[Dragana Damjanovic [:dragana]]

(In reply to Paul Vitale [:iidebyo] from comment #11)
> (In reply to Dragana Damjanovic [:dragana] from comment #10)
> > (In reply to Paul Vitale [:iidebyo] from comment #9)
> > > (In reply to Dragana Damjanovic [:dragana] from comment #8)
> > > > (In reply to Paul Vitale [:iidebyo] from comment #6)
> > > > > This patch auto-merged for me.  It's dependent on bug 1203503 's attachment
> > > > > "part 1. change necko to allow CONNECT-only requests."
> > > > > 
> > > > > :mayhemer, maybe we can merge this back into the other bug since you're back
> > > > > and I'm waiting on EF atm?
> > > > 
> > > > It does not compile with this patch. NS_HTTP_CONNECT_ONLY does not exist any
> > > > more.
> > > 
> > > You'll need to import the "part 1" patch before this patch to get it to
> > > compile.  I gave it a run through try but I may have missed a platform.
> > > 
> > > https://treeherder.mozilla.org/#/
> > > jobs?repo=try&revision=27317b7122e38bde2152e0ec6d9698e3154a3386 (part 2 is
> > > out of order but it shouldn't matter)
> > 
> > ok. A note about this would have been good... :)
> 
> Yep, sorry about that. Any guidance on a try server run for this patch?
> Like, platforms/all tests?

you can do xpcshell test, I think that is enough. platforms: all although this is rather platform independent so linux is enough.

Comment 13

[Paul Vitale [:iidebyo]]

Try server run from a bit ago.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ff569273e962346ab6b574ad25d16cef99fa0a9

Comment 14

[Paul Vitale [:iidebyo]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a032d5bb2e446f90eb235c37d760678eee361781

Comment 15

[Paul Vitale [:iidebyo]]

Comment on attachment 8996850 [details] [diff] [review]
Prevent TCP connection for CONNECT-only HTTP channel with non-http proxy

This patch should be ready for check-in.

Comment 16

[Dragana Damjanovic [:dragana]]

you can just add checkin-needed in 'Keywords:'

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/298abb6be2dc
Prevent TCP connection for CONNECT-only HTTP channel with non-http proxy r=dragana

Comment 18

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/298abb6be2dc