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