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

[Anthony Petrov]

Actually, I'd suggest to check for GTK version first, and even not try 
loading the symbol in case GTK is too old. This is what we do for some 
file chooser-related GTK methods.

--
best regards,
Anthony

On 4/6/2012 2:51 PM, Artem Ananiev wrote:
> Hi, Denis,
> 
> just a minor question.
> 
> In gtk2_show_uri_load(), why isn't it enough to check fp_gtk_show_uri 
> for NULL? For example, if this var is not NULL, why do we care about GTK 
> version?
> 
> Thanks,
> 
> Artem
> 
> On 4/2/2012 4:35 PM, Denis S. Fokin wrote:
>> Hi Anthony,
>>
>> I took you suggestions into account. I have not Please take another look.
>>
>> http://cr.openjdk.java.net/~denis/7123476/webrev.04/
>>
>> Thank you,
>> Denis.
>>
>> On 3/29/2012 4:48 PM, Anthony Petrov wrote:
>>> 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.
>>>>>>
>>>>
>>