[OpenJDK 2D-Dev] <AWT Dev> [8] Review request for 8005607: Recursion in J2DXErrHandler() Causes a Stack Overflow on Linux

[Anthony Petrov]

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