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

Alexander Scherbatiy alexandr.scherbatiy at oracle.com
Thu Jun 30 16:06:20 UTC 2016


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> 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).
>>>>>         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> 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/20160630/9a4cb51b/attachment.html>


More information about the swing-dev mailing list