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

[Denis S. Fokin]

Ok. I will send a new version shortly.

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.
>>>>
>>