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

RESOLVED FIXED in Firefox 65

Status

()

enhancement
P5
normal
RESOLVED FIXED
10 months ago
6 months ago

People

(Reporter: iidebyo, Assigned: iidebyo)

Tracking

unspecified
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

(Whiteboard: [necko-triaged])

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

10 months ago
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.
Assignee

Comment 1

10 months ago
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 3

10 months ago
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.
Flags: needinfo?(paul.m.vitale)
Priority: -- → P5
Whiteboard: [necko-triaged]
Assignee

Comment 4

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

Updated

10 months ago
Attachment #8994050 - Attachment is obsolete: true
Flags: needinfo?(paul.m.vitale)
Assignee

Updated

10 months ago
Attachment #8996850 - Flags: review?(dd.mozilla)
sorry for a delay. :( This patch will need rebasing :(
Flags: needinfo?(paul.m.vitale)
Assignee

Comment 6

9 months ago
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?
Flags: needinfo?(paul.m.vitale) → needinfo?(honzab.moz)
(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.
Assignee: nobody → paul.m.vitale
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(honzab.moz)
Depends on: 1203503
(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.
Assignee

Comment 9

9 months ago
(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)
(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... :)
Attachment #8996850 - Flags: review?(dd.mozilla) → review+
Assignee

Comment 11

9 months ago
(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?
(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.
Assignee

Comment 15

6 months ago
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.
Attachment #8996850 - Flags: checkin?(dd.mozilla)
you can just add checkin-needed in 'Keywords:'
Attachment #8996850 - Flags: checkin?(dd.mozilla)

Comment 17

6 months ago
Pushed by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/298abb6be2dc
Prevent TCP connection for CONNECT-only HTTP channel with non-http proxy r=dragana
Keywords: checkin-needed

Comment 18

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/298abb6be2dc
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.