Closed Bug 485764 Opened 15 years ago Closed 15 years ago

Implement Windows System Proxy Settings Service

Categories

(Core :: Networking, defect)

x86
Windows Vista
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: Mitch, Assigned: Mitch)

References

()

Details

(Keywords: platform-parity)

Attachments

(2 files, 6 obsolete files)

Attached patch Patch v1.0 (obsolete) — Splinter Review
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.
Attachment #369867 - Flags: review?(roc)
Flags: wanted1.9.2?
Why can't we use the Registry APIs directly here?

Ted, will this Makefile force the creation of a shared library in libxul builds?
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.
(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!)
Attached patch Patch v1.1 (obsolete) — Splinter Review
> 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.
Attachment #369867 - Attachment is obsolete: true
Attachment #370349 - Flags: review?(roc)
Attachment #369867 - Flags: review?(roc)
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.
You can adapt the "matches" function I wrote here.
Attached patch Patch v1.2 (obsolete) — Splinter Review
Attachment #370349 - Attachment is obsolete: true
Attachment #371176 - Flags: review?(roc)
Attachment #370349 - Flags: review?(roc)
+        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! :-)
Attached patch Patch v1.3 (obsolete) — Splinter Review
Attachment #371176 - Attachment is obsolete: true
Attachment #371200 - Flags: review?(roc)
Attachment #371176 - Flags: review?(roc)
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!
Attached patch Patch v1.4 (obsolete) — Splinter Review
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.
Attachment #371200 - Attachment is obsolete: true
Attachment #374208 - Flags: review?(roc)
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.
Attached patch Patch v1.5 (obsolete) — Splinter Review
Attachment #374208 - Attachment is obsolete: true
Attachment #374449 - Flags: review?(roc)
Attachment #374208 - Flags: review?(roc)
+        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.
Attached patch Patch v1.6Splinter Review
Attachment #374449 - Attachment is obsolete: true
Attachment #374576 - Flags: superreview?(roc)
Attachment #374576 - Flags: review?(roc)
Attachment #374449 - Flags: review?(roc)
Attachment #374576 - Flags: superreview?(roc)
Attachment #374576 - Flags: superreview+
Attachment #374576 - Flags: review?(roc)
Attachment #374576 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/435f08a09805

Thanks Mitch!
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: wanted1.9.2?
Resolution: --- → FIXED
Flags: in-testsuite?
Target Milestone: --- → mozilla1.9.2a1
Blocks: 139393
You need to log in before you can comment on or make changes to this bug.