On Sat, 13 Jun 2026 16:31:35 GMT, Michael Strauß <[email protected]> wrote:
>> I agree, but my thought behind was not the GC, but rather this:
>>
>> We can change just the skin at runtime.
>> In this case we would still have stale references to the `disclosureNode`
>> here in the `Map`.
>> And the `disclosureNode` can still exist after we changed the skin, as the
>> `TableRow` is unchanged (and the `disclosureNode` comes from it). Then this
>> skin might not be GC'ed at all.
>>
>> Also it is allowed to re-use a skin. So e.g. we could replace the skin of
>> `TableRow` with another one, and then back to the previous one.
>> I don't think this will work right now because everything is done in the
>> constructor (and not in `install()`), but that might be possible in the
>> future.
>>
>> I agree both are rare situation and something you most likely don't do.
>> But since it does not hurt to clear/null the fields here, and we are
>> improving the skin in case it is not possible to dispose (GC) it
>> immediately, I think we should rather do it and are safe for whatever a
>> developer might do.
>
>> I agree, but my thought behind was not the GC, but rather this:
>>
>> We can change just the skin at runtime. In this case we would still have
>> stale references to the `disclosureNode` here in the `Map`. And the
>> `disclosureNode` can still exist after we changed the skin, as the
>> `TableRow` is unchanged (and the `disclosureNode` comes from it). Then this
>> skin might not be GC'ed at all.
>
> The disclosure node must not strongly reference the skin after the skin is
> removed. If it does, it's a bug that needs to be fixed, because that would
> mean that a removed skin will stick around and not be collected. I don't
> think there is a bug here: it's okay if the skin strongly references the
> disclosure node; it's just the other way that would be a problem.
>
>> Also it is allowed to re-use a skin. So e.g. we could replace the skin of
>> `TableRow` with another one, and then back to the previous one. I don't
>> think this will work right now because everything is done in the constructor
>> (and not in `install()`), but that might be possible in the future.
>
> No, it's not allowed to reuse a skin. In the `SkinBase(C)` constructor, the
> `control` field is assigned to hold the associated control. This field is
> nulled in `SkinBase.dispose()`, so installing the same skin again on the
> control will fail with NPE if the `control` field is dereferenced at any
> point.
>
>> I agree both are rare situation and something you most likely don't do. But
>> since it does not hurt to clear/null the fields here, and we are improving
>> the skin in case it is not possible to dispose (GC) it immediately, I think
>> we should rather do it and are safe for whatever a developer might do.
>
> It _does_ hurt to null fields of a disposed object, because it suggests that
> this would do anything meaningful (like fixing a memory leak). But that's not
> the case: if there was a memory leak (i.e. if the skin wasn't eligible for GC
> and therefore kept its referenced objects alive), nulling the field would
> just hide the problem. The solution in that case would be to ensure that a
> disposed skin is eligible for GC.
Just to make it more clear what exactly I was worried about, this situation:
treeTableRow.setDisclosureNode(node)
└── TreeTableRowSkin picks it up
└── stores in Map<Node, FadeTransition>
treeTableRow.setSkin(new TreeTableRowSkin(treeTableRow))
└── Old Skin disposed, new TreeTableRowSkin picks it up
└── stores in Map<Node, FadeTransition> as well
Old skin has the reference and new one has.
I did run some tests and could not find a problem. Still was a bit worried as
it was not always clear how to correctly write the skin so it follows the best
practices.
I think in the past the people probably chose to rather clear/null everything
to be safe. Or, I saw a comment somewhere IRC that this was done to 'help' the
GC. So yeah, a bit out of place and I personally then just followed the `null
everything` pattern as I saw it quite often.
Example in the super class of `TableRowSkinBase`:
https://github.com/openjdk/jfx/blob/2abb9a3b0a7e1071bea0b8bf2ca3d83e6fdf25f8/modules/javafx.controls/src/main/java/javafx/scene/control/skin/LabeledSkinBase.java#L246-L252
> No, it's not allowed to reuse a skin
Yes (although this can be achieved when extending from `Skin` and using the
`install` method correctly - too much work). Just brought it up so we think
about everything. But does not affect us in any way.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/2177#discussion_r3409860752
