<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
Mon Feb 18 04:04:55 PST 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,
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
>>> 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/20130218/57bc9c16/attachment.html
More information about the awt-dev
mailing list