<Swing Dev> [8] Review request for JI-9038899: [macosx]Memory leak in ScreenMenu
Alexandr Scherbatiy
alexandr.scherbatiy at oracle.com
Wed Jun 22 17:59:07 UTC 2016
On 6/22/2016 6:14 PM, Robin Stevens wrote:
> 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.
The issue should be fixed in JDK 9 first. Could you prepare a
webrev [1] for the fix against the JDK 9 client repository [2].
[1] http://openjdk.java.net/guide/webrevHelp.html
[2] http://hg.openjdk.java.net/jdk9/client/jdk
Thanks,
Alexandr.
>
> Kind regards,
>
> Robin
>
> On Wed, Jun 8, 2016 at 9:17 AM, Robin Stevens <robin.stevens at scz.be
> <mailto:robin.stevens at 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.stevens at scz.be <mailto: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/20160622/9b08861f/attachment.html>
More information about the swing-dev
mailing list