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

Thu Jun 30 16:58:26 UTC 2016

Thanks for reviewing, and pushing this fix.

My application which bumped into this memory leak is however still running
on JDK8.
Is this a kind of fix that can still be backported ? If so, do I just need
to send a new webrev to the mailinglist for the JDK8 repository and have it
reviewed ?

On Thu, Jun 30, 2016 at 6:06 PM, Alexander Scherbatiy <
alexandr.scherbatiy at oracle.com> wrote:

> The fix looks good to me.
>
> Thanks,
> Alexandr.
>
>
> On 30/06/16 11:34, Alexander Zvegintsev wrote:
>
> The fix looks good to me.
>
> On 6/30/16 9:07 AM, Alexandr Scherbatiy wrote:
>
> On 6/29/2016 10:08 PM, Robin Stevens wrote:
>
> 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
>
>   The uploaded webrev:
>     http://cr.openjdk.java.net/~alexsch/robin.stevens/8158325/webrev.01
>
>   Thanks,
>   Alexandr.
>
>
> Robin
>
> On Wed, Jun 29, 2016 at 2:01 PM, Alexander Zvegintsev <
> <alexander.zvegintsev at oracle.com>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> 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>
>>> 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>
>>>> 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/20160630/c60168a0/attachment.html>