Hi Anton, The updated fix looks fine to me. Thank you.
Just a minor suggestion: could you please add a comment just before the line 441 at GLXSurfaceData.c to state that this particular XSync() call is performed w/o an AWTLock? No need to resend a new webrev for this change.
-- best regards, Anthony On 07/10/2013 07:16 PM, Anton Litvinov wrote:
Hello Anthony, Thank you for review of this fix and for these remarks. I think that it is better to make macros return values, then to rename them to STORE_*, because it will reduce the number of parameters of the macros and make them clear. Yes, sure, I can file a separate bug for changing of JDK 8 code related to the fix to make it equal to the fix for JDK 7, but only after the final version of the fix for JDK 7 is deduced and integrated into jdk7u-dev repository. Could you please review the second version of the fix with corrections which should follow your remarks. Webrev: http://cr.openjdk.java.net/~alitvinov/8005607/jdk7/webrev.01 The following changes were added to the fix: 1. The macros GET_HANDLER_ERROR_OCCURRED_FLAG, GET_XERROR_CODE from the file "awt_util.h" were changed to expressions to return values. The places, from which the macros are called, were edited. "awt_util.h": 119 errorOccurredFlag = GET_HANDLER_ERROR_OCCURRED_FLAG(env, handlerRef); "GLXSurfaceData.c": 443 errorOccurredFlag = GET_HANDLER_ERROR_OCCURRED_FLAG(env, errorHandlerRef); "awt_xembed_server.c": 659 xerror_code = GET_XERROR_CODE(env, savedError); "awt_wm.c": 1053 xerror_code = GET_XERROR_CODE(env, savedError); 2743 xerror_code = GET_XERROR_CODE(env, savedError); 2. The tags <code>...</code> were changed for {@code ...} in all java comments which were added by the fix. The edited files are "XErrorHandler.java" and "XErrorHandlerUtil.java". 3. The comment related to XSync() method call in the method "sun.awt.X11.XErrorHandlerUtil.RESTORE_XERROR_HANDLER(boolean doXSync)" was moved in the body of "if (doXSync)" statement. Also jdk7u-dev repository with this fix was successfully compiled for all architectures on Linux OS and Oracle Solaris OS. And the fix was verified for the bug JDK-8005607 and JDK-8015730 in my local environment. Thank you, Anton On 7/9/2013 5:50 PM, Anthony Petrov wrote:Hi Anton, The word "get" usually implies that a method returns some value. The GET_HANDLER_ERROR_OCCURRED_FLAG/GET_XERROR_CODE macros that you introduce actually store a value in a variable. So perhaps they might be renamed to STORE_* then? Alternatively, you could use the ?: operator and actually make the macros "return" a value. E.g.: ((h) ? J_C(...) : JNI_FALSE) or ((sE = J_GSFBN()) ? J_CMBN() : Success ) You could even try to avoid passing the savedError arg (if it's unneeded in caller code) with the following macro: ({jobject sE = ...; (sE ? J_C() : Success);}) (not sure if this compiles on Solaris with old C compilers though). Also, the G_X_C initializes the value with Success, but G_H_E_O_F doesn't do that. Which means that if handlerRef is NULL for some reason, the value stored in the variable errorOccirredFlag might be undefined. So, if we choose the STORE_ alternative, should we use a similar pattern for both macros in this regard, or there's a reason to not do that? I suggest to replace legacy <code>...</code> in javadocs in XErrorHandlerUtil.java with the modern variant {@code ... }. (And I'd appreciate if you could file a separate bug and port this change to jdk8 as well).112 // Wait until all requests are processed by the X server 113 // and only then uninstall the error handler. 114 if (doXSync) { 115 XSync();The comment applies to the XSync() call itself, so it's worth moving it inside the if(){} block. -- best regards, Anthony On 07/09/2013 01:30 PM, Anton Litvinov wrote:Hello Artem and Anthony, Could you please review this backport of the fix, which was approved by you for JDK 8, from JDK 8 to JDK 7. Bug: http://bugs.sun.com/view_bug.do?bug_id=8005607 Webrev: http://cr.openjdk.java.net/~alitvinov/8005607/jdk7/webrev.00 JDK 8 webrev: http://cr.openjdk.java.net/~alitvinov/8005607/webrev.05 This fix is identical to the original fix for JDK 8 with the exception of the following places: 1) The new macro "GET_XERROR_CODE" was added in the file "src/solaris/native/sun/awt/awt_util.h". This macro allows native code to get "error_code" value of a saved XErrorEvent object from the global toolkit error handler in Java code. This variable is used in functions defined in the files "awt_wm.c", "awt_xembed_server.c". 138 #define GET_XERROR_CODE(env, savedError, errorCode) do { 2) The following 3 native XError handlers were substituted for the corresponding Java synthetic analogs in the files "src/solaris/native/sun/awt/awt_wm.c", "src/solaris/native/sun/awt/awt_xembed_server.c". a. xerror_ignore_bad_window(Display *dpy, XErrorEvent *err) --> Existing "sun.awt.X11.XErrorHandler.IgnoreBadWindowHandler" b. xerror_verify_change_property(Display *dpy, XErrorEvent *err) --> Existing "sun.awt.X11.XErrorHandler.VerifyChangePropertyHandler" c. xerror_detect_wm(Display *dpy, XErrorEvent *err) --> Created "sun.awt.X11.XErrorHandler.XChangeWindowAttributesHandler" 3) The fix for JDK-8015730, which is the regression of the fix for JDK-8005607 discovered in JDK 8, was ported to JDK 7 as part of this backport fix without any changes. The fix was tested by means of a manual testcase attached to the bug's record in JBS both on Linux OS and on Oracle Solaris OS. Thank you, Anton
