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

[Artem Ananiev]

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