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

Anton Litvinov anton.litvinov at oracle.com
Wed Feb 13 08:57:08 PST 2013

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
 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,

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 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 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/awt-dev/attachments/20130213/ecbe7d7c/attachment.html 

More information about the awt-dev mailing list