<AWT Dev> [9] Review Request: 8165769 Hang in the help menu item

[Semyon Sadetsky]

On 16.11.2016 17:52, Sergey Bylokhov wrote:
> On 16.11.16 17:32, Semyon Sadetsky wrote:
>>> There are a few issues which can cause a hang in some of our menu
>>> stress tests:
>>>  - Lack in synchronization in the Menu, we tried to synchronized the
>>> write of fields, but skip synchronization when we read the fields(in
>>> getters for example). In the fix most of the fields were marked as
>>> final(if possible) or volatile.
>> Since the synchronized blocks cannot be simply omitted the adding
>> volatile keyword to the fields doesn't improve multithreading capability
>> of menu components and the most of the proposed changes practically
>> useless.
>
> Please clarify the statement above.
I can give you an example:

CheckboxMenuItem.java

  243     public synchronized void addItemListener(ItemListener l) {
  244         if (l == null) {
  245             return;
  246         }
  247         itemListener = AWTEventMulticaster.add(itemListener, l);
  248         newEventsOnly = true;
  249     }

we are enabling event by calling the method above on thread A. The 
method is synchronized.

at the same time on thread B dispatchEventImpl(AWTEvent e) is called 
which is not synchronized at all.
If thread A is paused by the scheduler at line 248 then thread B may be 
executed and the event will be skipped in dispatchEventImpl(AWTEvent e) 
because of newEventsOnly=false still.
By making newEventsOnly and itemListener fields volatile you only 
guarantee their visibility for thread B but this doesn't make 
newEventsOnly change atomic along with adding an item listener.

Same for enableEvents(long eventsToEnable) method.
>
>>
>> Changing package private accesses to private in general improves
>> encapsulation and may be approved. But multithreading issues should be
>> fixed differently.
>> Actually the MenuComponent appContext field is never changed according
>> to its spec:
>> /**
>>      * The {@code AppContext} of the {@code MenuComponent}.
>>      * This is set in the constructor and never changes.
>>      */
>>     transient AppContext appContext;
>>
>> I suggest to declare it final and also this will be enough to make it
>> thread safe.
>
> But in some corner cases we change this value, so it cannot be final.
What is that corner case? The comment clearly states that it is never 
changed.
>
>>
>> Note that Component appContext field can be changed and this
>> multithreading issue should be resolved. Since the filed is accessed
>> only using the component accessor it should be enough to synchronize the
>> corresponding getter and setter.
Also it seems Menu#isHelpMenu field is never used except for toString() 
and may be removed.
>>
>> --Semyon
>>>  - When the submenu is removed from Menu/MenuBar we do not reset its
>>> parent field if the Menu/MenuBar have peer==null. So if later we tried
>>> to call MenuBar.setHelpMenu(submenu) we skip this submenu because we
>>> think it was added already.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8165769
>>> Webrev can be found at:
>>> http://cr.openjdk.java.net/~serb/8165769/webrev.00
>>>
>>
>
>