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

[Anton Litvinov]

Hello Anthony,

Thank you very much for review of the fix. Yes, sure, the comment 
stating that XSync() function is intentionally called without AWT lock 
is added to the file "GLXSurfaceData.c". A URL of the new version of the 
fix with this change is provided below.

441     // Call XSync without the acquired AWT lock to avoid a deadlock 
(see 8015730).
442     XSync(awt_display, False);

Webrev: http://cr.openjdk.java.net/~alitvinov/8005607/jdk7/webrev.02

The new webrev was generated, because I will have to ask my team member 
with "Committer" status to integrate the fix and he will need just a 
correct patch.

Thank you,
Anton

On 7/12/2013 2:14 PM, Anthony Petrov wrote:
> 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
>>>>
>>