<AWT Dev> [8] Review request for 7123476: DesktopOpenTests:When enter the file path and click the open button, it crash

[Anthony Petrov]

Hi Denis,

It's not that I'm insisting on anything. I'm just looking into the 
gnome_interface.c code and seeing that we both check the return value 
for NULL as well as check the dlerror() status. I would like to see some 
consistency between loading the GTK and Gnome libs in this regard.

Also, please take a look at man dlsym, it says:

>    dlsym()
>        The function dlsym() takes a "handle" of a dynamic library returned by dlopen() and the null-terminated symbol name, returning the address where that symbol is loaded into memory.   If  the  symbol  is  not
>        found,  in  the  specified  library  or any of the libraries that were automatically loaded by dlopen() when that library was loaded, dlsym() returns NULL.  (The search performed by dlsym() is breadth first
>        through the dependency tree of these libraries.)  Since the value of the symbol could actually be NULL (so that a NULL return from dlsym() need not indicate an error), the correct way to test for  an  error
>        is to call dlerror() to clear any old error conditions, then call dlsym(), and then call dlerror() again, saving its return value into a variable, and check whether this saved value is not NULL.

I realize that while a NULL pointer may be a valid return value for 
dlsym(), it's useless for our purposes and is very unlikely to happen in 
this case anyway. However, if I read the specification of dlsym() 
correctly, since we're going to call a function referenced by the return 
value of dlsym(), we must check it for NULL as well as check the 
dlerror() status. If any of this indicates an error, we should assume 
that the init() function has failed, and hence should return FALSE.

--
best regards,
Anthony

On 03/28/12 21:14, Denis S. Fokin wrote:
> Hi Anthony,
>
> thank you for the review notes.
>
> Actually, I expect that if fp_gtk_show_uri is null we have some kind of
> dlerror. Anyway I check fp_gtk_show_uri in
> Java_sun_awt_X11_XDesktopPeer_gnome_1url_1show. So I would not add
> additional check here. But if you insist I will add the NULL check.
>
> Thank you,
> Denis.
>
>
>
> On 3/28/2012 7:01 PM, Anthony Petrov wrote:
>> Hi Denis,
>>
>> src/solaris/native/sun/xawt/gnome_interface.c
>>> 2 * Copyright (c) 2005, 2011, Oracle and/or its affiliates. All rights
>>> reserved.
>>
>> I think this file has just been created in 2012 :)
>>
>>> 30 fprintf(stderr, "gnome_load\n");
>>
>> Please remove all the debugging output, or put it under the #ifdef
>> INTERNAL_BUILD.
>>
>>> 70 fprintf(stderr, "can not find symble gnome_url_show\n");
>>
>> s/symble/symbol/
>>
>> src/solaris/native/sun/awt/gtk2_interface.c
>>> 447 return TRUE;
>>
>> I think it also makes sense to check fp_gtk_show_uri for NULL before
>> returning TRUE here.
>>
>> The rest of the fix looks fine to me. Thank you!
>>
>> --
>> best regards,
>> Anthony
>>
>> On 3/27/2012 5:03 PM, Denis S. Fokin wrote:
>>> Hi Anthony,
>>>
>>> here is a new version of the fix.
>>>
>>> http://cr.openjdk.java.net/~denis/7123476/webrev.03/
>>>
>>> I took into account you suggestions. Now the implementation loads gtk
>>> API if it exists on the library path. If it does not exists we try to
>>> load gnome API. If it is not successful we do not support the
>>> functionality.
>>>
>>> I introduced a couple of files to keep gnome interface separately like
>>> we do with gtk. I expect that we remove them as soon as all our
>>> supported OS configurations will be have installed the proper GTK
>>> library by default.
>>>
>>> As for the synchronization section, I do not see how to use the
>>> fp_gdk_threads_* functions with gnome API so I put the critical under
>>> gtk-specific if-clause section.
>>>
>>> Thank you,
>>> Denis.
>>>
>>> On 2/1/2012 9:03 PM, Anthony Petrov wrote:
>>>> Hi Denis,
>>>>
>>>> The gtk_show_uri() is available since GTK 2.14. Did you verify if all
>>>> platforms supposed to be supported by JDK 8 have this version of GTK
>>>> libraries installed by default? I'm mostly concerned about Solaris
>>>> systems, as well as corporate Linux desktops. If this is not the case,
>>>> then perhaps using this function should be conditional, and with the
>>>> old
>>>> GTK library we should fall back to using the old API. You may notice
>>>> that, for example, for the file chooser we have an explicit check for
>>>> GTK 2.8.0 and use the new
>>>> gtk_file_chooser_set_do_overwrite_confirmation() API only when it's
>>>> available.
>>>>
>>>> I like that we move all the GTK-related utility code to the
>>>> gtk2_interface files. A few comments:
>>>>
>>>> 1. Please use the TRUE and FALSE constants instead of 1 and 0 as a
>>>> return value for gtk2_show_uri_load().
>>>>
>>>> 2. Should the fprintf() call be #ifdef'ed for INTERNAL_BUILD's only?
>>>>
>>>> --
>>>> best regards,
>>>> Anthony
>>>>
>>>> On 2/1/2012 7:39 PM, Denis S. Fokin wrote:
>>>>> Hi AWT team,
>>>>>
>>>>> Please review a fix for the CR 7123476 at
>>>>>
>>>>> http://cr.openjdk.java.net/~denis/7123476/webrev.01
>>>>>
>>>>> CR URL:
>>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7123476
>>>>>
>>>>> The Gnome API is deprecated so we need to migrate on GTK function. See
>>>>> the next thread.
>>>>>
>>>>> http://mail.gnome.org/archives/gnome-devel-list/2009-January/msg00004.html
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Thank you,
>>>>> Denis.
>>>
>