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.scherba...@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-...@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.scherba...@oracle.com>alexandr.scherba...@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.zvegint...@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.zvegint...@oracle.com>alexander.zvegint...@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.scherba...@oracle.com>alexandr.scherba...@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 >>>>> >>>>> >>>>> >>>>> >>>>> >>>> >>>> >>> >>> >> >> >> > >
