<Swing Dev> [9] Review request for JDK-8158325: [macosx]Memory leak in com.apple.laf.ScreenMenu

[Robin Stevens]

Hello,

attached you find an updated webrev which addresses the comments:

- no custom remove implementation, but instead call fItems.clear() after
calling removeAll()
- Attached the container listener to the popupmenu
- Used the key instead of the value to remove items from the hashmap
- The test is now marked to run only on OS X


Robin

On Wed, Jun 29, 2016 at 2:01 PM, Alexander Zvegintsev <
alexander.zvegintsev at oracle.com> wrote:

> You should create the diff against the repository. This will allow to test
> your fix without applying a bunch of patches.
>
> --
> Thanks,
> Alexander.
>
> On 06/29/2016 02:49 PM, Robin Stevens wrote:
>
> Hello Alexander,
>
> just one last question. I assume I need to send a new webrev . But do I
> have to create one which contains the diff compared to the current tip of
> the repository, or do I need to create one which contains the diff compared
> to my previous patch ?
>
> Robin
>
> On Wed, Jun 29, 2016 at 12:03 PM, Alexander Zvegintsev <
> <alexander.zvegintsev at oracle.com>alexander.zvegintsev at oracle.com> wrote:
>
>> Hello Robin,
>>
>> Actually I missed your review, when I've posted mine.
>>
>> I think that we should proceed with your review as it was the first one.
>> So please disregard my review request.
>>
>> On 6/29/16 12:40 PM, Robin Stevens wrote:
>>
>> Hello Alexandr, Semyon,
>>
>> 2 reviews of this proposed path have happened.
>>
>> One from Alexandr Scherbatiy who stated that the fix looked good.
>> One from Alexander Zvegintsev who had some comments, and immediately
>> mailed his own review with a modified version of my proposed patch (see
>> <http://mail.openjdk.java.net/pipermail/swing-dev/2016-June/006196.html>
>> http://mail.openjdk.java.net/pipermail/swing-dev/2016-June/006196.html).
>> His patch is based on my patch, but implements the comments he had.
>>
>> I am not sure what I need to do now.
>> I can address his comments, but then I would end up with the same patch
>> as he proposed in
>> http://mail.openjdk.java.net/pipermail/swing-dev/2016-June/006196.html .
>> Please let me know how to proceed with this.
>>
>> Thanks,
>>
>> Robin
>>
>> On Wed, Jun 29, 2016 at 11:16 AM, Alexandr Scherbatiy <
>> <alexandr.scherbatiy at oracle.com>alexandr.scherbatiy at oracle.com> wrote:
>>
>>> On 6/29/2016 11:43 AM, Semyon Sadetsky wrote:
>>>
>>> It looks like that fix is posted twice for the same issue...
>>>
>>> Which one is the correct one?
>>>
>>>   It should be the first contributed fix. We are just waiting fro the
>>> response from the fix contributor:
>>>
>>> http://mail.openjdk.java.net/pipermail/swing-dev/2016-June/006200.html
>>>
>>>   Thanks,
>>>   Alexandr.
>>>
>>> --Semyon
>>>
>>> On 6/23/2016 7:08 PM, Robin Stevens wrote:
>>>
>>> Hello all,
>>>
>>> attached is a webrev for issue JDK-8158325 Memory leak in
>>> com.apple.laf.ScreenMenu: removed JMenuItems are still referenced.
>>>
>>> Patch contains a test case which reveals the bug, and a fix.
>>>
>>> There were a few issues with the ScreenMenu class:
>>>
>>> - The ContainerListener was attached to the JMenu and not to the JMenu#getPopupMenu.
>>> The JMenu itself does not fire any ContainerEvents, but
>>>
>>> the popup does. As a result, the cleanup code in ScreenMenu was never
>>> triggered. The patch fixes this by attaching the ContainerListener to
>>> the popup menu.
>>>
>>> Note that the ScreenMenu class also attaches a ComponentListener to the JMenu.
>>> I had no idea whether that one must be attached to the popup menu as well,
>>> so I did not change it.
>>>
>>> - The cleanup code was not triggered when removeAll() was called from
>>> the updateItems method. I fixed this by overriding the remove(int)
>>> method, and
>>>
>>> putting the cleanup code in that method. An alternative here would be
>>> to not override the remove(int) method, but instead call fItems.clear()
>>> after calling removeAll() . However, overriding the remove(int) method
>>> sounded more robust to me.
>>>
>>> - The cleanup code was incorrect. It tried to remove an item from fItems
>>> (a map) by calling remove with the value instead of the key. Now the
>>> remove is called with the key. Because the cleanup code has been moved,
>>> this required me to loop over the map as I have no direct access to the
>>> key in the
>>>
>>> remove(int) method
>>>
>>> - The test can be run on all platforms, although it was written for an
>>> OS X specific bug. As it can run on all platforms, I did not disable it
>>> on non OS X platforms. Let me know if I need to adjust this.
>>>
>>> Kind regards,
>>>
>>> Robin
>>>
>>>
>>>
>>>
>>>
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20160629/4162aa62/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: webrev.zip
Type: application/zip
Size: 70801 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20160629/4162aa62/webrev.zip>