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