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

Artem Ananiev artem.ananiev at oracle.com
Mon Feb 18 12:23:09 UTC 2013

On 2/18/2013 4:04 PM, Anton Litvinov wrote:
> 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.

OK, I missed that usages when looking at the webrev. Let it stay 
unchanged now.

>  2. Yes, I completely agree that "XErrorHandlerUtil.getDisplay()" is
>     reduntant. This method will be eliminated.



> Thank you,
> Anton
> 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

More information about the 2d-dev mailing list