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; + } +}
