Last Comment Bug 485764 - Implement Windows System Proxy Settings Service
: Implement Windows System Proxy Settings Service
Status: RESOLVED FIXED
: pp
Product: Core
Classification: Components
Component: Networking (show other bugs)
: Trunk
: x86 Windows Vista
: -- normal (vote)
: mozilla1.9.2a1
Assigned To: Mitchell Field [:Mitch]
:
: Patrick McManus [:mcmanus]
Mentors:
http://mxr.mozilla.org/mozilla-centra...
: 386911 524272 (view as bug list)
Depends on:
Blocks: 139393 534917
  Show dependency treegraph
 
Reported: 2009-03-29 00:59 PDT by Mitchell Field [:Mitch]
Modified: 2010-04-29 04:07 PDT (History)
6 users (show)
ted: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1.0 (16.13 KB, patch)
2009-03-29 00:59 PDT, Mitchell Field [:Mitch]
no flags Details | Diff | Splinter Review
Patch v1.1 (19.21 KB, patch)
2009-03-31 20:34 PDT, Mitchell Field [:Mitch]
no flags Details | Diff | Splinter Review
Simpler *-matching code (1.51 KB, text/html)
2009-03-31 21:24 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details
Patch v1.2 (15.61 KB, patch)
2009-04-05 16:46 PDT, Mitchell Field [:Mitch]
no flags Details | Diff | Splinter Review
Patch v1.3 (15.49 KB, patch)
2009-04-05 23:50 PDT, Mitchell Field [:Mitch]
roc: review+
roc: superreview+
Details | Diff | Splinter Review
Patch v1.4 (16.78 KB, patch)
2009-04-22 21:04 PDT, Mitchell Field [:Mitch]
no flags Details | Diff | Splinter Review
Patch v1.5 (16.65 KB, patch)
2009-04-24 06:01 PDT, Mitchell Field [:Mitch]
no flags Details | Diff | Splinter Review
Patch v1.6 (16.55 KB, patch)
2009-04-25 00:18 PDT, Mitchell Field [:Mitch]
roc: review+
roc: superreview+
Details | Diff | Splinter Review

Description Mitchell Field [:Mitch] 2009-03-29 00:59:47 PDT
Created attachment 369867 [details] [diff] [review]
Patch v1.0

The URL field contains a link to the nsISystemProxySettings interface definition, and the following are implementations for Unix(-like) and Mac OS X, respectively.

http://mxr.mozilla.org/mozilla-central/source/toolkit/system/unixproxy/nsUnixSystemProxySettings.cpp

http://mxr.mozilla.org/mozilla-central/source/toolkit/system/osxproxy/nsOSXSystemProxySettings.mm

There is currently no such implementation for Windows, and this patch against mozilla-central adds one.
Comment 1 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-03-29 16:09:59 PDT
Why can't we use the Registry APIs directly here?

Ted, will this Makefile force the creation of a shared library in libxul builds?
Comment 2 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-03-29 20:27:52 PDT
Can you add a comment with a URL to some documentation for these registry entries?

Is there really no way for ProxyServer to specify protocol-specific proxies and a catch-all proxy?

+    nsAutoString buf;
+    nsCAutoString cbuf;
+
+    rv = mRegKey->ReadStringValue(NS_LITERAL_STRING("ProxyOverride"), buf);
+    if (NS_FAILED(rv))
+        return PR_FALSE;
+
+    cbuf = NS_ConvertUTF16toUTF8(buf);

Simpler to write

+    nsAutoString buf;
+    rv = mRegKey->ReadStringValue(NS_LITERAL_STRING("ProxyOverride"), buf);
+    if (NS_FAILED(rv))
+        return PR_FALSE;
+
+    NS_ConvertUTF16toUTF8 cbuf(buf);

Similar for cbuf in GetProxyForURI

+    if ((cbuf.Find("<local>") != -1) &&
+        (host.EqualsLiteral("localhost") ||
+         host.EqualsLiteral("127.0.0.1")))
+            return PR_TRUE;

What about "xxx<local>", should that trigger this path?

+    nsTArray<nsCAutoString> tokens;

You can't use an nsTArray of nsCAutoStrings. Use nsTArray<nsCString> instead.

MatchOverride desperately needs some comments explaining the grammar you're trying to parse.
Comment 3 Ted Mielczarek [:ted.mielczarek] 2009-03-30 04:06:37 PDT
(In reply to comment #1)
> Ted, will this Makefile force the creation of a shared library in libxul
> builds?

As written, it appears to be correct. It should be linked into libxul for libxul builds, and build a shared lib in non-libxul builds. (This is purely by inspection, of course. A run through the tryserver probably wouldn't hurt!)
Comment 4 Mitchell Field [:Mitch] 2009-03-31 20:34:54 PDT
Created attachment 370349 [details] [diff] [review]
Patch v1.1

> Is there really no way for ProxyServer to specify protocol-specific proxies and
> a catch-all proxy?

Windows' Proxy Settings dialog doesn't allow it.
Comment 5 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-03-31 20:56:52 PDT
I'm sorry Mitch, I messed up ... using the Windows registry directly is definitely more code than using the service you were using before. Please go back to the registry code you had before. Sorry!

I think you should break out the *-matching code into its own function, PatternMatchesHost or something like that. I think you can simplify it quite a bit. Instead of finding tokens and putting them in an array, just scan through the pattern and the hostname in parallel. I'll try writing some psuedocode for you.

+    // Windows formats its proxy override list in the form:
+    // server;server;server where server is a server name or IP address,
+    // or "<local>". <local> must be translated to "localhost;127.0.0.1".

How about
   // Windows formats its proxy override list in the form:
   // server;server;server where 'server' is a server name pattern or IP address,
   // or "<local>". "<local>" must be translated to "localhost;127.0.0.1".
   // In a server name pattern, a '*' character matches any substring and
   // all other characters must match themselves; the whole pattern must match
   // the whole hostname.
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-03-31 21:24:13 PDT
Created attachment 370352 [details]
Simpler *-matching code

You can adapt the "matches" function I wrote here.
Comment 7 Mitchell Field [:Mitch] 2009-04-05 16:46:35 PDT
Created attachment 371176 [details] [diff] [review]
Patch v1.2
Comment 8 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-04-05 17:15:33 PDT
+        if (override.FindChar('*') != -1)
+        {
+            if (PatternMatch(host, override))
+                return PR_TRUE;
+        }
+        else if (override.Equals(host))
+        {
+            return PR_TRUE;
+        }

We don't need this FindChar and Equals path here, PatternMatch will do the right thing and it's almost as fast (and we don't care about performance here, almost certainly).

+        else if (override.EqualsLiteral("<local>"))
+        {
+            // This override matches local addresses.
+            if (host.EqualsLiteral("localhost") ||
+                host.EqualsLiteral("127.0.0.1"))
+                return PR_TRUE;
+        }

This should come first, since I think we don't want the server "<local>" to actually match a host named "<local>".

Our style is
if (...) {
  ...
} else if (...) {
  ...
} else {
  ...
}
This would make the code a bit shorter in a few places.

+    PRInt32 tokenEnd = 0;

This can be declared down inside the 'while'.

+            token = Substring(aOverride, tokenStart, tokenEnd - tokenStart);

You can declare 'token' here, and make it a "const nsDependentString&".

+    nsCAutoString hostPort;
+    PRInt32 start;
+    PRInt32 end;

These can be declared down where you initialize them. Generally it's good minimize the scope of declared variables.

Getting close! :-)
Comment 9 Mitchell Field [:Mitch] 2009-04-05 23:50:01 PDT
Created attachment 371200 [details] [diff] [review]
Patch v1.3
Comment 10 Eric H. Jung 2009-04-07 11:50:07 PDT
Nice work so. Some comments:

(In reply to comment #4)
> > Is there really no way for ProxyServer to specify protocol-specific proxies and
> > a catch-all proxy?
> 
> Windows' Proxy Settings dialog doesn't allow it.

The GUI may not allow for it, but protocol-specific proxies are nevertheless supported. Sometimes registry changes are pushed out to an enterprise using the Internet Explorer Administration Kit (http://technet.microsoft.com/en-us/ie/bb219517.aspx) and other group policy editors, so this dialog can be completely bypassed. Docs on the format of protocol-specific proxies: http://msdn.microsoft.com/en-us/library/aa383996(VS.85).aspx. That link also contains docs on the format of <local>. Might be good comment fodder.

Small excerpt:
<EXCERPT>
By default, the function assumes that the proxy specified by lpszProxyName is a CERN proxy. An application can specify more than one proxy, including different proxies for the different protocols. For example, if you specify "ftp=ftp://ftp-gw HTTP=http://jericho:99 proxy", FTP requests are made through the ftp-gw proxy, which listens at port 21, and HTTP requests are made through a CERN (HTTP) proxy called jericho, which listens at port 99. Otherwise, HTTP requests would be made through the CERN proxy called proxy, which listens at port 80. Note that if the application is only using FTP, for example, it would not need to specify "ftp=ftp://ftp-gw:21". It could specify just "ftp-gw".
</EXCERPT>

Although the docs only mention ftp, http, and https, gopher appears to be used in the wild (See "accepted solution" at http://www.experts-exchange.com/Programming/Languages/CPP/Q_11115625.html for this example: "ftp=192.168.12.1:8080;gopher=192.168.12.22:8080;http=192.168.12.1:8080;https=192.168.12.1:8080").

Moreover, since SOCKS proxies are also supported per the docs, one wonders how a SOCKS server is differentiated from an HTTP server in the ProxyServer registry value. I will do some testing and searching to find out. If SOCKS is supported, SetProxyResult() should sometimes be called with "SOCKS", but as it stands now it is only called with "PROXY" (for HTTP/CERN proxies) and "DIRECT" (for no proxy).

The Unix implementation of this interface checks for negative ports and also truncates/trims whitespace from hostnames. Should this implementation do the same? It doesn't appear to AFAICT.

Finally, I'm pretty sure the parsing of the ProxyServer registry value should support three kinds of values. I've found examples of all of these in the wild:

1. ftp=192.168.12.1:8080;gopher=192.168.12.22:8080;http=192.168.12.3:8080;https=192.168.12.4:8080 (semicolon delimited)
2. ftp=192.168.12.2:8080 http=192.168.12.1:8080 https=192.168.12.2:8080 (space delimited)
3. ftp=192.168.12.19 (no delimiters, but also no port--port 21 is implied)
4. 192.168.12.1:8080 (example at http://www.astahost.com/info.php/Proxy-Network-Drive-Script_t17458.html, presumably this is the same as all protocols going through this HTTP proxy)

Sorry for all the extra work, Mitch!
Comment 11 Mitchell Field [:Mitch] 2009-04-22 21:04:25 PDT
Created attachment 374208 [details] [diff] [review]
Patch v1.4

This patch handles SOCKS proxies properly, allows proxy lists and proxy override lists to be space- or semicolon-delimited, and falls back to the default ("catch-all") proxy when a protocol-specific proxy is not specified.
Comment 12 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-04-23 05:07:11 PDT
I think we should probably replace FormatRegString and FindChar(';') with a real tokenizer loop plus skipping zero-length proxy entries. Cleaner than mutating the string turning ' ' into ';'. FindCharInSet would be helpful:
http://mxr.mozilla.org/mozilla-central/search?string=findcharinset

Otherwise looks good. I think that covers almost everything in comment #10.
Comment 13 Mitchell Field [:Mitch] 2009-04-24 06:01:37 PDT
Created attachment 374449 [details] [diff] [review]
Patch v1.5
Comment 14 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-04-24 15:01:29 PDT
+        if (start == end)
+            break;

You don't need this (in both places).

Other than that, we're good to go! It would be nice to have automated tests here, but I don't know how to write them.
Comment 15 Mitchell Field [:Mitch] 2009-04-25 00:18:04 PDT
Created attachment 374576 [details] [diff] [review]
Patch v1.6
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-04-25 05:10:20 PDT
http://hg.mozilla.org/mozilla-central/rev/435f08a09805

Thanks Mitch!
Comment 17 Jo Hermans 2009-10-24 08:00:21 PDT
*** Bug 524272 has been marked as a duplicate of this bug. ***
Comment 18 Jo Hermans 2010-04-29 04:07:03 PDT
*** Bug 386911 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.