<Swing Dev> [8] Review request for JI-9038899: [macosx]Memory leak in ScreenMenu

Alexandr Scherbatiy alexandr.scherbatiy at oracle.com
Wed Jun 22 18:05:12 UTC 2016


On 6/22/2016 8:59 PM, Alexandr Scherbatiy wrote:
> 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
   The issue has been recorder under JDK id [3]:
     JDK-8158325 Memory leak in com.apple.laf.ScreenMenu: removed 
JMenuItems are still referenced

   [3] https://bugs.openjdk.java.net/browse/JDK-8158325

   Thanks,
   Alexandr.
>
>   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> 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/3bc1851e/attachment.html>


More information about the swing-dev mailing list