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

[Anthony Petrov]

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
>