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
<mailto: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
<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.zvegint...@oracle.com
<mailto: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
<mailto: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).
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.scherba...@oracle.com
<mailto: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
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
