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