<AWT Dev> [8] Review request 7059886: 6 JCK manual awt/Desktop tests fail with GTKLookAndFeel - GTK intialization issue

[Anthony Petrov]

+1

--
best regards,
Anthony

On 09/30/2013 04:08 PM, Artem Ananiev wrote:
>
> This version of the fix looks fine.
>
> Thanks,
>
> Artem
>
> On 9/27/2013 9:17 PM, Alexander Zvegintsev wrote:
>> Anthony,
>>
>> please see inline:
>>
>> On 09/27/2013 06:25 PM, Anthony Petrov wrote:
>>> Hi Alexander,
>>>
>>> How about getAndSetInitializationNeededFlag ?
>>>
>>>>   74      * If it returns {@code false} user must call
>>>> g_thread_init() on his own.
>>>
>>> "on their own"? :) Yet, it might be better phrased as:
>>>
>>> "A return value of {@code false} indicates that the calling code should
>>> call the g_thread_init() function in Glib before releasing the lock."
>>>
>>>
>> Thanks, this sounds much better.
>>> src/solaris/native/sun/awt/gtk2_interface.c
>>>>  837             //According the GTK documentation,
>>>> gdk_threads_init() should be
>>>>  838             //called before gtk_init() or gtk_init_check()
>>>>  839             fp_gdk_threads_init();
>>>
>>> Interesting. I see we only specify we want user code to call
>>> g_thread_init(), while here we also call gdk_threads_init() and use
>>> the same flag to decide whether to call it or not. Is this OK? What
>>> about other clients (FX)?
>>>
>> FX will call gdk_threads_init() within the lock, since it internally
>> sets global g_mutex, second call to gdk_threads_init() will replace this
>> mutex.
>> We can get into a trouble if we call gdk_threads_init() while other
>> thread holding lock on this mutex.
>>
>> Here  is updated webrev:
>> http://cr.openjdk.java.net/~serb/alexz/7059886/webrev.02/
>>
>> Thanks,
>>
>> Alexander.
>>
>>> --
>>> best regards,
>>> Anthony
>>>
>>> On 09/27/2013 06:02 PM, Alexander Zvegintsev wrote:
>>>> Hi Anthony, Artem,
>>>>
>>>> here is a new version of webrev with suggested improvements:
>>>> http://cr.openjdk.java.net/~serb/alexz/7059886/webrev.01/
>>>>
>>>> Name of a function in GThreadHelper is not ideal, but I can't think out
>>>> any better.
>>>>
>>>>> We don't expect C code to use try - finally, but when we use
>>>>> GThreadHelper from FX, we'll do.
>>>> In FX we will use it from native too.
>>>>
>>>>> BTW, is g_thread_get_initialized() really deprecated? If that is the
>>>>> case, it doesn't worth to use it, but rely on GThreadHelper only.
>>>> I think it worth to use it. It is deprerecated due to:
>>>>> |g_thread_init| has been deprecated since version 2.32 and should not
>>>>> be used in newly-written code. The GLib threading system is
>>>>> automatically initialized at the start of your program.
>>>> hence g_thread_get_initialized() returns always true on a modern GLib
>>>> versions.
>>>>
>>>>
>>>> Thanks,
>>>>
>>>> Alexander.
>>>>
>>>> On 09/24/2013 04:12 PM, Anthony Petrov wrote:
>>>>> Hi Alexander,
>>>>>
>>>>> A few comments on the GThreadHelper class:
>>>>>
>>>>> 1. I think the static methods in the GThreadHelper class should be
>>>>> public, because they are intended to be called by external code. Note
>>>>> that all public symbols should have a javadoc.
>>>>>
>>>>> 2. Why do we need both isInitializationNeeded() and initFinished()?
>>>>> Can the isInitializationNeeded() act (and be named) like the
>>>>> AtomicBoolean.compareAndSet/getAndSet() methods, in that it would
>>>>> automatically set the initialized flag to true?
>>>>> Or do you see a use case when a code may want to check that GTK isn't
>>>>> initialized yet and still not initialize it afterwards? What could
>>>>> this be needed for?
>>>>>
>>>>> 3. Since the flag has to be queried/modified under the LOCK only, it
>>>>> doesn't need to be volatile.
>>>>>
>>>>> 4. In Javadocs, the first sentence usually summarizes what a method
>>>>> does. Locking requirements should be stated either at the end of the
>>>>> first sentence, or in a subsequent sentence.
>>>>>
>>>>> --
>>>>> best regards,
>>>>> Anthony
>>>>>
>>>>> On 09/24/2013 02:40 PM, Alexander Zvegintsev wrote:
>>>>>> Hello,
>>>>>>
>>>>>> Please review the fix for the issue:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-7059886
>>>>>> The webrev is available here:
>>>>>> http://cr.openjdk.java.net/~serb/alexz/7059886/webrev.00/
>>>>>>
>>>>>> For old versions of GLib (< 2.24) calling g_thread_init () [1]
>>>>>> multiple
>>>>>> times will crash an application.
>>>>>> There are two ways to find out if g_thread_init() has been called:
>>>>>> g_thread_supported ()[2] and g_thread_get_initialized ()[3]
>>>>>>
>>>>>> g_thread_supported () is a macro, so we cannot load it with dlsym
>>>>>> g_thread_get_initialized () was introduced in 2.20, but we have to
>>>>>> support versions < 2.20
>>>>>>
>>>>>> Currently in JDK we have an internal flag which protects us from such
>>>>>> multiple calls, but at least we
>>>>>> have Java FX which does not have access to this flag.
>>>>>>
>>>>>> The idea of the fix it to make single entry point for everyone who is
>>>>>> about to call g_thread_init () to
>>>>>> check if it has been called.
>>>>>>
>>>>>> Should we bring back g_thread_get_initialized () check for GLib >=
>>>>>> 2.20
>>>>>> for safety?
>>>>>>
>>>>>> [1]
>>>>>> https://developer.gnome.org/glib/stable/glib-Deprecated-Thread-APIs.html#g-thread-init
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> [2]
>>>>>> https://developer.gnome.org/glib/stable/glib-Deprecated-Thread-APIs.html#g-thread-supported
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> [3]
>>>>>> https://developer.gnome.org/glib/stable/glib-Deprecated-Thread-APIs.html#g-thread-get-initialized
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>
>>