Bug ID: JDK-8241364 ☂ Cleanup skin implementations to allow switching

JDK-8241364 : ☂ Cleanup skin implementations to allow switching

Type: Task
Component: javafx
Sub-Component: controls

Priority: P4
Status: Open
Resolution: Unresolved

Submitted: 2020-03-20
Updated: 2023-12-19

Versions (Unresolved/Resolved/Fixed)

The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.

Other
tbdUnresolved

Related Reports

Blocks :	JDK-8245303 - InputMap: memory leak due to incomplete cleanup on remove mapping
Blocks :	JDK-8294809 - ListenerHelper for managing and disconnecting listeners
Blocks :	JDK-8295175 - SplitPaneSkin: memory leak when changing skin
Blocks :	JDK-8295531 - ComboBoxBaseSkin: memory leak when changing skin
Blocks :	JDK-8295754 - PaginationSkin: memory leak when changing skin
Blocks :	JDK-8295796 - ScrollPaneSkin: memory leak when changing skin
Blocks :	JDK-8295806 - TableViewSkin: memory leak when changing skin
Blocks :	JDK-8295809 - TreeTableViewSkin: memory leak when changing skin
Blocks :	JDK-8236840 - Memory leak when switching ButtonSkin
Blocks :	JDK-8240506 - TextFieldSkin/Behavior: misbehavior on switching skin
Blocks :	JDK-8242621 - TabPane: Memory leak when switching skin
Blocks :	JDK-8244657 - ChoiceBox/ToolBarSkin: misbehavior on switching skin
Blocks :	JDK-8245145 - Spinner: throws IllegalArgumentException when replacing skin
Blocks :	JDK-8245282 - Button/Combo Behavior: memory leak on dispose
Blocks :	JDK-8246195 - ListViewSkin/Behavior: misbehavior on switching skin
Blocks :	JDK-8246202 - ChoiceBoxSkin: misbehavior on switching skin, part 2
Blocks :	JDK-8246745 - ListCell/Skin: misbehavior on switching skin
Blocks :	JDK-8247576 - Labeled/SkinBase: misbehavior on switching skin
Blocks :	JDK-8253634 - TreeCell/Skin: misbehavior on switching skin
Blocks :	JDK-8256821 - TreeViewSkin/Behavior: misbehavior on switching skin
Blocks :	JDK-8268877 - TextInputControlSkin: incorrect inputMethod event handler after switching skin
Blocks :	JDK-8269081 - Tree/ListViewSkin: must remove flow on dispose
Blocks :	JDK-8273071 - SeparatorSkin: must remove child on dispose
Blocks :	JDK-8274061 - Tree-/TableRowSkin: misbehavior on switching skin
Blocks :	JDK-8283551 - ControlAcceleratorSupport menu items listener causes memory leak
Blocks :	JDK-8295242 - ScrollBarSkin: memory leak when changing skin
Blocks :	JDK-8295500 - AccordionSkin: memory leak when changing skin
Blocks :	JDK-8295506 - ButtonBarSkin: memory leak when changing skin
Blocks :	JDK-8294589 - MenuBarSkin: memory leak when changing skin
Duplicate :	JDK-8088856 - Umbrella : issues related to dynamic skin replacement
Relates :	JDK-8244419 - TextAreaSkin: throws UnsupportedOperation on dispose
Relates :	JDK-8290844 - Add Skin.install() method
Relates :	JDK-8244531 - Tests: add support to identify recurring issues with controls et al
Relates :	JDK-8292213 - Add Skin.install() method

Description

Identify and correct the dispose() of different control skins which do not cleanup correctly when disposed.

Some cleanups are to remove the,
1. listeners added to control's property, Use registerChangeListener() wherever possible.
2. event handlers added to control.
3. bindings created with controls property.
4. KeyCombination added to Scene.getAccelerators().

5. return if getSkinnable() == null, so that multiple calls to dispose() do not cause NPE. JDK-8244112
6. children added repeatedly JDK-8245145
7. ineffective removal of listeners JDK-8245282

Please update the list as and when a new cleanup is encountered.

Comments

+1 for default method. public interface Skin<C extends Skinnable> { /** gets invoked by Control.setSkin() after the old skin is removed by its dispose(). this method might be implemented whenever the new skin needs to set a property, or a handler on the control. */ default public void install() { } }
18-07-2022
We can and should discuss the value proposition for this. The biggest concern is behavioral compatibility with existing applications and third-party skins. As long as this is done in an "opt-in" manner, such that existing skins continue to operate in the same way as they do today, then it seems like it might be feasible. Existing skins could either be left alone or they could "opt in" to the new behavior. Similarly new skins could add a concrete "install" method, or not. For this to happen, Skin::install would need to be a no-op default method, and the core JavaFX built-in controls would need to work correctly in the face of an install method that does nothing. One other comment about compatibility: > This proposal breaks compatibility with java8 and all the versions where we do not back port. JDK 8 is not relevant to this discussion, since there is no public Skin API in 8. The concern is limited to applications that build with JavaFX 11 or later.
18-07-2022
The price for keeping the existing Skin design is maintenance, memory leaks, and problems down the road with custom- or customer- defined skins (notice a long list of issues linked to this ticket). I'd like to propose a public API change to fix these issues once and for all. 1. Add public void install(); method to Skin interface. 2. Add explanation on what should and should not be in the constructor and what should be in the new install() to javadoc. Emphasize that the actual installation code better be in install(). 2. Modify Control.setSkin(Skin) method to call the new install() method after oldSkin.dispose() A lot of existing skins - those that only add listeners or nodes in their constructors - would not have to change, or we could touch them all to move the installation code to the install() method. This proposal breaks compatibility with java8 and all the versions where we do not back port. Alternatively, we could invent a new interface, Skin_v2 extends Skin with the new install() method. We cannot add the new method to SkinBase as there are skins that implement Skin interface directly, which do not extend SkinBase (ContextMenuSkin is an example).
18-07-2022
It is not that we break 1:1 relationship between control and skin. The problem is not that we have listeners from two skins attached, or that we added two sets of nodes. The problem is that doing all that in the skin's constructor is premature - and it corrupts the state of the control when control has a property that is being set by a skin. To illustrate, here is the sequence of events in the current design: A1) create new skin and install new skin. some information from the currently installed skin is lost. A2) remove old skin This sort of works because most of the time the operation is reversible regardless of the order it is performed - adding a listener, removing a listener. adding nodes, removing nodes. But for some controls, skin do set a property - and doing so in step A1) corrupts the state, by irreversibly overriding a property. The right sequence of events should be: B1) create new skin B2) uninstall old skin B3) install new skin In my experience, the price of not fixing a bad design costs more than fixing it. Just look at the long list of bugs in this ticket - memory leaks and loss of functionality. I am in favor of fixing the design, perhaps by versioning the Skin interface (Skin_v2?).
18-07-2022
agree that we have a problem here: when instantiating a skin, there might be two skins attached to the control at the same time (happens always when a skin is to be replaced) which violates the 1:1 relationship between control and skin. Might be sub-optimal design, oversight or intention, doesn't really matter, IMO, because we are stuck with the current state specification-wise. Adding api probably will break existing apps and require a major rewrite of the skins (didn't think it through to the end, but that's what my gut tells me :) .. so we might also consider option 3: do something on the implementation level which mitigates the problems with minimal effect on spec/existing apps - like f.i. let skinBase dispose any already attached skin on the control
17-07-2022
Please check out my comments in JDK-8268877. I believe we are dealing with sub-optimal design - a lack of Skin.install() method. While it's ok to add listeners to various properties of the corresponding Control in the skin constructor, it is not ok to mutate its properties, for example by setting event handlers. In Swing, every UI class has install() and uninstall(), designed specifically to separate instantiation and initialization. We have two choices, generally speaking: 1. change the public API by adding Skin.install() and calling in after the old skin is dispose()'d. I know [~kcr] is very reluctant to change the public APIs. 2. keep fixing issues by implementing tricky workarounds, and wait for more failures in the field.
15-07-2022
+1! The longer version: good idea to collect potential incarnations of the problem (and patterns on how to fix them if available) here :) Just fyi: I'm keeping a loose collection of thoughts on mishaviors (and potential fixes) - it's a moving bullet, though https://github.com/kleopatra/swingempire-fx/wiki/Memory-Leaks
02-06-2020
>>> Cleanup skin implementations to allow switching. What do you think? This sounds good to me. I will update the title and bug description. And I think we should list all the different kind of cleanups here in the description as and when we encounter when addressing all skins.
11-05-2020
Would suggest to widen this task a bit: the underlying issue is incomplete cleanup of a skin implementation (and maybe its behaviour as well) when "removed" from a control. A memory leak is one of several potential misbehaviors, possibly the tip of the iceberg, others are: - exceptions (mostly NPEs but might be others) from listeners that are still listening (when the state of the control they had been attached to changes) - exceptions from eventHandlers that are still active - maybe others?? Unfortunately, they are not necessarily orthogonal to each other, can be unrelated or smeared across implementations (even their super). So I don't think it manageable to have a task for each of the misbehaviors. Instead we might include them here, change the title to something like: Cleanup skin implementations to allow switching. What do you think? As to memory leaks, a first test (not yet fully stable, working at it in JDK-8244531) seems to indicate that 14 of the 41 skins in control.skin package do not leak - so there's quite a task ahead.
08-05-2020