Trouble with garbage collection on ComboBox

[pbauerochse]

Hi there,

I think I'm having trouble with fields being garbage collected although they are still in use. Maybe I'm just misusing Mvvmfx.

I have a ComboBox, and two Datepicker fields in my Fxml View. The ComboBox contains date presets, so when the user selects one of the presets, the date fields should change accordingly.

This view is included in a main view via <fx:include />

<ComboBox fx:id="presetComboBox" />
<DatePicker fx:id="startDatePicker" />
<DatePicker fx:id="endDatePicker" />

Here is the CodeBehind

@FxmlPath("/ui/views/main/toolbar/Toolbar.fxml")
class ToolbarView : FxmlView<ToolbarViewModel>, Initializable {

    lateinit var presetComboBox: ComboBox<ToolbarTimespan>
    lateinit var startDatePicker: DatePicker
    lateinit var endDatePicker: DatePicker

    @InjectViewModel
    private lateinit var viewModel: ToolbarViewModel

    override fun initialize(location: URL?, resources: ResourceBundle?) {
        presetComboBox.items = viewModel.timespans
        presetComboBox.converter = ToolbarTimespanConverter(viewModel.timespans)
        presetComboBox.valueProperty().bindBidirectional(viewModel.selectedTimespan)

        startDatePicker.valueProperty().bindBidirectional(viewModel.startDate)
        endDatePicker.valueProperty().bindBidirectional(viewModel.endDate)
    }

And here is the ViewModel

class ToolbarViewModel {

    val timespans: ObservableList<ToolbarTimespan> = FXCollections.observableArrayList()
    val selectedTimespan: ObjectProperty<ToolbarTimespan> = SimpleObjectProperty()
    val startDate: ObjectProperty<LocalDate> = SimpleObjectProperty()
    val endDate: ObjectProperty<LocalDate> = SimpleObjectProperty()

    override fun initialize() {
        timespans.setAll(createTimespanItems())
        selectedTimespan.addListener { _, _, timespan -> updateDateFields(timespan) }
    }

    private fun updateDateFields(timespan: ToolbarTimespan) {
        LOGGER.debug("Selected time span changed to $timespan")
        timespan.startDate?.let { startDate.value = it }
        timespan.endDate?.let { endDate.value = it }
    }

[...]

Now, when I change the ComboBox selected Item, my Log Statements in the ViewModel don't fire, so the Listener does not seem to be invoked. I traced it down to some WeakReference that seems to be garbage collected.

I tried to debug it by having the ViewModel inherit from SceneLifecycle and thought i would see the View being removed but it does not. But it was not removed but I noticed, that all of a sudden my Listener gets invoked.

Do you have any hints what I'm doing wrong?

Thanks in advance

[manuel-mauky]

Hi, yes this is a known problem that comes from the way JavaFX works. I've created an issue #430 some time ago to point out to this issue and share some ideas for tools that the mvvmFX framework could provide so support here. However, I haven't implemented anything yet.

So what's the problem? When JavaFX loads an FXML file it looks at the controller class, injects all components and executes the initialize method. From the perspective of the FXMLoader the job of the controller class is done at this point. It doesn't hold any references to the controller instance. All that is there are at runtime are the instances of the UI components. The garbage collector will not collect ui components. If a ui component keeps a reference to the controller class then also the controller class won't be collected. But if this is not the case, then the controller class is free for collection.

So when does a ui component holds a reference to the controller class? One way is to add a listener. A listener is an inner class and inner classes in java have references to their parent class. However, bindings in JavaFX are using weak references internally so no reference to the parent class is created. So actually the problem is not the viewModel but the view/controller instance which is garbage collected first.

So how to solve this? Adding useless listeners of cause would be a solution but this is not really clean. A clearer solution that I prefer is this:

1) (optional but cleaner) inject the root node of the FXML file in your controller class 2) add the controller class as "user-data" to the root node using setUserData method. See javadoc

Example (java code not kotlin):

class MyView implements FxmlView<MyViewModel> {
    @FXML
    private Parent rootNode;

    public void initialize() {
        rootNode.setUserData(this); 
    }
}

"user-data" is there to store arbitrary data and I think this is a good use-case. The setUserData method is available on all JavaFX nodes so instead of injecting the root-node you could also invoke it on one of your already injected ui components (you combobox for example) and it would have the same effect. However, in my oppinion it's not that clean because this "hack" doesn't belong to this specific component but to the whole view. Also it might be a good idea to add a comment because it's not obvious what this line means.

This should solve the problem as now the view instance won't be garbage collected and as the view has a reference to the viewModel, the viewModel won't be collected too.

[pbauerochse]

Hi @lestard thanks for your in-depth explanation. I was afraid it might be something very complex and not too easy to fix.

I think adding the view as userdata is not very intuitive and I would have to remember doing this for most of the views. Then when I'll look at my code in a couple of years (or even months) I might wonder why the hell I did that.

With the SceneLifecycle workaround I could at least create an interface like

interface GarbageCollectionSafeView extends SceneLifecycle {
    // default implementations of the two lifecycle methods
}

simply leave a comment in there referring to this issue and know right away why I did that.

Do you see any drawbacks with just implementing SceneLifecycle? Is that more likely to create memory leaks? I assume under the hood this is the "useless listeners" solution you were referring to, isn't it?

[manuel-mauky]

I don't see any drawbacks from using SceneLifecycle. We prevent the view from being garbage collected only until the onViewRemoved method is invoked. After that the framework removes it's reference to it so it shouldn't introduce memory leaks.

But to be honest, I don't have much experience or feedback from other users for this usage scenario as theSceneLifecycle is a relatively new feature and it was introduced for quite the opposite situtation than yours: When a controller should be garbage collected but couldn't because of some still existing reference from the view, you would add the lifecycle and then are able to explicitly remove the reference so that the controller can be collected afterwards. The behaviour of the SceneLifecycle in combination with a situation where the controller is prematurally garbage collected is quite new for me. On the other hand your usage is not that uncommon and the garbage collection problem is a known issue and the lifecycle should solve it so I think it's fine.

Besides that, there shouldn't be any other drawbacks from using the SceneLifecycle. Of cause there is a small overhead added (a listener is registered for every view that implements the interface) but I don't see how this could actually be a problem for performance.