<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 17:18:37 UTC 2016
On 30/06/16 20:58, Robin Stevens wrote:
> 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 ?
Try to backport the fix using the unshuffle_patch scrpt [1].
If no more changes are required just send the request for approval to
the jdk8u-dev at openjdk.java.net alias using the template [2] and example [3].
[1] http://cr.openjdk.java.net/~chegar/docs/portingScript.html
[2] http://openjdk.java.net/projects/jdk8u/approval-template.html
[3] http://mail.openjdk.java.net/pipermail/jdk8u-dev/2016-June/005652.html
Thanks,
Alexandr.
>
>
>
> On Thu, Jun 30, 2016 at 6:06 PM, Alexander Scherbatiy
> <alexandr.scherbatiy at oracle.com
> <mailto: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
>>> <http://cr.openjdk.java.net/%7Ealexsch/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/95d3498a/attachment.html>
More information about the swing-dev
mailing list