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

[Denis S. Fokin]

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