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

Anthony Petrov anthony.petrov at oracle.com
Thu Mar 21 12:19:37 PDT 2013 

This looks perfect. Thank you, Denis.

--
best regards,
Anthony

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

More information about the awt-dev mailing list