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

Artem Ananiev artem.ananiev at oracle.com
Wed Mar 20 13:22:29 PDT 2013 

Hi, Denis,

I've no other comments. Looks fine.

Thanks,

Artem

On 3/20/2013 11:43 AM, Denis S. Fokin wrote:
> 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.
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>
>

More information about the awt-dev mailing list