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

[Alexander Zvegintsev]

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