[OpenJDK 2D-Dev] [8] Review request for 8005607: Recursion in J2DXErrHandler() Causes a Stack Overflow on Linux
Anton Litvinov
anton.litvinov at oracle.com
Tue Mar 26 12:40:52 UTC 2013
Hello,
Please review the following fix for a bug. The fix passed 3 cycles of
review by AWT development team. Artem Ananiev and Anthony Petrov
approved it. But because the fix modifies also Java 2D Graphics code,
review by 2D Graphics development team is necessary. New "webrev.04" was
generated to resolve problem in not smooth patching of the latest
version of the file "src/solaris/native/sun/java2d/opengl/GLXSurfaceData.c".
Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8005607
Webrev: http://cr.openjdk.java.net/~alitvinov/8005607/webrev.04
Thank you,
Anton
On 2/20/2013 5:43 PM, Artem Ananiev wrote:
>
> Looks fine.
>
> Thanks,
>
> Artem
>
> On 2/18/2013 8:08 PM, Anton Litvinov wrote:
>> 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 2d-dev
mailing list