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

Alexander Zvegintsev alexander.zvegintsev at oracle.com
Thu Jun 30 07:34:04 UTC 2016


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/3c048b04/attachment.html>


More information about the swing-dev mailing list