[OpenJDK 2D-Dev] <AWT Dev> [8] Review request for 8005607: Recursion in J2DXErrHandler() Causes a Stack Overflow on Linux

Anton Litvinov anton.litvinov at oracle.com
Mon Feb 18 12:04:55 UTC 2013

Hello Artem,

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.

 1. 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.
 2. Yes, I completely agree that "XErrorHandlerUtil.getDisplay()" is
    reduntant. This method will be eliminated.

Thank you,

On 2/18/2013 3:16 PM, Artem Ananiev wrote:
> Hi, Anton,
> a few minor comments:
> 1. XErrorHandlerUtil: can saved_error be private instead of package 
> protected?
> 2. XErrorHandlerUtil.getDisplay() seems to be redundant.
> In general, the fix looks perfectly fine to me. Please, wait for 
> comments from Java2D team, though.
> Thanks,
> Artem
> On 2/13/2013 8:57 PM, Anton Litvinov wrote:
>> 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
>>> 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 
>>>>>>>>>> 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 at 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20130218/57bc9c16/attachment.html>

More information about the 2d-dev mailing list