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
