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 ?
On Thu, Jun 30, 2016 at 6:06 PM, Alexander Scherbatiy < 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>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> 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 >>>> >>>> >>>> >>>> >>>> >>> >>> >> >> > > >
