RFR:8170868: DefaultProxySelector should use system defaults on Windows, MacOS and Gnome

[Chris Hegarty]

Arno,

Thanks for doing this, and your patience so far. One more final
round of comments from me. Mostly minor. I’ve put them in the
form of a webrev so you can easily view and import them, if you
agree.

  http://cr.openjdk.java.net/~chegar/8170868_additional/

1)  A few comment rewordings and formatting suggestions.

2) I see duplicate DIRECT, and a few specific proxies, in my
    testing. Maybe my environment or PAC file is returning
    duplicate entries. I suggest that, at the java-level, we filter
    out duplicates while preserving ordering. ( see above webrev )

3) We do not use asserts in core libraries. So I replaced the usage
    with an early return NULL ( no proxy ). At a later stage we could
    throw an exception or something, but probably not all that
    interesting.

I have run this change ( along with my additional suggestions )
through our internal build and test system. No surprises.

-Chris.

> On 31 Jan 2017, at 09:14, Zeller, Arno <arno.zeller at sap.com> wrote:
> 
> Hi Chris,
> 
> thanks a lot - I did not see this case in my tests. I added your change to my new webrev:
> http://cr.openjdk.java.net/~clanger/webrevs/8170868.5/
> 
> 
> Best regards,
> Arno
> 
>> -----Original Message-----
>> From: Chris Hegarty [mailto:chris.hegarty at oracle.com]
>> Sent: Montag, 30. Januar 2017 17:14
>> To: Zeller, Arno <arno.zeller at sap.com>
>> Cc: net-dev at openjdk.java.net
>> Subject: Re: RFR:8170868: DefaultProxySelector should use system defaults
>> on Windows, MacOS and Gnome
>> 
>> Arno,
>> 
>> I found an issue on Windows when proxies are specified per-protocol, i.e.
>> they are returned with their optional scheme set. I believe that the scheme
>> should be checked, at least without this change FTP proxies were being
>> returned for HTTP URL's, on my machine.
>> 
>> $ hg -R jdk diff
>> diff -r 07e07fecf383
>> src/java.base/windows/native/libnet/DefaultProxySelector.c
>> --- a/src/java.base/windows/native/libnet/DefaultProxySelector.c
>> Mon Jan 30 14:09:14 2017 +0000
>> +++ b/src/java.base/windows/native/libnet/DefaultProxySelector.c
>> Mon Jan 30 16:09:23 2017 +0000
>> @@ -99,6 +99,7 @@
>>  * Returns the size of the array as int.
>>  */
>> static int createProxyList(LPWSTR win_proxy, const WCHAR *pproto,
>> list_item **head) {
>> +    static const wchar_t separators[] = L"\t\r\n ;";
>>     list_item *current = NULL;
>>     int nr_elems = 0;
>>     wchar_t *context = NULL;
>> @@ -109,13 +110,26 @@
>>      * The proxy server list contains one or more of the following strings
>> separated by semicolons or whitespace.
>>      * ([<scheme>=][<scheme>"://"]<server>[":"<port>])
>>      */
>> -    current_proxy = wcstok_s(win_proxy, L"; ", &context);
>> -    while (current_proxy != NULL) {
>> +    current_proxy = wcstok_s(win_proxy, separators, &context);
>> +    while (current_proxy != NULL) {
>>         LPWSTR pport;
>>         LPWSTR phost;
>>         int portVal = 0;
>>         wchar_t *next_proxy = NULL;
>>         list_item *proxy = NULL;
>> +        wchar_t* pos = NULL;
>> +
>> +        /* Filter based on the scheme, if there is one */
>> +        pos = wcschr(current_proxy, L'=');
>> +        if (pos) {
>> +          *pos = L'\0';
>> +          if (wcscmp(current_proxy, pproto) != 0) {
>> +              current_proxy = wcstok_s(NULL, separators, &context);
>> +              continue;
>> +          }
>> +          current_proxy = pos + 1;
>> +        }
>> 
>>         /* Let's check for a scheme and ignore it. */
>>         if ((phost = wcsstr(current_proxy, L"://")) != NULL) { @@ -152,7 +166,7
>> @@
>>             }
>>         }
>>         /* goto next proxy if available... */
>> -        current_proxy = wcstok_s(NULL, L"; ", &context);
>> +        current_proxy = wcstok_s(NULL, separators, &context);
>>     }
>>     return nr_elems;
>> }
>> @@ -268,7 +282,6 @@
>>         if (win_proxy != NULL) {
>>             wchar_t *context = NULL;
>>             int defport = 0;
>> -            WCHAR pproto[MAX_STR_LEN];
>>             int nr_elems = 0;
>> 
>>             /**
>> @@ -290,10 +303,7 @@
>>                 goto noproxy;
>>             }
>> 
>> -            /* Prepare protocol string to compare with. */
>> -            _snwprintf(pproto, sizeof(pproto), L"%s=", lpProto);
>> -
>> -            nr_elems = createProxyList(win_proxy, pproto, &head);
>> +            nr_elems = createProxyList(win_proxy, lpProto, &head);
>>             if (nr_elems != 0 && head != NULL) {
>>                 int index = 0;
>>                 proxy_array = (*env)->NewObjectArray(env, nr_elems, proxy_class,
>> NULL);
>> 
>> -Chris.
>> 
>> P.S. I will take a look at the latest webrev.
>>