<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