<AWT Dev> status of 6532373 (xcb_xlib.c:50: xcb_xlib_unlock: Assertion 'c->xlib.lock' failed.)

[Oleg Sukhodolsky]

Hi Josh,

Josh Triplett wrote:
> Oleg Sukhodolsky wrote:
>> JFYI, I have developed preliminary version of the fix (some lines may be 
>> changed in final version because of possible conflicts with removing 
>> Motif-based AWT implementation)
>>
>> But anyone who suffer from the problem can already use these changes.
> 
> Thank you for looking at this.  Comments on the patch below; note that
> other than the comment about using the major numer in the library
> name, none of them absolutely need fixing before applying the bugfix.

Thank you for your comments:

>> +++ awt_GraphicsEnv.c        2007-09-25 13:06:28.000000000 +0400
>> @@ -30,10 +30,11 @@
>>   #include <sun_awt_X11GraphicsEnvironment.h>
>>   #include <sun_awt_X11GraphicsDevice.h>
>>   #include <sun_awt_X11GraphicsConfig.h>
>>   #ifndef HEADLESS
>>   #include <X11/extensions/Xdbe.h>
>> +#include <X11/XKBlib.h>
> 
> XKBlib?  This change doesn't seem related to the rest of the patch.

yep it was fix for build warning on solaris.

> And also, wouldn't that make the later inclusion of the same header
> under __linux__ redundant?  (Unless this block resides under an #ifdef
> outside the context.)

I've missed this, thank you.

>> +static void xinerama_init_linux()
>> +{
>> +    void* libHandle = 0;
>> +    char* XineramaLibName= "libXinerama.so";
> 
> Given that you expect a particular interface from this library, you
> should include the major number in this name: "libXinerama.so.1".
> Otherwise, if the system you run on has an incompatible version of
> this library (such as a new libXinerama.so.2, or an old
> libXinerama.so.0), Java would break mysteriously at runtime by trying
> to use the old interface in the new library.  If you include the major
> number, you will find the library with the version you expect, or you
> will not find the library at all.  Furthermore, distribution packages
> often split libraries into a runtime package and a development package
> (for instance, in Debian, libxinerama1 and libxinerama-dev), and only
> the development package includes the symlink for the unversioned
> library name; thus, if you use "libXinerama.so", distributions either
> need to patch Java to use the versioned library name or make Java
> depend on the development packages for libXinerama.

Ok, I will change this too.

>> +    /* load library */
>> +    libHandle = dlopen(XineramaLibName, RTLD_LAZY | RTLD_GLOBAL);
>> +    if (libHandle != 0) {
>> +        XineramaQueryScreens = (XineramaQueryScreensFunc*)
>> +            dlsym(libHandle, XineramaQueryScreensName);
>> +
>> +        if (XineramaQueryScreens != NULL) {
>> +            DTRACE_PRINTLN("calling XineramaGetInfo func on Linux");
> 
> Wrong function name in the string here.  Should say XineramaQueryScreens.

Copy-paste :(

>> -#else /* Solaris */
>> -
>> -    char* XinExtName = "XINERAMA";
>> -    int32_t major_opcode, first_event, first_error;
>> -    Bool gotXinExt = False;
>> +}
>> +#endif
>> +#ifndef __linux__ /* Solaris */
>> +static void xinerama_init_solaris()
>> +{
>>       void* libHandle = 0;
>> -    unsigned char fbhints[MAXFRAMEBUFFERS];
>> -    int locNumScr = 0;
>> -
>>       char* XineramaLibName= "libXext.so";
> 
> Same comment as before about using the major number, but this bug
> already exists so fixing it doesn't form part of this bugfix patch.

I think that for solaris it is not a problem.  Because we always have 
unversioned .so and the functions we use are supposed to be there for 
backward compatibility.

>> +/*
>> + * Checks is Xinerama is running and perform Xinerama-related
> 
> Typo: s/Checks is/Checks if/.

Ok.

>> + * platform dependent initialization.
>> + */
>> +static void xineramaInit(void) {
>> +    char* XinExtName = "XINERAMA";
>> +    int32_t major_opcode, first_event, first_error;
>> +    Bool gotXinExt = False;
>> +
>> +    gotXinExt = XQueryExtension(awt_display, XinExtName, &major_opcode,
>> +                                &first_event, &first_error);
>> +
>> +    if (!gotXinExt) {
>> +        DTRACE_PRINTLN("Xinerama is not available");
>> +        return;
>> +    }
>> +
>> +    DTRACE_PRINTLN("Xinerama extension is available");
> 
> The two strings here seem inconsistent: one says "Xinerama" and the
> other says "Xinerama extension".

fixed.

>>   Java_sun_awt_X11GraphicsDevice_getDisplay(JNIEnv *env, jobject this)
>>   {
>>   #ifdef HEADLESS
>>       return NULL;
>>   #else
>> -    return (jlong) awt_display;
>> +    return ptr_to_jlong(awt_display);
> 
> This change doesn't seem related to the rest of the patch.

one more fix for build warning.

Oleg.

> 
> - Josh Triplett
>