Memory Leaks

Scope

Many skin implementations misbehave when replacing them at runtime. The misbehavior is various, f.i.:

Memory leaks

in the control/skin pair the control is the long-lived and the skin the short-lived partner - so leaks will happen if the control (or any of its properties) holds a strong reference to the skin.

Runtime errors after dispose

happens if the skin is disposed without removing a listener/eventHandler that calls methods on the skin

Runtime errors on replace

happens on contract violation of dispose and/or incomplete cleanup of children

Lessons learned

Listeners

all listeners added by the skin/behavior - weak or not - must be removed on dispose.

EventHandlers

all eventHandlers added by the skin/behavior must be removed on dispose.

Children

(suspected) all children added to the control must be removed.

Memory Leaks in Skins and Behaviors

Comment from Kevin

In general, there are two approaches to avoiding listener-related memory leaks. One is to use a WeakListener; the other is to explicitly remove the listener when the object is removed or otherwise no longer needed. Using a WeakListener is certainly easier, but runs the risk of the listener being removed too early and not cleaning up after itself. I’m not suggesting that’s the case here, but it is worth looking at. The one thing I would ask you to take a look at is whether it would matter if the old skin didn’t call setDefaultButton(oldScene, false) when removed (and similarly setCancelButton).

Removed too early

As specified by the api doc of WeakListener: using code must keep a strong reference to the actual listener.

Cleanup after itself

The Trouble with Weak Listeners

• accidentally not having a strong listener (in chained bindings f.i.)

• reactFx (and later Var/Val) has better support

• in a comment: removed only if gc actually is triggered - which might happen exactly at the time to listener is notified at which moment there’s a strong reference to weakListener leading to not being garbage collected

Only removed when change happens

resolved as wontfix

• listener only removed when a change happens (which might not be the case for a long-lived property that’s rarely to never changed, f.i. Locale that all nodes need to listen to)

• from the other end (my digging): thought that expressionHelperBase.trim would remove garbage-collected weak listener - but doesn’t: relies on the weaklistener removing itself - should it? Actually, trim does work - but only if the sizeOfListenerArray == oldCapacity

Patterns used in Skins

Skin API

registerChangeListener builds a chain of consumers (one chain per property) that’s registered as one (weakListener?) and explicitly removed in dispose. As long as the property has the same life-cycle as the control, we don’t need to bother: it’s either removed as well when the control dies or the listener is removed when only the skin goes out of strongly reachable scope.

One drawback is that - while registered as ChangeListener - the information about the oldValue is lost. Another might be that it’s hard to remove/replace/overload listener behavior without relying on implementation knowledge.

Manual Weak/List/ChangeListener

ChangeListener<Some> someChangeListener = (ov, oldSome, newSome) -> {
   if (oldSome != null) {
      cleanup(oldSome);
   }
   if (newSome != null) [
      doStuff(newSome);
   }
}
WeakChangeListener<Some> weakSomeChangeListener = new WeakChangeListener<>(someChangeListener);

// in constructor/initialize
control.someProperty().addListener(weakSomeChangeListener);

The options in dispose

• do nothing (not an option - all manually added listeners must be removed, see below)

• remove listener, even though it is weak

  control.someProperty().removeListener(weakSomeChangeListener);

• cleanup and remove listener

  if (control.getSome() != null) {
     cleanup(control.getSome());
  }
  control.someProperty().removeListener(weakSomeChangeListener);

Turned out that the first is not an option: in most contexts it might not cause a memory leak, but still have malicious side-effects (like f.i. NPEs, IOOBEs, ..) in the time between skin disposal and the listener not yet gc’ed. After dispose, the skin is in an invalid state, it’s invalid to call any of its methods (public or private doesn’t matter).

A typical example for a pattern causing an NPE without explicit cleanup/removal:

InvalidationListener someListener = e -> updateSome();
WeakInvalidationListener weakSomeListener = new WeakInvalidationListener(someListener);

// adding
control.someProperty().addListener(weakSomeListener);

// update method
private void updateSome() {
    // NPE
    getSkinnable().doStuff();
}

Anti-Patterns and their Fixes

Intention to Remove, but Failing

There are several variants:

Removing a different listener than has been added

// in constructor
getNode().focusedProperty().addListener(this::focusChanged);

// in dispose
getNode().focusedProperty().removeListener(this::focusChanged);

The lambdas are different instances, so the removal doesn’t do anything. Fix is to extract the listener into a field.

Removing an eventHandler in a different role than it was added

private final EventHandler<KeyEvent> keyEventListener = e -> {
      ...
}

// in constructor
control.addEventFilter(KeyEvent.ANY, keyEventListener);

// in dispose
control.removeEventHandler(KeyEvent.ANY, keyEventListener);

Obvious fix is to take great care to add/remove the same role ;)

Listener not removable

Happens if they are created on-the-fly, doesn’t really matter whether strong or weak wrappers around the strong - as soon as there’s no reference to added listener (or eventHandler) it cannot be removed.

Adding a WeakListener

itemsObserver = observable -> updateChoiceBoxItems();
control.itemsProperty().addListener(new WeakInvalidationListener(itemsObserver));

Adding a Listener as lambda

From PaginationSkin

control.maxPageIndicatorCountProperty().addListener(o -> {
    resetIndiciesAndNav();
});