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

[Sergey Bylokhov]

On 17.11.16 11:27, Semyon Sadetsky wrote:
>> The statement above is incorrect, there is no "cache". I do not know
>> where you get "changed fields is guaranteed to be flushed upon exit
>> from the synchronized block". Also there is no guarantee that the
>> reader will see the latest version of the field if the reader will use
>> another mutex or will not use synchronization at all. In the fix
>> "volatile" will guarantee that the readers will see the latest version
>> which was set.
> Flushing the cache is reality. By adding volatile to all Menu* fields
> you didn't make the methods effects predictable in case of they are
> called concurrently because they are not synchronized between each
> other. After this change it still may not be recommended to use menus
> from different threads arbitrarily.

all above is wrong.

> Or if I'm wrong and this change is a real improvement why you don't add
> volatile to all AWT classes' fields?
> The rest AWT classes are not better synchronized than the menu onces.

There is a bug for all other components.
https://bugs.openjdk.java.net/browse/JDK-6765536

>>>> 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.
>>
>> But we still have a setter which is called by other code. Also it
>> cannot be made final because it is updated during de-serialization.
> And you still did not answer where it is really called. Probably the
> setter may be removed? At least please update the comment statement
> since in the change you assumes that the opposite is true.

Even if the setter will be removed the field cannot be final because it 
is updated during de-serialization. I will remove the comment about 
constructor before the push.

>
> Why you left without synchronization the analogous field in the
> Component class? This field is really modified I can give you examples.
>>
>>>>> 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.
>>
>> It is used to provide an information in the "string" that this menu is
>> a helpmenu. It is used in some tests as well. We also should take care
>> since this object is Serializable.
>>
>>>>
>>>>>>>
>>>>>>> --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
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
>


-- 
Best regards, Sergey.