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 <robin.stev...@scz.be
<mailto:robin.stev...@scz.be>> 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
<robin.stev...@scz.be <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) {
+ MenuItem item;
+ synchronized (getTreeLock()) {
+ item = getItem(index);
+ }
+ super.remove(index);
+ if(item != null){
+ Set<Map.Entry<Component, MenuItem>> entrySet =
fItems.entrySet();
+ Component keyToRemove = null;
+ for(Map.Entry<Component, MenuItem> 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.WeakReference;
+import java.lang.reflect.InvocationTargetException;
+import java.util.Objects;
+
+import javax.swing.JFrame;
+import javax.swing.JLabel;
+import javax.swing.JMenu;
+import javax.swing.JMenuBar;
+import javax.swing.JMenuItem;
+import javax.swing.WindowConstants;
+
+public class ScreenMenuMemoryLeakTest {
+
+ private static byte[] sBytes;
+
+ private static WeakReference<JMenuItem> sMenuItem;
+ private static JFrame sFrame;
+ private static JMenu sMenu;
+
+ public static void main(String[] args) throws
InvocationTargetException, InterruptedException {
+ EventQueue.invokeAndWait(new Runnable() {
+ @Override
+ public void run() {
+ System.setProperty("apple.laf.useScreenMenuBar", "true");
+ showUI();
+ }
+ });
+
+ EventQueue.invokeAndWait(new Runnable() {
+ @Override
+ public void run() {
+ removeMenuItemFromMenu();
+ }
+ });
+ fillUpMemory();
+ JMenuItem menuItem = sMenuItem.get();
+ EventQueue.invokeAndWait(new Runnable() {
+ @Override
+ public void run() {
+ sFrame.dispose();
+ }
+ });
+ if (menuItem != null) {
+ throw new RuntimeException("The menu item should
have been GC-ed");
+ }
+ }
+
+ private static void showUI() {
+ sFrame = new JFrame();
+ sFrame.add(new JLabel("Some dummy content"));
+
+ JMenuBar menuBar = new JMenuBar();
+
+ sMenu = new JMenu("Menu");
+ JMenuItem item = new JMenuItem("Item");
+ sMenu.add(item);
+
+ sMenuItem = new WeakReference<>(item);
+
+ menuBar.add(sMenu);
+
+ sFrame.setJMenuBar(menuBar);
+
sFrame.setDefaultCloseOperation(WindowConstants.DO_NOTHING_ON_CLOSE);
+ sFrame.pack();
+ sFrame.setVisible(true);
+ }
+
+ private static void removeMenuItemFromMenu() {
+ JMenuItem menuItem = sMenuItem.get();
+ Objects.requireNonNull(menuItem, "The menu item
should still be available at this point");
+ sMenu.remove(menuItem);
+ }
+
+ /**
+ * Fill up the available heap space to ensure that any
Soft and
+ * WeakReferences gets cleaned up
+ */
+ private static void fillUpMemory() {
+ int size = 1000000;
+ for (int i = 0; i < 50; i++) {
+ System.gc();
+ System.runFinalization();
+ try {
+ sBytes = null;
+ sBytes = new byte[size];
+ size = (int) (((double) size) * 1.3);
+ } catch (OutOfMemoryError error) {
+ size = size / 2;
+ }
+ try {
+ if (i % 3 == 0) {
+ Thread.sleep(321);
+ }
+ } catch (InterruptedException t) {
+ // ignore
+ }
+ }
+ sBytes = null;
+ }
+}
