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

Thu Mar 21 09:19:39 PDT 2013

Hi Anthony,

it is not quite static, but I have added the initialization. The 
declaration now is extern and the initialization is in the compilation 
unit scope.

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

Thank you,
    Denis.

On 3/21/2013 5:02 PM, Anthony Petrov wrote:
> Hi Denis,
>
> I suggest to add a static initializer for the gnome_url_show symbol in
> the gnome_interface.c so that even if gnome_load() fails, the variable
> would be equal to NULL rather than point to a random address in memory.
> Otherwise the fix looks fine to me.
>
> --
> best regards,
> Anthony
>
> On 03/20/13 22:43, 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.
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>
>>