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

[Anthony Petrov]

Looks great. Thanks.

--
best regards,
Anthony

On 07/12/2013 04:02 PM, Anton Litvinov wrote:
> 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
>>>>>
>>>
>