Hi Vyom, thanks for the comments – now I understand the problem. I reworked all three platforms to check for exceptions and NULL if needed. Regarding the JNIReleaseString calls: I seem to be on the save sidether. They are listed as some of the few calls that can be called with a pending exception as described in http://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/design.html#exception_handling
Please see my new webrev: http://cr.openjdk.java.net/~clanger/webrevs/8170868.2/ Thanks and best regards, Arno From: Vyom Tewari [mailto:vyom.tew...@oracle.com] Sent: Donnerstag, 22. Dezember 2016 14:27 To: Zeller, Arno <arno.zel...@sap.com> Cc: net-dev@openjdk.java.net Subject: Re: RFR:8170868: DefaultProxySelector should use system defaults on Windows, MacOS and Gnome On 12/22/2016 4:08 PM, Zeller, Arno wrote: Hi Vyom, thanks you for having a look at my patch! Regarding your suggestion to call CHECK_NULL_RETURN in DefaultProxySelector.c: I guess you mean the Unix variant of DefaultProxySelector.c (src/java.base/unix/native/libnet/DefaultProxySelector.c). 419 if (proxies) { 420 if (!error) { 421 int i; 422 // create an empty LinkedList to add the Proxy objects. 423 proxyList = (*env)->NewObject(env, list_class, list_ctrID); 424 if (proxyList != NULL) { 425 for(i = 0; proxies[i]; i++) { ... 454 } 455 } 456 } 457 (*g_strfreev)(proxies); 458 } There I check in the next line if proxyList is NULL and skip the rest in this case, but I cannot return without freeing the memory I got from the gnome function call by calling (g_strfreev) later - otherwise there would be a memory leak. yes that true, but if NewObject failed at line 423 then there will be pending JNI exception in "getSystemProxy" method which calls "getProxyByGProxyResolver" method. I will suggest you to check for any JNI exception after calling the "getProxyByGProxyResolver". And in the Windows case it is the same pattern - at the point where I removed the CHECK_NULL_RETURN macros I hold Windows system memory (in proxy_info and ie_proxy_config) that I have to free with GlobalFree(...) and I also have to release the JNI memory I hold with ReleaseStringChars(...). I hope this explains the motivation of my change at this point. it make sense but I am not the JNI expert may be some one else can comments if it is safe to call the functions like "ReleaseStringChars" etc even if there is pending JNI exception before(if NewString fails at 266) function call. Thanks, Vyom Best regards, Arno From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On Behalf Of Vyom Tewari Sent: Mittwoch, 21. Dezember 2016 17:08 To: net-dev@openjdk.java.net<mailto:net-dev@openjdk.java.net> Subject: Re: RFR:8170868: DefaultProxySelector should use system defaults on Windows, MacOS and Gnome Hi Arno, I suggest you to call "CHECK_NULL_RETURN" after below line in DefaultProxySelector.c // create an empty LinkedList to add the Proxy objects. + proxyList = (*env)->NewObject(env, list_class, list_ctrID); in windows/native/libnet/DefaultProxySelector.c file you remove the couple of "CHECK_NULL_RETURN" jstring jhost; - if (pport == 0) - pport = defport; - jhost = (*env)->NewStringUTF(env, phost); - CHECK_NULL_RETURN(jhost, NULL); - isa = (*env)->CallStaticObjectMethod(env, isaddr_class, isaddr_createUnresolvedID, jhost, pport); - CHECK_NULL_RETURN(isa, NULL); - proxy = (*env)->NewObject(env, proxy_class, proxy_ctrID, type_proxy, isa); - return proxy; + if (portVal == 0) + portVal = defport; + jhost = (*env)->NewString(env, phost, (jsize)wcslen(phost)); + if (jhost != NULL) { + isa = (*env)->CallStaticObjectMethod(env, isaddr_class, isaddr_createUnresolvedID, jhost, portVal); + if (isa != NULL) { + jobject proxy = (*env)->NewObject(env, proxy_class, proxy_ctrID, type_proxy, isa); + if (proxy != NULL) { + (*env)->CallBooleanMethod(env, proxy_list, list_addID, proxy); + } + } + } } Is there is specific reason behind removing these checks ? Thanks, Vyom On 12/21/2016 6:23 PM, Zeller, Arno wrote: Hi Volker, thanks for your valuable comments! I have a new patch ready that should address your issues and contains also a forgotten change to the map file... New webrev: http://cr.openjdk.java.net/~clanger/webrevs/8170868.1/<http://cr.openjdk.java.net/%7Eclanger/webrevs/8170868.1/> - make/lib/NetworkingLibraries.gmk ... Have you tried to use LIBNET_EXCLUDE_FILES := $(JDK_TOPDIR)/src/java.base/unix/native/libnet/DefaultProxySelector.c I think this should work and it would mke it possible to rename: src/java.base/macosx/native/libnet/DefaultProxySelector_mac.c to: src/java.base/macosx/native/libnet/DefaultProxySelector.c which is much nicer IMHO :) Great idea - it works and is of course the much nicer solution! - DefaultProxySelector.java 322 return proxyList == null ? Arrays.asList(Proxy.NO_PROXY) : proxyList; Not sure if it would make sense to preallocate a static List with a single Proxy.NO_PROXY element and always return that if proxyList equals null? I return a new List object each time, because the select(URI uri) method does not state anything about not modifying the returned list. In case I return a static list containing only the NO_PROXY element a caller could remove the object from the list and other caller would use the same modified list. To avoid this I always create a new List object. - java.base/unix/native/libnet/DefaultProxySelector.c You've removed "#include <strings.h>". Have you built on all Unix platforms (AIX, Solaris) to make sure you don't break anything? It compiled on Linux, AIX, Solaris and Mac without problems for me. - java.base/windows/native/libnet/DefaultProxySelector.c Not sure if I understand this right, but in the gconf case above you insert all proxies returned by "g_proxy_resolver_lookup" into the prox-list returned by DefaultProxySelector_getSystemProxies. In the Windows case you write: 247 * From MSDN: The proxy server list contains one or more of the following strings separated by semicolons or whitespace. 248 * ([<scheme>=][<scheme>"://"]<server>[":"<port>]) 249 * We will only take the first entry here because the java.net.Proxy class has only one entry. Why can't you build up a proxy list here in the same way you do it for the gconf case on Unix? Sorry - I just forgot to implement it. Good that you found it. The new webrev contains the missing functionality. - src/java.base/macosx/native/libnet/DefaultProxySelector_mac.c 76 #define kResolveProxyRunLoopMode CFSTR("com.sap.jvm.DefaultProxySelector") I'm not familiar with the Mac programming model, but I don't think "com.sap.jvm.DefaultProxySelector" is a good name for kResolveProxyRunLoopMode. It should be something like "java.net.DefaultProxySelector" but I'm open for better proposals :) You are right - I changed it to "sun.net.spi.DefaultProxySelector". PS: for your next RFR, can you please also add the estimated sisze and the bug id to the subject line (e.g. "RFR(M): 8170868:DefaultProxySelector should..."). This makes it much easier to find a review thread :) I'll do my best next time... Best regards, Arno On Wed, Dec 14, 2016 at 5:06 PM, Zeller, Arno <arno.zel...@sap.com><mailto:arno.zel...@sap.com> wrote: Hi, can you please review my proposal for bug 8170868 - DefaultProxySelector should use system defaults on Windows, MacOS and Gnome. Bug: https://bugs.openjdk.java.net/browse/JDK-8170868 Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8170868.0/<http://cr.openjdk.java.net/%7Eclanger/webrevs/8170868.0/> Thanks a lot, Arno Zeller
