<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 08:08:16 PST 2013
Hello Artem,
Could you please review a new version of the fix. The method
"XErrorHandlerUtil.getDisplay()" was removed and
"XErrorHandlerUtil.XSync()" method refers to the field "display"
directly now.
Webrev: http://cr.openjdk.java.net/~alitvinov/8005607/webrev.03
Thank you,
Anton
On 2/18/2013 4:23 PM, Artem Ananiev wrote:
>
> 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.
>
> Thanks,
>
> Artem
>
>> 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
>>>>
>>
More information about the awt-dev
mailing list