Hello Anthony,
Could you please review the third version of the fix containing
modifications discussed with you in the previous letter.
Webrev: http://cr.openjdk.java.net/~alitvinov/8005607/webrev.02
This version of the fix differs from the previous in the following places:
1. A comment about the place of invocation of the method
"XErrorHandlerUtil.init" was added to a documentation block of the
method.
2. A code related to XShmAttach function common to the files
"src/solaris/native/sun/awt/awt_GraphicsEnv.c" and
"src/solaris/native/sun/java2d/x11/X11SurfaceData.c" was extracted
into a separate function "TryXShmAttach" declared in
"src/solaris/native/sun/awt/awt_GraphicsEnv.h" file.
3. All JNI code related to X error handling was implemented as
corresponding macros defined in
"src/solaris/native/sun/awt/awt_util.h" file.
Thank you,
Anton
On 1/31/2013 7:42 PM, Anton Litvinov wrote:
Hello Anthony,
Thank you for the review and these remarks. Surely, the comment will
be added. I think that all JNI code related to XShmAttach can be
definitely transferred into a separate dedicated function, which will
be declared in "src/solaris/native/sun/awt/awt_GraphicsEnv.h" file. I
will try to wrap all JNU calls connected with XErrorHandler into the
particular "WITH_XERROR_HANDLER", "RESTORE_XERROR_HANDLER" functions
or macros.
Thank you,
Anton
On 1/31/2013 4:57 PM, Anthony Petrov wrote:
Hi Anton,
A couple comments:
1. src/solaris/classes/sun/awt/X11/XErrorHandlerUtil.java
80 private static void init(long display) {
This method is private and isn't called from anywhere in this class
itself. This looks confusing. Please add a comment stating that this
method is invoked from native code, and from where exactly.
2. Interesting that we use this machinery to call the XShmAttach()
from native code twice, and the code looks quite similar in each
case. Would it be possible to extract the common code in a separate
function (a-la BOOL TryXShmAttach(...)) to avoid code replication?
There are other usages as well, so we could also introduce a macro
(such as the old EXEC_WITH_XERROR_HANDLER but now with other
arguments) that would minimize all the JNU_ calls required to use
this machinery.
Otherwise the fix looks great.
--
best regards,
Anthony
On 1/30/2013 20:14, Anton Litvinov wrote:
Hello Anthony,
Could you, please, review a second version of the fix, which is
based on an idea of reusing the existing AWT native global error
handler from Java 2D native code.
Webrev: http://cr.openjdk.java.net/~alitvinov/8005607/webrev.01
The fix consists of the following parts:
1. Migration of all X error handling code from XToolkit to a new
XErrorHandlerUtil class for resolution of interdependency between
a static initialization block of XToolkit and a block
initializing
java.awt.GraphicsEnvironment singleton. Such dependency is
created
by new calls to XToolkit static methods from
"src/solaris/native/sun/awt/awt_GraphicsEnv.c",
"src/solaris/native/sun/java2d/x11/X11SurfaceData.c" files.
2. Substitution of XToolkit.WITH_XERROR_HANDLER,
XToolkit.RESTORE_XERROR_HANDLER ... for corresponding methods,
fields of XErrorHandlerUtil class in all places of JDK source
code, where they were used.
3. Substitution of all found native X error handlers which are
set in
native code (awt_GraphicsEnv.c, X11SurfaceData.c,
GLXSurfaceData.c) for new synthetic Java error handlers.
4. Removal of X error handling code used by the native error
handlers
from "solaris/native/sun/awt/awt_util.c"
"solaris/native/sun/awt/awt_util.h" files.
Thank you,
Anton
On 1/11/2013 3:45 PM, Anthony Petrov wrote:
I'm not Jim, but as I indicated earlier my opinion is that the
easiest way to fix this is to install the existing J2DXErrHandler()
only once. That is, it is the second option listed by you. Of
course, the J2DXErrHandler needs to be updated as well to detect
whether 2D code wants to use it at the moment or it must simply
delegate to the previous handler (i.e. where the code currently
installs/uninstalls the handler, it must instead set a global
boolean flag or something.)
While the first option (reusing the existing AWT machinery) is an
interesting idea in general, I think it is complex and would
require too much additional testing and bring an unjustified risk
to the solution for such a basic problem.
--
best regards,
Anthony
On 1/11/2013 14:44, Anton Litvinov wrote:
Hello Jim,
Thank you very much for the review and provision of a new idea of
a solution. Elimination of the logic, which sets/unsets
J2DXErrHandler() for each call "XShmAttach(awt_display,
&shminfo))" should effectively resolve the issue, but only in case
if all other native error handlers, which were set by the system
function "XSetErrorHandler()" in JDK or in any external library,
observe the rule of relaying of all events, which are not relative
to them, to the previously saved error handlers. Otherwise an
error generated during "XShmAttach" function call will not be
handled by J2DXErrHandler().
Could you answer the following question. By setting
J2DXErrHandler() only once and forever do you mean usage of AWT
global event handler "static int ToolkitErrorHandler(Display *
dpy, XErrorEvent * event)" from
"src/solaris/native/sun/xawt/XlibWrapper.c" with Java synthetic
handlers or creation of another global native error handler with
J2DXErrHandler as native synthetic handler?
Thank you,
Anton
On 1/10/2013 5:44 AM, Jim Graham wrote:
I think I'd rather see some way to prevent double-adding the
handler in the first place as well. Since it is only ever used
on errors I also think it is OK to set it once and leave it there
forever...
...jim
On 1/9/13 8:08 AM, Anthony Petrov wrote:
Hi Anton et al.,
If I read the description of the bug correctly, specifically
this part:
The problem occurs, if another thread (for example, GTK thread) is
doing the same sort of thing concurrently. This can lead to the
following situation.
JVM thread: Sets J2DXErrHandler(), saves ANY_PREVIOUS_HANDLER as
previous GTK thread: Sets some GTK_HANDLER, saves
J2DXErrHandler() as previous JVM thread: Restores
ANY_PREVIOUS_HANDLER GTK thread: Restores
J2DXErrHandler() JVM
thread: Sets J2DXErrHandler(), saves J2DXErrHandler() as previous
It is obvious that at this final step 2D is in an inconsistent
state. We
don't expect to replace our own error handler (and it shouldn't
have
been there in the first place).
I realize that the fix you propose works around this problem.
But this
doesn't look like an ideal solution to me.
BTW, IIRC, in JDK7 (and 6?) we decided to set the actual X11 error
handler only once and never replace it. All the rest of the
push_handler/pop_handler logic is now located in Java code (see
XToolkit.SAVED_ERROR_HANDLER() and the surrounding logic). I
believe
that we should somehow share this machinery with the 2D code to
avoid
this sort of problems. Though I'm not sure if this will
eliminate this
exact issue.
2D/AWT folks: any other thoughts?
--
best regards,
Anthony
On 12/29/2012 17:44, Anton Litvinov wrote:
Hello,
Please review the following fix for a bug.
Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8005607
https://jbs.oracle.com/bugs/browse/JDK-8005607
Webrev: http://cr.openjdk.java.net/~alitvinov/8005607/webrev.00
The bug consists in a crash which is caused by a stack overflow
for
the reason of an infinite recursion in AWT native function
J2DXErrHandler() under certain circumstances on 32-bit Linux
OS. The
fix is based on introduction of the logic, which detects indirect
recursive calls to J2DXErrHandler() by means of a simple
counter, to
J2DXErrHandler() native function. Such a solution requires minimum
code changes, does not alter the handler's code significantly and
eliminates this bug.
Adding 2d-dev@openjdk.java.net e-mail alias to the list of
recipients
of this letter, because the edited function's name is related
to Java
2D area of JDK, despite of the fact that the edited file is
located in
AWT directory.
Thank you,
Anton
