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

Thu Jul 7 18:37:41 UTC 2016

Hello,

I got approval for the backport to JDK8 from Rob McKenna (see
http://mail.openjdk.java.net/pipermail/jdk8u-dev/2016-July/005686.html).
The backport can just be applied after reshuffling the patch. Test succeeds
after applying the patch.

Note that for the reshuffling, you need to add the following line

jdk/src/java.desktop/macosx/classes/com/apple/laf :
jdk/src/macosx/classes/com/apple/laf

to common/bin/unshuffle_list.txt .

As I have no commit rights, I am looking for someone willing to do the
backport to JDK8.

Link to the JDK9 webrev:
http://cr.openjdk.java.net/~alexsch/robin.stevens/8158325/webrev.01

Thanks,

Robin

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

> 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>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> 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>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/20160707/e1d78fe1/attachment.html>