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

[Semyon Sadetsky]

On 11/16/2016 7:43 PM, Sergey Bylokhov wrote:

> On 16.11.16 19:21, Semyon Sadetsky wrote:
>> 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.
>
> The example above produce the same result as if the thread B will call 
> dispatchEventImpl() early than addItemListener() was called by thread 
> A. And this is correct behavior(the new events will be proceeded only 
> when we set newEventsOnly to true).
> addItemListener is synchronized, because we need to synchronize access 
> to the list of listeners "itemListener" when we add/remove listener.
Then explain to me why to make all those fields volatile? Because the 
cache for the changed fields is guaranteed to be flushed upon exit from 
the synchronized block, so the changes will be visible to other threads 
when the method returns.
>
>>
>> Same for enableEvents(long eventsToEnable) method.
Also, the state field setter is synchronized by this object monitor 
while the peer object which it should notify is protected by another 
monitor and may be reset concurrently.
>
>>> 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.
>
> We have a setter and we call it in applets, trayicon and in X11.
But TryIcon is not a MenuComponent. It seems the comment is correct.
>
>>>
>>>>
>>>> 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.
>
> Why it can be removed since it is used in the toString()?
Because in this case it looks like cache anti-pattern and it should be 
replaced with the real value. For which purpose the toString() may be 
used? for debugging? But it seems one cannot guarantee that the cache is 
updated in each moment of time in case of multi-threading.
>
>>>>
>>>> --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
>>>>>
>>>>
>>>
>>>
>>
>
>