RFR: 8242621: TabPane: Memory leak when switching skin
Jeanette Winzenburg
fastegal at openjdk.java.net
Tue Oct 13 15:06:14 UTC 2020
On Tue, 13 Oct 2020 12:56:10 GMT, Ambarish Rapte <arapte at openjdk.org> wrote:
> `TabPaneSkin` installs some listeners that are not removed when `TabPaneSkin` is changed.
> The fix converts listeners to WeakListeners and also removes them on dispose.
>
> There is a NPE check change needed in `isHosrizontal()`. Without this check it causes NPE if pulse is in progress while
> TabPaneSkin is getting disposed.
> `SkinMemoryLeakTest` already had a test which only needed to be enabled.
> Test fails before and passes after this change.
no review yet, just a couple of quick comments from my experience on recent cleanup of skins:
- if NPE checks seem to be necessary, they always indicate an illegal state: whatever a method is doing, it must not
access the skin after dispose. Usually it's the caller of the method that's misbehaving, it simply must not happen.
It's worth digging why _exactly_ that's happening and if/how it can be solved without guarding against the null
- while the overall memory test is already done, we still must test every single skin against side-effects (f.i. of
listeners not doing any "late" update due to not being yet removed - the NPE above could well be such a case). Have a
look at SkinCleanupTest for examples
- when changing listener wiring, it's often a good idea to test that they are still doing there job (if not yet covered
in the available tests)
yeah, you don't get away with not writing tests *good-humored-grinning
-------------
PR: https://git.openjdk.java.net/jfx/pull/318
More information about the openjfx-dev
mailing list