449251 - Connection/Proxy: "Use this proxy server for all protocols" should not configure SOCKS

Status: NEW

(Core :: Networking: Proxy, defect, P3)

Description

[benc]

(I dupe checked by reading all new FF bugs w/ "proxy" or "connection" in the summary).

STEPS:

Preferences > Advanced > Network > Connection "Settings" button.

Enter an http proxy, for example "httpproxy".

Click on the checkbox "Use this proxy server for all protocols"

OBSERVED:

The string copy and enable disable logic affects all manual proxies below it.

EXPECTED:

Socks should not be affected.

HTTP, FTP, Gopher, SSL proxies use the same proxy client-server protocol: a URL proxy over HTTP. SOCKS is a completely different protocol, the socks server listens on a different port, and of the two products I know of that support both "HTTP Proxy" (MS ISA and Sun/iPlanet/Netscape Proxy Server), neither of them supports a combination HTTP/SOCKS proxy on a single port.

So, the Firefox behavior, which is to copy the HTTP proxy into all fields, that is incorrect.

This bug is for the checkbox/form-filling behavior only. If we fix this, we can worry about re-factoring the visual elements of the panel in another bug. The checkins could be carpooled, or done in series.

In other words, someone's socks server would never run on the same port as the http proxy. And there is no reliable way of assuming that the hostname would ever be the same, so we shouldn't copy anything to SOCKS.

COMPARISON:

In seamonkey, there is a manual proxy checkbox called:

"Use HTTP Proxy settings for all protocols"

This check box configures everything EXCEPT SOCKS.

Here's the link to the code:

http://mxr.mozilla.org/seamonkey/source/suite/common/pref/pref-proxies-advanced.js#125

125 function DoProxyCopy()

It copies: ftp, gopher, ssl.

There is also some panel layout that calls out that SOCKS is different than the protocol-specific proxies.

Comment 1

[benc]

PROPOSED fix:

There are a couple things that need to be done:

http://mxr.mozilla.org/seamonkey/source/browser/components/preferences/connection.js

133   updateProtocolPrefs: function ()
remove "socks" from here:

137     var proxyPrefs = ["ssl", "ftp", "socks", "gopher"];

remove these lines:

161     var socksVersionPref = document.getElementById("network.proxy.socks_version");
162     socksVersionPref.disabled = proxyTypePref.value != 1 || shareProxiesPref.value;

There might be more polish needed as well, I haven't reviewed all the JS for this panel yet.

Also, it looks like the updateProtocolPrefs functionality might be needlessly duplicated in:

40   beforeAccept: function ()

Comment 2

[benc]

-> me.

Besides removing the reference, there needs to be some extra work done to make sure the socks preferences are enabled and disabled properly. This is possibly due to the fact that the onsyncfrompreference handlers might be inconsistent/redundant w/ the use of updateProtocolPrefs().

I should have a patch soon.

Comment 3

[Michael Kohler [:mkohler]]

This is a mass change. Every comment has "assigned-to-new" in it.

I didn't look through the bugs, so I'm sorry if I change a bug which shouldn't be changed. But I guess these bugs are just bugs that were once assigned and people forgot to change the Status back when unassigning.

Comment 4

[Jason Duell]

OK, this actually matters more now, since the Web Sockets spec has us try SOCKS proxies first (i.e. we set nsIProtocolProxyService::RESOLVE_PREFER_SOCKS_PROXY: the first actual use in the tree AFAICT).  So when the dialog for "use this (HTTP) proxy for all protocols" writes in the HTTP proxy host/port to the SOCKS setting, we try to talk SOCKS to an HTTP proxy and all websocket connections fail.  (see bug 713023).

Comment 5

[Jason Duell]

cc-ing Mark Banner in case there are Thunderbird issues for the fix here (tbird uses the exact same dialog).

Comment 6

[Jason Duell]

Attachment: v1. Disable SOCKS while "use HTTP proxy for all protocols" is set

OK, here's one way to fix this:  I explicitly set the SOCKS prefs to ""/0 (i.e. no SOCKS proxy) and grey them out when "use this (HTTP) proxy for all protocols" is set.  We remember any SOCKS proxy value that was previously set, and restore it if/when we no longer use the HTTP proxy for all protocols.

Why disable SOCKS, instead of just letting it cohabitate with HTTP?  Well, because 1) the wording does after all say "use the HTTP proxy for all protocols", so it logically doesn't make sense for SOCKS to matter; and 2) the only protocol that would still use SOCKS in this case would be web sockets, which is massively unintuitive (AFAIK: I don't know how the mail protocols interact with the settings here: standard8, do you know? bug 317248 makes it sound like we're somehow messing with IMAP too)

If we want/need to support simultaneous "HTTP for everything" + SOCKS usage, I think we need to be a lot clearer in the dialog about what's going to use SOCKS and what will use the HTTP proxy.  I expect that cleanup would take some time, so I'm in favor of landing this, which seems at least an incremental improvement.

Comment 7

[Jason Duell]

BTW, one can set both HTTP/SOCKS proxies currently, by setting them each manually (and not checking "use HTTP for all protocols").  But I'm not clear on any protocols that would then use SOCKS for anything, except web sockets (and perhaps mail?).   But users who want this option can still do it with my patch.

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 633360 [details] [diff] [review]
v1. Disable SOCKS while "use HTTP proxy for all protocols" is set

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

Makes sense. r+ on the code change, though it's really begging for a test. We barely have any tests for prefs code (and AFAIK, none for this dialog) - really need to change that, especially given the upcoming planned reorganisation of prefs.

Comment 9

[Jason Duell]

Comment on attachment 633360 [details] [diff] [review]
v1. Disable SOCKS while "use HTTP proxy for all protocols" is set

Got OK from biesi on IRC to make this change.

Comment 10

[Jason Duell]

Comment on attachment 633360 [details] [diff] [review]
v1. Disable SOCKS while "use HTTP proxy for all protocols" is set

https://hg.mozilla.org/integration/mozilla-inbound/rev/92e3821d85c2

Without this simple config UI patch, websockets are completely broken for users who have the "use the HTTP proxy for all protocols" option checked in their preferences (see bug 713023).

[Approval Request Comment]
Bug caused by (feature/regressing bug #): long-standing bug that websockets has exposed.
Testing completed (on m-c, etc.): manually tested, but fairly extensively.
Risk to taking this patch (and alternatives if risky): fairly low IMO.
String or UUID changes made by this patch: none

Comment 11

[Alex Keybl [:akeybl]]

(In reply to Jason Duell (:jduell) from comment #10)
> Risk to taking this patch (and alternatives if risky): fairly low IMO.

* When/how would you expect a regression to show up after taking this on Aurora or Beta?
* It looks like bug 713023 has only really been reported by one user. Why do you think this is critical?

We've already gone to build with our third desktop beta for FF14, so we're only really inclined to take on Aurora at this point (if at all).

Comment 12

[Jason Duell]

> When/how would you expect a regression to show up after taking this on Aurora or Beta?

Well, I don't expect a regression, so I don't know how to answer.  It's a fairly simple fix--the previous behavior to set SOCKS proxy to the HTTP proxy's host/port basically didn't make any sense, but wasn't getting used until web sockets.

> Why do you think this is critical?

Because it makes web sockets totally unusable for a class of user.  It's true we don't know what % of users have set the "use HTTP proxy for all protocols" checkbox.  But none of them are going to use web sockets until the patch lands.  It's possible that this is a tiny % of users, and web sockets aren't prevalent enough on the web yet that this will be a big deal.  

> so we're only really inclined to take on Aurora at this point 

I'm fine with whatever risk assessment decision you make.

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/92e3821d85c2

Comment 15

[Kim Sullivan]

For what its worth, this apparently affects users of StackOverflow who are using an proxy configured by a script (i.e. me). There are several duplicate bug reports on stack overflow meta regarding this issue, e.g. http://meta.stackoverflow.com/questions/130053/my-corporate-proxy-doesnt-support-web-sockets-which-makes-se-a-pain-to-use

Thanks for fixing this, cant wait for the fix propagate to beta.

Comment 16

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

Comment on attachment 633360 [details] [diff] [review]
v1. Disable SOCKS while "use HTTP proxy for all protocols" is set

This will not help people who have already set their proxy, right? I guess that is nothing we can fix easily :(

Comment 17

[Alex Keybl [:akeybl]]

Comment on attachment 633360 [details] [diff] [review]
v1. Disable SOCKS while "use HTTP proxy for all protocols" is set

[Triage Comment]
Approving for Aurora 15, we'll make a final decision for Beta 14 ahead of our next go to build on Tuesday.

Comment 18

[Jason Duell]

Kim:  the StackOverflow issue you're describing is likely to be a different bug.  I've filed bug 766817 for it--could you please comment in there with a URI that shows the issue?  Thanks.

Comment 19

[Jason Duell]

Backed out:

  https://hg.mozilla.org/mozilla-central/rev/4d0aa137268b

Alas, this patch is broken.  First, it doesn't fix the problem unless users actually revisit the Prefs | Advanced | Networking | Connection dialog as biesi points out.  Second, I've just discovered that if you already have "Use this proxy for all protocols" set, and you upgrade, go to the dialog, and save again, we wind up saving the old, bogus SOCKS setting (set to the HTTP proxy) to backup.socks, so if you ever later uncheck "Use this proxy for all protocols" it "restores" a bogus SOCK server value.

I'm going to go with a much simpler and less risky fix for aurora/beta in bug 713023, and then fix this properly on trunk.

Comment 20

[Jason Duell]

Attachment: part 1, v1: necko change to clear bogus SOCKS settings on first usage

biesi:

> This will not help people who have already set their proxy, right? 
> I guess that is nothing we can fix easily 

If we see SOCKS proxy host/port == HTTP proxy host/port, and "use this proxy for all protocols is set", we know that we've got a bogus setting from the GUI for SOCKS.   We may also have stored that value in the 'backup' SOCKS prefs (see bug 767047).  I think we can just clear them when we see this.

This isn't a complete fix for this bug--some GUI work still needed too.  That patch coming soon.

Comment 21

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

Comment on attachment 635399 [details] [diff] [review]
part 1, v1: necko change to clear bogus SOCKS settings on first usage

+                          shareProxy);

ew, references for out params :(


+            mSOCKSProxyHost.SetLength(0);

SetLength(0) -> Truncate()

Comment 22

[Valentin Gosu [:valentin] {{ PTO 2 - 12 April }}]

Not sure this patch is actually needed - it's been 7 years without landing, so probably not.
Putting it in the backlog just in case.

Comment 23

[BMO Automation]

Moving bug to Core/Networking: Proxy.