<AWT Dev> [7u] Review request for 8005607: Recursion in J2DXErrHandler() Causes a Stack Overflow on Linux

[Anton Litvinov]

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
>>