[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-2d-dev
Subject: Re: [OpenJDK 2D-Dev] <AWT Dev> [8] Review request for 8005607: Recursion in J2DXErrHandler() Causes
From: Anton Litvinov <anton.litvinov () oracle ! com>
Date: 2013-01-31 15:42:23
Message-ID: 510A90DF.2060501 () oracle ! com
[Download RAW message or body]
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@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
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic