<Swing Dev> [8] Review request for JI-9038899: [macosx]Memory leak in ScreenMenu
Robin Stevens
robin.stevens at scz.be
Wed Jun 22 15:14:37 UTC 2016
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.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/243e7a63/attachment.html>
More information about the swing-dev
mailing list