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