NPE when dragging widgets inside Groups, Tabs or Arrays.

[claudio-rosati]

The problem is caused by not unregistering listeners when widgets are disposed.

Specifically it happens in SelectedWidgetUITracker:429-430. The widgets dragged into a Group, for example, are removed from their original parent (e.g. Display) and added to the new one (e.g. Group). When removed they are disposed but the listeners previously registered are not unregistered, so those listeners still keep alive the previous representation that now has its toolkit set to null. When added to the new parent they are initialised, adding new listeners. So, when the widget is asked for a redraw the new and the old location listeners are invoked, and the old one will throws the NPE.

What I've done is to add the unregisterListeners to the JFXBaseRepresentation and to the derived class Ellipse (just for test).

@kasemir Please don't merge this PR yet: I've created it to explain the problem to you. I'm working on adding unregisterListeners to all the representation classes. I'll tell you when finished.

[kasemir]

The WidgetRepresentation.dispose() which sets toolkit=null was added recently when squashing memory leaks. I wonder if there's another solution that doesn't require updating every widget's representation.

[claudio-rosati]

I think no, also because otherwise a lot of listener having a reference to the representation can keep representations from being garbage collected. Moreover it is a bit asymmetrical not having unregisterListeners when the register one exists. Anyway I've almost finished to add unregisterListeners to all widgets.

[kasemir]

There's certainly nothing wrong with balancing the listener addition with the corresponding removal. But it's a lot of work now and for every new widget.

The problem right now is that we keep the widget with all its properties and listeners. As it's re-parented, we delete the representation and then create a new representation. Instead, I think we could: 1) Clone the widget 2) Delete the representation 3) Delete the old widget 4) Add the cloned widget 5) Represent the cloned widget

That way, a widget is only represented once, and then completely torn down. No old listeners to a deleted representation.

[claudio-rosati]

It seems to me too much, in any case there is a risk that somewhere else a representation can be removed still referencing through it listeners the widget, so it will never be really removed. If you give me a couple of hours I'll finish adding listeners removal to all widgets and tested all with D&D into group, tabs and arrays. Then you can decide if merge it or not.

[kasemir]

Well, I'm afraid there has been the unspoken idea that a widget is only represented once. The basic fix to the SelectedWidgetUITracker is quite small:

https://github.com/shroffk/phoebus/commit/088f28b277eeae9a543e8c372ce3c78bfd2e83ca#diff-99436c4a4f01c7e84450823d6874f62a

It removes and deletes the old widget, then adds a cloned widget. So the old representation and old widget are gone, a new widget is added which creates a new representation.

Essentially just 2 lines of code to update the 'widget' to a cloned copy.

[claudio-rosati]

OK, so close the PR and go ahead with that implementation. I'll test it as soon as available. Anyway I still think that leaving the asymmetry with listeners is not good from an architectural point of view, and it could generate memory leaks in the future.

[kasemir]

Well, I'm on the fence. You're right about the symmetry. Will see how well the alternate solution works once it's tested.

[claudio-rosati]

OK. I've few classes left with the removal of listeners, and I'll do it as a comparison. Moreover, nothing impeded to have both.

[kasemir]

Well, I'm afraid adding & then removing the listeners is the only way.

In the phoebus repo I tried to only update the SelectedWidgetUITracker, and the fundamental change is really small: Remove the old widget, add a new cloned widget. There are a few more details to update the tracker and selection, but overall it's still not a lot of code: https://github.com/shroffk/phoebus/compare/editor_represent_once?expand=1

But in the end, it remains incomplete. For example: 1) Add a Label 2) Change the label's text 3) Move the label into a group 4) Undo the move.. OK 5) Undo the text change.. Problem, because the re-parenting move replaced the widget, destroyed the original label representation. In step 4 the old label is added back, with a new representation, and then step 5 triggers the old representation...

==> We have to add & remove all listeners in a symmetric way.

Let's merge this, and then I update phoebus accordingly.

[claudio-rosati]

OK. I'll tell you when completed.

[kasemir]

Regarding the Tooltip.uninstall, did you find that's really necessary? From the Tooltip javadoc: "Note that the Tooltip does not have to be uninstalled: it will be garbage collected when it is not referenced by any Node. It is possible to manually uninstall the tooltip, however."

[claudio-rosati]

I can remove it. I added only because when the registerListeners is done, one will duplicate it to realise the unregisterListeners, and I thought having a corresponding method to remove the tooltip was helping. Let me know.

[kasemir]

Let's leave it in. May not be 100% necessary, but can't hurt, either. May even speed up GC or stop timers related to tool tip behavior.

[claudio-rosati]

That's my hope too. I still have 22 representations to complete. I'll do it tomorrow morning.

[kasemir]

I've started on the phoebus version. Many thanks for identifying this problem and working through all the widgets!

[claudio-rosati]

@kasemir I've completed the addition of unregisterListeners() to all widget representations.

[kasemir]

Great, thanks. I've updated the phoebus version. Will try to compare the two to see where you or I missed something.

[claudio-rosati]

OK, thank you. Maybe after the comparison you can merge one implementation into the other to have them identical.