SWT menu disposal memory leak

SWT is a widget toolkit developed as part of Eclipse that enables you to develop Java desktop applications that use the OS’s native widgets. It is an immensely useful library and really shines in the area of resource management, which boils down to “only dispose resources you’ve created yourself”.

Well, sort of.

Rules are fun

An article from 2001 titled “Managing Operating System Resources” touches the subject of SWT resource disposal and defines a set of rules:

1. If you created it, you dispose it.
   Fair enough: if you create a Font instance, you dispose of it when you’re done with it. Create a Shell? You’ll be the one to dispose it.
2. Disposing the parent disposes the children.
   The SWT Control class has a parent field pointing to a parent control. This parent-child relationship is used for composition, rendering, disposal and more. Because you’d probably go crazy if you’d have to dispose of all the controls you’ve created yourself, this rule helps us dispose all the controls in a Shell, for example. Great! Not all widgets have clear parent-child relationship, however. The article gives us some examples to work with:
   • A Composite will dispose of all the Composite’s children.
   • A Menu disposes all underlying MenuItems.
   • Disposing a Tree or TreeItem disposes all child items.
3. Disposing a MenuItem disposes a referenced Menu.
   MenuItems of type SWT.CASCADE reference a Menu to show when the user selects the item. For convenience, the popup menu is disposed together with the MenuItem.
4. Disposing a Control disposes a referenced Menu.
   Similar to the previous rule, disposing a Control with an active Menu set disposes them both. You set an active menu with .setMenu(…).

Do you really know your parents?

Not too much has changed in the 14 years since Carolyn MacLeod and Steve Northover wrote those guidelines. But I discovered that it’s not always straightforward to apply these rules. For example, why are rules #3 and #4 necessary? To instantiate a cascading menu, you pass the MenuItem as its parent, and rule #2 takes care of disposal! Let’s take a look at the Menu constructor:

/**
 * Constructs a new instance of this class given its parent
 * (which must be a MenuItem) and sets the style
 * for the instance so that the instance will be a drop-down
 * menu on the given parent's parent menu.
 * (...)
 */
public Menu(MenuItem parentItem) {
    // ...
}

The parameter parentItem and the accompanying docblock seem clear. Let’s take a look at the constructor we use when we instantiate a Menu for another widget:

/**
 * Constructs a new instance of this class given its parent,
 * and sets the style for the instance so that the instance
 * will be a popup menu on the given parent's shell.
 * (...)
 */
public Menu(Control parent) {
	// ...
}

Couldn’t be clearer than this, right? Well, it turns out the parent relationship Menu has with other widgets is a bit awkward. Let’s step through the code of the last constructor:

// Class Menu

public Menu(Control parent) {
	this (checkNull(parent).menuShell(), SWT.POP_UP);
}

// Class Control

Decorations menuShell() {
	return parent.menuShell();
}

// Class Decorations

Decorations menuShell() {
	return this;
}

Menu travels up the tree resolving parents until it finds an instance of Decorations, which will then return itself. An instance of Menu will set its parent to the nearest shell of the provided parent. So now we know that disposing the parent you passed to Menu’s constructor does not always dispose of the Menu!

Memory leaks and you

Imagine the following UI setup:

• You design a new Composite widget that displays an image and offers a context menu to save or delete this image. These actions are defined in the widget.
• The context menu is instantiated within the widget, and uses the widget as its parent.
• The Menu is only set using .setMenu(…) on the widget on a right click of the user’s mouse.
• The widget is created and disposed within a single shell that stays open.

This happens:

• The Menu is instantiated and finds the nearest Shell (Decorations) to use as its parent.
• The user does not right click, so .setMenu(…) never happens.
• The Composite widget is disposed of.
• Because the Menu is not a child of the widget, it is not disposed.
• The MenuItems in the Menu still contain references to the Composite widget.

And now you have a memory leak:

• The Shell keeps track of its Menus.
• The Menu keeps track of its MenuItems.
• The MenuItems point to a disposed widget.
• The widget potentially has references to many other objects.

Only when the Shell is disposed, all those Menus in limbo are actually disposed. “Why not always set the menu with .setMenu(…)?”, you ask. Well, then it wouldn’t be possible to have more than one context menu for the same widget.

You can find these leaks using the amazing JVisualVM. If you’ve got a JDK installed, this already sits on your hard drive. Make a heap dump of your process, find a UI class that should have been disposed (sort by instance count), and look for this pattern in the references tab:

The references are listed from the widget’s point of view, so reverse them to discover the leak:

• Display keeps a list of items with event listeners.
• One of these items is a MenuItem with an event listener.
• The event listener points to a lambda used in the MainWordListEditor class.
• The lambda’s arg$1 field points to the MainWordListEditor instance, which should have been disposed.

An additional SWT disposal rule

I would like to propose a new rule to take care of all this nasty business:

If you create a potentially unreferenced menu, you dispose it.

Or, to put this into code:

Menu menu = new Menu(widget);
// ...
widget.addListener(SWT.Dispose, e -> menu.dispose());

Easy enough!