Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8242621: TabPane: Memory leak when switching skin #318

Closed
wants to merge 5 commits into from

Conversation

@arapte
Copy link
Member

@arapte arapte commented Oct 13, 2020

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.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8242621: TabPane: Memory leak when switching skin

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jfx pull/318/head:pull/318
$ git checkout pull/318

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 13, 2020

👋 Welcome back arapte! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Oct 13, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 13, 2020

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Oct 13, 2020

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

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Oct 14, 2020

/reviewers 2

@openjdk
Copy link

@openjdk openjdk bot commented Oct 14, 2020

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).


getSkinnable().getTabs().removeListener(weakTabsListener);

getChildren().remove(tabHeaderArea);

This comment has been minimized.

@kevinrushforth

kevinrushforth Oct 15, 2020
Member

As mentioned offline, can you check whether removing tabHeaderArea is necessary?

This comment has been minimized.

@kleopatra

kleopatra Dec 15, 2020
Collaborator

good question :) Just have run into a similar case while working on cleaning up TextFieldSkin.

My current understanding is ... depends ;)

As tests show, some children keep the skin alive, others don't. Which must imply that the first somehow keep a strong reference to the skin, the latter (nor any of its children) don't. An example for the former is tabHeaderArea, an example for the latter is tabContentRegion. Was puzzled about how to distinguish the one from the other (which boils down to finding that strong reference) - until I (faintly ;) remembered that inner classes have an implicit reference to the enclosing instance: TabHeaderArea is an inner class with an implicit reference to the enclosing skin, while TabContentRegion is static nested. As long as the former reside in the scenegraph, it makes the skin leaky.

So we have to watch out for

  • explicit strong references from any node (down the hierarchy) to the skin
  • implicit strong reference to the enclosing skin class.

Both groups have to be removed in dispose to prevent memory leaks.

Even if not obviously leaking, children pile up when switching skin: most skins add to children, rarely set. We started a discussion of how to handle those that add Spinner misbehavior - not yet decided (personally, I didn't do anything about them in the cleanups, deferred to later ;) From today's knowledge, I would suggest to explicitly remove all direct children that the skin added to the control.

This comment has been minimized.

@arapte

arapte Dec 21, 2020
Author Member

As mentioned offline, can you check whether removing tabHeaderArea is necessary?

Both groups have to be removed in dispose to prevent memory leaks.

The list returned by SkinBase.getChildren() is actually the same list as Parent.getChildren().
SkinBase() constructor gets the reference of Parent.getChildren() and store it's reference in the class member named children. [see here]
So, Skin and Parent share the same list of children. So when a skin is being dispose()ed, it should remove any children that it added to the list. Otherwise, those children continue to be part of scenegraph.
Please check the newly added test SkinCleanupTest.testChildrenCountAfterSkinIsReplaced().

This does not stop a skin object from getting GCed, but the Children that skin adds don't get removed from Scenegraph. Looking at this behavior, it seems like this should be done for cleanup of all Skins.

This comment has been minimized.

@kleopatra

kleopatra Dec 21, 2020
Collaborator

quick nit-pick:

This does not stop a skin object from getting GCed,

actually, it does ... if it keeps a strong reference to the skin :)

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Dec 14, 2020

quick question: is this still in the lane for fx16?

@arapte
Copy link
Member Author

@arapte arapte commented Dec 15, 2020

quick question: is this still in the lane for fx16?

Yes, I will update PR later this week.

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Dec 15, 2020

quick question: is this still in the lane for fx16?

Yes, I will update PR later this week.

cool, looking forward to it :)

arapte added 2 commits Dec 21, 2020
@kevinrushforth kevinrushforth self-requested a review Dec 23, 2020
@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Jan 5, 2021

I'm still in holiday mood - and hope you are all well :) - will review later today, have to update my notes to the latest changes.

Copy link
Collaborator

@kleopatra kleopatra left a comment

Fix looks good, verified tests failing (except one, commented) before and passing after the fix.

Left a couple of comments inline. Plus there's an @ignore related to the issue in TabPaneTest (testNPEOnSwitchSkinAndChangeSelection) that should be removed.

arapte added 2 commits Jan 6, 2021
@arapte
Copy link
Member Author

@arapte arapte commented Jan 6, 2021

Fix looks good, verified tests failing (except one, commented) before and passing after the fix.

Thanks for the review. I have updated the PR according to all comments. Please take a look. :)

Copy link
Collaborator

@kleopatra kleopatra left a comment

looks fine now :)

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Looks good.

@openjdk
Copy link

@openjdk openjdk bot commented Jan 6, 2021

@arapte This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8242621: TabPane: Memory leak when switching skin

Reviewed-by: fastegal, kcr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 1 new commit pushed to the master branch:

  • e74f679: 8254101: Update copyright header for files modified in 2020

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Jan 6, 2021
@arapte
Copy link
Member Author

@arapte arapte commented Jan 7, 2021

/integrate

@openjdk openjdk bot closed this Jan 7, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jan 7, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 7, 2021

@arapte Since your change was applied there has been 1 commit pushed to the master branch:

  • e74f679: 8254101: Update copyright header for files modified in 2020

Your commit was automatically rebased without conflicts.

Pushed as commit c197b62.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants