<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