<Swing Dev> [8] Review request for JI-9038899: [macosx]Memory leak in ScreenMenu
Robin Stevens
robin.stevens at scz.be
Wed Jun 8 07:17:27 UTC 2016
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.stevens at 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;
> + }
> +}
>
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20160608/903d9073/attachment.html>
More information about the swing-dev
mailing list