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

Wed Mar 20 11:43:48 PDT 2013

Hi Artem,

 > 1. gnome_load() should better return gboolean, not int.
Done.

 > 2. There is not need to export gnome_vfs_init from gnome_interface.c
I have made it a function local variable in the gnome_load function.

 > 3. What are VERSIONED_JNI_LIB_NAME and JNI_LIB_NAME macros? They are
 > lost, when the code is moved from awt_Desktop.c to gnome_interface.c
The macros construct OS-specific names for a library. I have added a 
comment and restored the initial variant.

I also noticed that I have missed a definition of gtk2_show_uri_load, so 
I have added it to the  gtk2_interface.h. Otherwise, compiler produces a 
warning.

Please take a look at the new version

http://cr.openjdk.java.net/~denis/7123476/webrev.08/

Thank you,
     Denis.

On 3/20/2013 9:17 PM, Artem Ananiev wrote:
> Hi, Denis,
>
> a few minor comments:
>
> 1. gnome_load() should better return gboolean, not int.
>
> 2. There is not need to export gnome_vfs_init from gnome_interface.c
>
> 3. What are VERSIONED_JNI_LIB_NAME and JNI_LIB_NAME macros? They are
> lost, when the code is moved from awt_Desktop.c to gnome_interface.c
>
> Thanks,
>
> Artem
>
> On 3/20/2013 8:06 AM, Denis S. Fokin wrote:
>> Please take a look at another version.
>>
>> I have slightly improved it.
>>
>> http://cr.openjdk.java.net/~denis/7123476/webrev.07/
>>
>> Thank you,
>>     Denis.
>>
>> On 4/6/2012 2:50 PM, Anthony Petrov wrote:
>>> 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.
>>>>>>>>>
>>>>>>>
>>>>>
>>