Re: [8] Review request for JI-9038899: [macosx]Memory leak in ScreenMenu

[Alexandr Scherbatiy]

On 6/22/2016 8:59 PM, Alexandr Scherbatiy wrote:

On 6/22/2016 6:14 PM, Robin Stevens wrote:

Hello all,

my OCA has just been accepted.
Let me know if there are any other steps I need to take to get this 
review started.
The issue should be fixed in JDK 9 first. Could you prepare a 
webrev [1] for the fix  against the JDK 9 client repository [2].


  [1] http://openjdk.java.net/guide/webrevHelp.html
  [2] http://hg.openjdk.java.net/jdk9/client/jdk

  The issue has been recorder under JDK id [3]:
JDK-8158325 Memory leak in com.apple.laf.ScreenMenu: removed 
JMenuItems are still referenced


  [3] https://bugs.openjdk.java.net/browse/JDK-8158325

  Thanks,
  Alexandr.


  Thanks,
  Alexandr.


Kind regards,

Robin

On Wed, Jun 8, 2016 at 9:17 AM, Robin Stevens > wrote:


Hello all,

are there any additional steps I need to take to get a review of
this patch started ?

Kind regards,

Robin

On Tue, May 31, 2016 at 5:01 PM, Robin Stevens
 wrote:

Hello,

I created a patch for a bug I just logged through
http://bugs.java.com/ (still under review with identifier
JI-9038899).

The com.apple.laf.ScreenMenu class keeps hard references to
JMenuItems which have been removed from the JMenu.

The patch contains a fix for the memory leak and a test case
which reveals the issue.
The attached patch is a diff against
http://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/rev/4d6c03fb1039 .

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

PS: I only just started contributing. Let me know if I did
something incorrect in my workflow.

Patch (output of hg diff -g):

diff --git a/src/macosx/classes/com/apple/laf/ScreenMenu.java
b/src/macosx/classes/com/apple/laf/ScreenMenu.java
--- a/src/macosx/classes/com/apple/laf/ScreenMenu.java
+++ b/src/macosx/classes/com/apple/laf/ScreenMenu.java
@@ -29,6 +29,8 @@
 import java.awt.event.*;
 import java.awt.peer.MenuComponentPeer;
 import java.util.Hashtable;
+import java.util.Map;
+import java.util.Set;

 import javax.swing.*;

@@ -109,6 +111,7 @@
 final Component[] items = fInvoker.getMenuComponents();
 if (needsUpdate(items, childHashArray)) {
 removeAll();
+childHashArray = null;
 if (count <= 0) return;

 childHashArray = new int[count];
@@ -232,7 +235,7 @@
 synchronized (getTreeLock()) {
 super.addNotify();
 if (fModelPtr == 0) {
- fInvoker.addContainerListener(this);
+ fInvoker.getPopupMenu().addContainerListener(this);
fInvoker.addComponentListener(this);
 fPropertyListener = new
ScreenMenuPropertyListener(this);
fInvoker.addPropertyChangeListener(fPropertyListener);
@@ -266,13 +269,35 @@
 if (fModelPtr != 0) {
removeMenuListeners(fModelPtr);
 fModelPtr = 0;
- fInvoker.removeContainerListener(this);
+ fInvoker.getPopupMenu().removeContainerListener(this);
fInvoker.removeCompone

Re: [8] Review request for JI-9038899: [macosx]Memory leak in ScreenMenu

[Alexandr Scherbatiy]

On 6/22/2016 6:14 PM, Robin Stevens wrote:

Hello all,

my OCA has just been accepted.
Let me know if there are any other steps I need to take to get this 
review started.
The issue should be fixed in JDK 9 first. Could you prepare a 
webrev [1] for the fix  against the JDK 9 client repository [2].


  [1] http://openjdk.java.net/guide/webrevHelp.html
  [2] http://hg.openjdk.java.net/jdk9/client/jdk

  Thanks,
  Alexandr.


Kind regards,

Robin

On Wed, Jun 8, 2016 at 9:17 AM, Robin Stevens > wrote:


Hello all,

are there any additional steps I need to take to get a review of
this patch started ?

Kind regards,

Robin

On Tue, May 31, 2016 at 5:01 PM, Robin Stevens
mailto:robin.stev...@scz.be>> wrote:

Hello,

I created a patch for a bug I just logged through
http://bugs.java.com/ (still under review with identifier
JI-9038899).

The com.apple.laf.ScreenMenu class keeps hard references to
JMenuItems which have been removed from the JMenu.

The patch contains a fix for the memory leak and a test case
which reveals the issue.
The attached patch is a diff against
http://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/rev/4d6c03fb1039 .

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

PS: I only just started contributing. Let me know if I did
something incorrect in my workflow.

Patch (output of hg diff -g):

diff --git a/src/macosx/classes/com/apple/laf/ScreenMenu.java
b/src/macosx/classes/com/apple/laf/ScreenMenu.java
--- a/src/macosx/classes/com/apple/laf/ScreenMenu.java
+++ b/src/macosx/classes/com/apple/laf/ScreenMenu.java
@@ -29,6 +29,8 @@
 import java.awt.event.*;
 import java.awt.peer.MenuComponentPeer;
 import java.util.Hashtable;
+import java.util.Map;
+import java.util.Set;

 import javax.swing.*;

@@ -109,6 +111,7 @@
 final Component[] items = fInvoker.getMenuComponents();
 if (needsUpdate(items, childHashArray)) {
 removeAll();
+childHashArray = null;
 if (count <= 0) return;

 childHashArray = new int[count];
@@ -232,7 +235,7 @@
 synchronized (getTreeLock()) {
 super.addNotify();
 if (fModelPtr == 0) {
- fInvoker.addContainerListener(this);
+ fInvoker.getPopupMenu().addContainerListener(this);
fInvoker.addComponentListener(this);
 fPropertyListener = new
ScreenMenuPropertyListener(this);
fInvoker.addPropertyChangeListener(fPropertyListener);
@@ -266,13 +269,35 @@
 if (fModelPtr != 0) {
removeMenuListeners(fModelPtr);
 fModelPtr = 0;
- fInvoker.removeContainerListener(this);
+ fInvoker.getPopupMenu().removeContainerListener(this);
fInvoker.removeComponentListener(this);
fInvoker.removePropertyChangeListener(fPropertyListener);
 }
 }
 }
-
+
+@Override
+public void remove(int index) {
+Men

Re: [8] Review request for JI-9038899: [macosx]Memory leak in ScreenMenu

[Robin Stevens]

Hello all,

my OCA has just been accepted.
Let me know if there are any other steps I need to take to get this review
started.

Kind regards,

Robin

On Wed, Jun 8, 2016 at 9:17 AM, Robin Stevens  wrote:

> Hello all,
>
> are there any additional steps I need to take to get a review of this
> patch started ?
>
> Kind regards,
>
> Robin
>
> On Tue, May 31, 2016 at 5:01 PM, Robin Stevens 
> wrote:
>
>> Hello,
>>
>> I created a patch for a bug I just logged through http://bugs.java.com/
>> (still under review with identifier JI-9038899).
>>
>> The com.apple.laf.ScreenMenu class keeps hard references to JMenuItems
>> which have been removed from the JMenu.
>>
>> The patch contains a fix for the memory leak and a test case which
>> reveals the issue.
>> The attached patch is a diff against
>> http://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/rev/4d6c03fb1039 .
>>
>> 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
>>
>> PS: I only just started contributing. Let me know if I did something
>> incorrect in my workflow.
>>
>> Patch (output of hg diff -g):
>>
>> diff --git a/src/macosx/classes/com/apple/laf/ScreenMenu.java
>> b/src/macosx/classes/com/apple/laf/ScreenMenu.java
>> --- a/src/macosx/classes/com/apple/laf/ScreenMenu.java
>> +++ b/src/macosx/classes/com/apple/laf/ScreenMenu.java
>> @@ -29,6 +29,8 @@
>>  import java.awt.event.*;
>>  import java.awt.peer.MenuComponentPeer;
>>  import java.util.Hashtable;
>> +import java.util.Map;
>> +import java.util.Set;
>>
>>  import javax.swing.*;
>>
>> @@ -109,6 +111,7 @@
>>  final Component[] items = fInvoker.getMenuComponents();
>>  if (needsUpdate(items, childHashArray)) {
>>  removeAll();
>> +childHashArray = null;
>>  if (count <= 0) return;
>>
>>  childHashArray = new int[count];
>> @@ -232,7 +235,7 @@
>>  synchronized (getTreeLock()) {
>>  super.addNotify();
>>  if (fModelPtr == 0) {
>> -fInvoker.addContainerListener(this);
>> +fInvoker.getPopupMenu().addContainerListener(this);
>>  fInvoker.addComponentListener(this);
>>  fPropertyListener = new ScreenMenuPropertyListener(this);
>>  fInvoker.addPropertyChangeListener(fPropertyListener);
>> @@ -266,13 +269,35 @@
>>  if (fModelPtr != 0) {
>>  removeMenuListeners(fModelPtr);
>>  fModelPtr = 0;
>> -fInvoker.removeContainerListener(this);
>> +fInvoker.getPopupMenu().removeContainerListener(this);
>>  fInvoker.removeComponentListener(this);
>>  fInvoker.removePropertyChangeListener(fPropertyListener);
>>  }
>>  }
>>  }
>> -
>> +
>> +@Override
>> +public void remove(int index) {
>> +MenuItem item;
>> +synchronized (getTreeLock()) {
>> +item = getItem(index);
>> +}
>> +super.remove(index);
>> +if(item != null){
>> +Set> entrySet =
>> fItems.entrySet();
>> +Component keyToRemove = null;
>> +for(Map.Entry entry : entrySet){
>> +if(entry.getValue() == item){
>> +keyToRemove=entry.getKey();
>> +break;
>> +}
>> +}
>> +if(keyToRemove != null){
>> +fItems.remove(keyToRemove);
>> +}
>> +}
>> +}
>> +
>>  /**
>>   * Inv

Re: [8] Review request for JI-9038899: [macosx]Memory leak in ScreenMenu

[Robin Stevens]

Hello all,

are there any additional steps I need to take to get a review of this patch
started ?

Kind regards,

Robin

On Tue, May 31, 2016 at 5:01 PM, Robin Stevens  wrote:

> Hello,
>
> I created a patch for a bug I just logged through http://bugs.java.com/
> (still under review with identifier JI-9038899).
>
> The com.apple.laf.ScreenMenu class keeps hard references to JMenuItems
> which have been removed from the JMenu.
>
> The patch contains a fix for the memory leak and a test case which reveals
> the issue.
> The attached patch is a diff against
> http://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/rev/4d6c03fb1039 .
>
> 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
>
> PS: I only just started contributing. Let me know if I did something
> incorrect in my workflow.
>
> Patch (output of hg diff -g):
>
> diff --git a/src/macosx/classes/com/apple/laf/ScreenMenu.java
> b/src/macosx/classes/com/apple/laf/ScreenMenu.java
> --- a/src/macosx/classes/com/apple/laf/ScreenMenu.java
> +++ b/src/macosx/classes/com/apple/laf/ScreenMenu.java
> @@ -29,6 +29,8 @@
>  import java.awt.event.*;
>  import java.awt.peer.MenuComponentPeer;
>  import java.util.Hashtable;
> +import java.util.Map;
> +import java.util.Set;
>
>  import javax.swing.*;
>
> @@ -109,6 +111,7 @@
>  final Component[] items = fInvoker.getMenuComponents();
>  if (needsUpdate(items, childHashArray)) {
>  removeAll();
> +childHashArray = null;
>  if (count <= 0) return;
>
>  childHashArray = new int[count];
> @@ -232,7 +235,7 @@
>  synchronized (getTreeLock()) {
>  super.addNotify();
>  if (fModelPtr == 0) {
> -fInvoker.addContainerListener(this);
> +fInvoker.getPopupMenu().addContainerListener(this);
>  fInvoker.addComponentListener(this);
>  fPropertyListener = new ScreenMenuPropertyListener(this);
>  fInvoker.addPropertyChangeListener(fPropertyListener);
> @@ -266,13 +269,35 @@
>  if (fModelPtr != 0) {
>  removeMenuListeners(fModelPtr);
>  fModelPtr = 0;
> -fInvoker.removeContainerListener(this);
> +fInvoker.getPopupMenu().removeContainerListener(this);
>  fInvoker.removeComponentListener(this);
>  fInvoker.removePropertyChangeListener(fPropertyListener);
>  }
>  }
>  }
> -
> +
> +@Override
> +public void remove(int index) {
> +MenuItem item;
> +synchronized (getTreeLock()) {
> +item = getItem(index);
> +}
> +super.remove(index);
> +if(item != null){
> +Set> entrySet =
> fItems.entrySet();
> +Component keyToRemove = null;
> +for(Map.Entry entry : entrySet){
> +if(entry.getValue() == item){
> +keyToRemove=entry.getKey();
> +break;
> +}
> +}
> +if(keyToRemove != null){
> +fItems.remove(keyToRemove);
> +}
> +}
> +}
> +
>  /**
>   * Invoked when a component has been added to the container.
>   */
> @@ -289,9 +314,7 @@
>  final Component child = e.getChild();
>  final MenuItem sm = fItems.get(child);
>  if (sm == null) return;
> -
>  remove(sm);
> -fItems.remove(sm);
>  }
>
>  /**
> diff --git a/test/com/apple/laf/ScreenMenu

[8] Review request for JI-9038899: [macosx]Memory leak in ScreenMenu

[Robin Stevens]

Hello,

I created a patch for a bug I just logged through http://bugs.java.com/
(still under review with identifier JI-9038899).

The com.apple.laf.ScreenMenu class keeps hard references to JMenuItems
which have been removed from the JMenu.

The patch contains a fix for the memory leak and a test case which reveals
the issue.
The attached patch is a diff against
http://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/rev/4d6c03fb1039 .

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

PS: I only just started contributing. Let me know if I did something
incorrect in my workflow.

Patch (output of hg diff -g):

diff --git a/src/macosx/classes/com/apple/laf/ScreenMenu.java
b/src/macosx/classes/com/apple/laf/ScreenMenu.java
--- a/src/macosx/classes/com/apple/laf/ScreenMenu.java
+++ b/src/macosx/classes/com/apple/laf/ScreenMenu.java
@@ -29,6 +29,8 @@
 import java.awt.event.*;
 import java.awt.peer.MenuComponentPeer;
 import java.util.Hashtable;
+import java.util.Map;
+import java.util.Set;

 import javax.swing.*;

@@ -109,6 +111,7 @@
 final Component[] items = fInvoker.getMenuComponents();
 if (needsUpdate(items, childHashArray)) {
 removeAll();
+childHashArray = null;
 if (count <= 0) return;

 childHashArray = new int[count];
@@ -232,7 +235,7 @@
 synchronized (getTreeLock()) {
 super.addNotify();
 if (fModelPtr == 0) {
-fInvoker.addContainerListener(this);
+fInvoker.getPopupMenu().addContainerListener(this);
 fInvoker.addComponentListener(this);
 fPropertyListener = new ScreenMenuPropertyListener(this);
 fInvoker.addPropertyChangeListener(fPropertyListener);
@@ -266,13 +269,35 @@
 if (fModelPtr != 0) {
 removeMenuListeners(fModelPtr);
 fModelPtr = 0;
-fInvoker.removeContainerListener(this);
+fInvoker.getPopupMenu().removeContainerListener(this);
 fInvoker.removeComponentListener(this);
 fInvoker.removePropertyChangeListener(fPropertyListener);
 }
 }
 }
-
+
+@Override
+public void remove(int index) {
+MenuItem item;
+synchronized (getTreeLock()) {
+item = getItem(index);
+}
+super.remove(index);
+if(item != null){
+Set> entrySet =
fItems.entrySet();
+Component keyToRemove = null;
+for(Map.Entry entry : entrySet){
+if(entry.getValue() == item){
+keyToRemove=entry.getKey();
+break;
+}
+}
+if(keyToRemove != null){
+fItems.remove(keyToRemove);
+}
+}
+}
+
 /**
  * Invoked when a component has been added to the container.
  */
@@ -289,9 +314,7 @@
 final Component child = e.getChild();
 final MenuItem sm = fItems.get(child);
 if (sm == null) return;
-
 remove(sm);
-fItems.remove(sm);
 }

 /**
diff --git a/test/com/apple/laf/ScreenMenu/ScreenMenuMemoryLeakTest.java
b/test/com/apple/laf/ScreenMenu/ScreenMenuMemoryLeakTest.java
new file mode 100644
--- /dev/null
+++ b/test/com/apple/laf/ScreenMenu/ScreenMenuMemoryLeakTest.java
@@ -0,0 +1,106 @@
+/*
+ * @test
+ * @summary [macosx] Memory leak in com.apple.laf.ScreenMenu
+ * @run main/timeout=300/othervm -Xmx12m ScreenMenuMemoryLeakTest
+ */
+import java.awt.EventQueue;
+import java.lang.ref.WeakReferenc