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

[Alexandr Scherbatiy]

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 
> <mailto: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
>>     <mailto: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
>>>         <mailto: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/4c935f9e/attachment.html>