<AWT Dev> [OpenJDK 2D-Dev] [8] Review request for 8005607: Recursion in J2DXErrHandler() Causes a Stack Overflow on Linux
Anthony Petrov
anthony.petrov at oracle.com
Fri Feb 15 04:18:40 PST 2013
Hi Anton,
The fix looks great to me. Thanks for all your hard work!
--
best regards,
Anthony
On 2/13/2013 20:57, 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
>> 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
>
More information about the awt-dev
mailing list