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

8236259: MemoryLeak in ProgressIndicator #71

Closed
wants to merge 7 commits into from

Conversation

@FlorianKirmaier
Copy link

FlorianKirmaier commented Dec 19, 2019

Hi everyone,

ticket: https://bugs.openjdk.java.net/browse/JDK-8236259

The fix itself is quite straight forward.
It basically just removed the listener which causes the leak.

The unit-test for the fix is a bit more complicated.

I added a library JMemoryBuddy https://github.com/Sandec/JMemoryBuddy (written by myself), which simplifies testing for memory leaks.
I think there are many places in the JavaFX-codebase that could highly benefit from this library.
It could also simplify many of the already existing unit tests.
It makes testing for memory-leaks readably and reliable.
It would also be possible to just copy the code of the library into the JavaFX-codebase.
It only contains a single class.

I also had to make a method public, to write the test. I'm open to ideas, how I could solve it differently.


Progress

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

Issue

Reviewers

  • Kevin Rushforth (kcr - Reviewer)
  • Ambarish Rapte (arapte - Reviewer)

Download

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

…oesn't get collected

With this fix the listener, which caused the memoryleak, is collected.
@bridgekeeper
Copy link

bridgekeeper bot commented Dec 19, 2019

👋 Welcome back fkirmaier! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request (refresh this page to view it).

@FlorianKirmaier FlorianKirmaier changed the title JDK-8236259: MemoryLeak in ProgressIndicator 8236259: MemoryLeak in ProgressIndicator Dec 19, 2019
@openjdk openjdk bot added the rfr label Dec 19, 2019
@mlbridge
Copy link

mlbridge bot commented Dec 19, 2019

…oesn't get collected

removed the hard reference to the fontlistener
Copy link
Member

kevinrushforth left a comment

I haven't looked at the fix itself, but there are two things that must be fixed before this PR can be considered:

  1. You need to remove the additional 3rd-party test library dependency.
  2. You need to revert the change that makes a test method public API.
build.gradle Outdated Show resolved Hide resolved
@FlorianKirmaier
Copy link
Author

FlorianKirmaier commented Dec 19, 2019

As mentioned, I could copy the class of the library into the JavaFX-Project. Alternatively, I could remove the unit-test entirely.

But to be honest, the current state related to memory-leaks in the JavaFX project is a bit questionable. JavaFX is filled with leaks and code which could have leaks.
I would like to add many more tests like this into the JavaFX-CodeBase. But without the class/library I can only write unreadable code filled with sleeps, Weakreferences etc., which would be sad because now I know how to do it better.

@kleopatra
Copy link
Contributor

kleopatra commented Dec 19, 2019

Florian, basically it's a single class isn't it? If so, it might be acceptable to add that class to the test.xx.infrastructure package (don't know if doing so would require a jbs issue)

@kleopatra
Copy link
Contributor

kleopatra commented Dec 19, 2019

for now, you could have a look at ProgressIndicatorTest (or ProgressSkinTest) attemptGC - might not be optimal (don't know enough about the dirty details of gc :) but gets the test failing/passing before/after fixing a memory leak

@FlorianKirmaier
Copy link
Author

FlorianKirmaier commented Dec 19, 2019

The point of having this GC-magic in a library is, that it's verified by the library that it's implementation is stable for all common JavaVersions. The attemptGC is basically bad practice. I think in the JavaFX-Codebase this kind of GC-Tests are invented about 10 times. No one knows which of these implementations are stable and their code is usually unreadable. I know it, I have done it multiple times.

Copying to Infrastructure would be fine for me. It removes some benefits, like easy upgrades, Travis-verified implementation etc. But it's probably the best short-term solution.

@kleopatra
Copy link
Contributor

kleopatra commented Dec 19, 2019

probably not invented so often - just c&p'd from searches of occurances of System.gc ;-)

@kevinrushforth
Copy link
Member

kevinrushforth commented Dec 19, 2019

Yes, exactly.

@FlorianKirmaier Proposing to add test class to the test infrastructure would be a reasonable alternative to pulling it in as a third-party dependency. I recommend separating it out into its own enhancement, with a separate JBS issue. I see that it has a dependency on the jdk.management module. As long as that dependency only surfaces as a test dependency, that won't be a problem. It would be nice if it were available to all modules, meaning that putting it somewhere in javafx.base would be good.

@dsgrieve
Copy link

dsgrieve commented Dec 19, 2019

I would urge caution about incorporating JMemoryBuddy without seeking out advice from GC experts. I have my doubts that it will work reliably given its reliance on System.gc(). (Opinion is my own, not my employer's).

@FlorianKirmaier
Copy link
Author

FlorianKirmaier commented Dec 19, 2019

@dsgrieve
It's worth mentioning that JavaFX already has many tests based on System.gc().
An advantage of having a testsuit as an library (or copyied from an library) is, that its stability is regulary verified by the travis builds for different JVMs.
The alternative would be to not test for memory-leaks at all which is much worse than having slightly unstable tests.
Maybe it can make sense to seperate these tests for leaks in an own testgroup.

I'm introducing this library in more and more projects. I never had problems with unstable tests.
I only had this kind of problem when I wrote the WeakReference/System.gc/sleep-logic for every single test.

@mlbridge
Copy link

mlbridge bot commented Dec 20, 2019

Mailing list message from Eric Bresie on openjfx-dev:

Would it just be better to run some code analysis tools on the code base to hunt for these? Like maybe sonarqube integratedas part of CI build analysis?

Eric Bresie
Ebresie at gmail.com

On December 19, 2019 at 4:23:14 PM CST, Florian Kirmaier <fkirmaier at openjdk.java.net> wrote:
On Thu, 19 Dec 2019 19:59:07 GMT, David Grieve <dgrieve at openjdk.org> wrote:

Yes, exactly.

@FlorianKirmaier Proposing to add test class to the test infrastructure would be a reasonable alternative to pulling it in as a third-party dependency. I recommend separating it out into its own enhancement, with a separate JBS issue. I see that it has a dependency on the `jdk.management` module. As long as that dependency only surfaces as a test dependency, that won't be a problem. It would be nice if it were available to all modules, meaning that putting it somewhere in `javafx.base` would be good.

I would urge caution about incorporating JMemoryBuddy without seeking out advice from GC experts. I have my doubts that it will work reliably given its reliance on System.gc(). (Opinion is my own, not my employer's).

@dsgrieve
It's worth mentioning that JavaFX already has many tests based on System.gc().
An advantage of having a testsuit as an library (or copyied from an library) is, that its stability is regulary verified by the travis builds for different JVMs.
The alternative would be to not test for memory-leaks at all which is much worse than having slightly unstable tests.
Maybe it can make sense to seperate these tests for leaks in an own testgroup.

I'm introducing this library in more and more projects. I never had problems with unstable tests.
I only had this kind of problem when I wrote the WeakReference/System.gc/sleep-logic for every single test.

-------------

PR: https://git.openjdk.java.net/jfx/pull/71

@FlorianKirmaier
Copy link
Author

FlorianKirmaier commented Dec 20, 2019

I highly doubt that a code analysis tool will find such memoryleaks.

@kevinrushforth
Copy link
Member

kevinrushforth commented Dec 20, 2019

I agree. Static analysis tools are quite limited in this regard, and are in now way a substitute for regression testing.

So the question is how best to test fixes for memory leaks at runtime. Our current approach can be best characterized as "ad hoc", and is not all that robust (although works well enough in most cases and is still much better than doing no testing at all). I would welcome discussion of a more robust approach for testing, but it should be decoupled from this bug fix, and discussed as a separate JBS Enhancement request.

@mlbridge
Copy link

mlbridge bot commented Dec 21, 2019

Mailing list message from Eric Bresie on openjfx-dev:

Accepting that static analysis tools are not best at finding memory
leaks...however they can find some easy to find ones. I was just wondering
if some of these can be run across the code base to find these "easy to
find" ones to help in the process.

Additionally, if there are certain checks that want to be flowed out across
the code base (i.e. checks for initialization, then having the analysis run
as part of a CI build jobs which could be periodically run and reported on
to might help maintain some level of code quality over time. I know this
can potentially find a lot of false failures but just thought it might be
worth considering at some point.

Eric Bresie
ebresie at gmail.com

On Fri, Dec 20, 2019 at 7:30 AM Kevin Rushforth <kcr at openjdk.java.net>
wrote:

On Fri, 20 Dec 2019 09:53:56 GMT, Florian Kirmaier <fkirmaier at openjdk.org>
wrote:

@dsgrieve
It's worth mentioning that JavaFX already has many tests based on
System.gc().
An advantage of having a testsuit as an library (or copyied from an
library) is, that its stability is regulary verified by the travis builds
for different JVMs.
The alternative would be to not test for memory-leaks at all which is
much worse than having slightly unstable tests.
Maybe it can make sense to seperate these tests for leaks in an own
testgroup.

I'm introducing this library in more and more projects. I never had
problems with unstable tests.
I only had this kind of problem when I wrote the
WeakReference/System.gc/sleep-logic for every single test.

I highly doubt that a code analysis tool will find such memoryleaks.

I agree. Static analysis tools are quite limited in this regard, and are
in now way a substitute for regression testing.

So the question is how best to test fixes for memory leaks at runtime. Our
current approach can be best characterized as "ad hoc", and is not all that
robust (although works well enough in most cases and is still much better
than doing no testing at all). I would welcome discussion of a more robust
approach for testing, but it should be decoupled from this bug fix, and
discussed as a separate JBS Enhancement request.

-------------

PR: https://git.openjdk.java.net/jfx/pull/71

@kevinrushforth
Copy link
Member

kevinrushforth commented Jan 6, 2020

The use of static analysis tools to catch certain types of problems is orthogonal to a regression test to validate a bug fix of a specific memory leak.

@FlorianKirmaier as mentioned previously, please file a new JBS Enhancement to propose adding a test utility for memory leak test. You can then start a discussion on the openjfx-dev mailing list.

As for fixing this memory leak, I recommend one of the following two approaches:

  1. Modify the PR without relying on any new utilities (meaning you would create yet-another adhoc test for memory leaks).
  2. Close this PR, wait for memory leak test utility to be integrated, and then resubmit the PR for this leak using that utility.

Obviously, approach 1 would get the fix in faster. You could then modify the test to use the new utility as part of implementing the utility.

@kevinrushforth
Copy link
Member

kevinrushforth commented Jan 24, 2020

/reviewers 2

@openjdk
Copy link

openjdk bot commented Jan 24, 2020

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

Removed the unit-tests because they introduce a new dependency
@FlorianKirmaier
Copy link
Author

FlorianKirmaier commented Jan 27, 2020

A little bit late ...
I have now removed unit-test and it's dependency.
I will add a ticket about adding them again.

@kevinrushforth
Copy link
Member

kevinrushforth commented Feb 4, 2020

Rather than removing the test, I was suggesting that you create a test for memory leaks using the same ad hoc approach that our other memory leak tests use. This could later be modified to use the new GC test utility as part of creating that utility. The pattern used in, for example, TabPaneHeaderLeakTest.java, works well enough, even though it repeats a fair amount of boilerplate code.

@scientificware scientificware mentioned this pull request Jan 8, 2020
6 of 15 tasks complete
readded unit-test without any dependency
@FlorianKirmaier
Copy link
Author

FlorianKirmaier commented Mar 2, 2020

I've now readded the unit-test. It based on the "InitialNodesMemoryLeakTest".

Command to execute: ./gradlew -PFULL_TEST=true -PUSE_ROBOT=true :systemTests:test --tests test.javafx.scene.control.ProgressIndicatorLeakTest

It's now part of the systemtests, because the memory-semantics for the tests in controls is changed due to the TestToolkit.

@kevinrushforth kevinrushforth self-requested a review Mar 4, 2020
Copy link
Member

kevinrushforth left a comment

The fix looks good to me. I've verified that it fixes the leak.

The newly added test looks good as well, with some comments left inline (formatting, a missing copyright header, and a couple other suggestions).

cleaned up code based on code review
@FlorianKirmaier
Copy link
Author

FlorianKirmaier commented Mar 10, 2020

@kevinrushforth
I've now cleaned it up based on your suggestions.
It would be great when Travis could validate the formatting.
It would save a lot of time for everyone.

@kevinrushforth
Copy link
Member

kevinrushforth commented Mar 10, 2020

Thanks, I'll take a look.

As for automating this, we no longer have Travis builds hooked up. I have a pending task to look into using GitHub Actions to do a sanity build / test (which would be a big help). We could look into adding a checkstyle task to that to check for at least some of the style guidelines.

@kevinrushforth
Copy link
Member

kevinrushforth commented Mar 10, 2020

@arapte or @kleopatra can one of you be the second reviewer?

updated comment based on codereview
removed create garbage
@arapte
arapte approved these changes Mar 13, 2020
@openjdk
Copy link

openjdk bot commented Mar 13, 2020

@FlorianKirmaier This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

8236259: MemoryLeak in ProgressIndicator

Reviewed-by: kcr, arapte
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /solves command.

Since the source branch of this PR was last updated there have been 86 commits pushed to the master branch. Since there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate 9ea7f96ec4794debe189d64aa0efffe1b6cb9f3b.

As you do not have Committer status in this project, an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kevinrushforth, @arapte) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Mar 13, 2020
@arapte
Copy link

arapte commented Mar 17, 2020

@FlorianKirmaier
Please integrate this PR by issuing /integrate command in a new comment. After that one of us can sponsor it.

@FlorianKirmaier
Copy link
Author

FlorianKirmaier commented Mar 17, 2020

/integrate

@openjdk openjdk bot added the sponsor label Mar 17, 2020
@openjdk
Copy link

openjdk bot commented Mar 17, 2020

@FlorianKirmaier
Your change (at version bfb6d1c) is now ready to be sponsored by a Committer.

@arapte
Copy link

arapte commented Mar 17, 2020

/sponsor

@openjdk openjdk bot closed this Mar 17, 2020
@openjdk openjdk bot added integrated and removed sponsor ready rfr labels Mar 17, 2020
@openjdk
Copy link

openjdk bot commented Mar 17, 2020

@arapte @FlorianKirmaier The following commits have been pushed to master since your change was applied:

  • 9ea7f96: 8240832: Remove unused applecoreaudio.md third-party legal file
  • 0fc1420: Merge
  • f8c235b: 8240631: Create release notes for JavaFX 14
  • 50e15fc: 8240466: AppJSCallback* apps launched by ModuleLauncherTest intermittently hang
  • e3026b9: 8240688: Remove the JavaBeanXxxPropertyBuilders constructors
  • cfa1193: 8236685: [macOs] Remove obsolete file dialog subclasses
  • f25e8cf: 8212034: Potential memory leaks in jpegLoader.c in error case
  • b2ac76a: 8240451: JavaFX javadoc build fails with JDK 14
  • cf0bba6: 8240211: Stack overflow on Windows 32-bit can lead to crash
  • 337ed72: 8237926: Potential memory leak of model data in javafx.scene.control.ListView
  • 960f039: 8208761: Update constant collections to use the new immutable collections
  • 10c9528: 8240265: iOS: Unnecessary logging on pinch gestures
  • 4c132cd: 8237889: Update libxml2 to version 2.9.10
  • 20328b3: 8240218: IOS Webkit implementation broken
  • 4c82af8: 8236832: [macos 10.15] JavaFX Application hangs on video play on Cata…
  • 9cd6f79: 8196586: Remove use of deprecated finalize methods from javafx property objects
  • ef2f9ce: 8238755: allow to create static lib for javafx.media on linux
  • c3ee1a3: 8239822: Intermittent unit test failures in RegionCSSTest
  • 3150562: 8235772: Remove use of deprecated finalize method from PiscesRenderer and AbstractSurface
  • 4eaff0d: 8239109: Update SQLite to version 3.31.1
  • d8e7f85: 8239454: LLIntData : invalid opcode returned for 16 and 32 bit wide instructions
  • 48ddd80: Merge
  • 35a01ca: 8228867: Fix mistakes in FX API docs
  • b5e65ec: 8238434: Ensemble: Update version of Lucene to 7.7.2
  • e986459: 8227619: Potential memory leak in javafx.scene.control.ListView
  • a74137a: 8236839: System menubar removed when other menubars are created or modified
  • 21d3b7e: 8237453: [TabPane] Incorrect arrow key traversal through tabs after reordering
  • e224e54: 8238526: Cherry pick GTK WebKit 2.26.3 changes
  • 6968e38: 8237469: Inherited styles don't update when node is moved
  • 3c55a7e: Merge
  • a4b9f24: 8237503: Update copyright header for files modified in 2020
  • 1749e85: 8237975: Non-embedded Animations do not play backwards after being paused
  • 2ab40c1: 8231513: JavaFX cause Keystroke Receiving prompt on MacOS 10.15 (Catalina)
  • aa91ebb: 8237944: webview native cl "-m32" unknown option for windows 32-bit build
  • 56267e1: 8237770: Error creating fragment phong shader on iOS
  • 95bf2c0: 8237782: Only read advances up to the minimum of the numHorMetrics or the available font data.
  • d05e8fc: 8237833: Check glyph size before adding to glyph texture cache
  • 1213ea7: Merge
  • 1823f6e: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
  • ca37c1f: 8238249: GetPrimitiveArrayCritical passed with hardcoded FALSE value
  • b96bc52: 8237003: Remove hardcoded WebAnimationsCSSIntegrationEnabled flag in DumpRenderTree
  • d303a21: Merge
  • 5a0e71b: 8237372: NullPointerException in TabPaneSkin.stopDrag
  • 79fc0d0: 8232824: Removing TabPane with strong referenced content causes memory leak from weak one
  • aa6f3a4: 8236912: NullPointerException when clicking in WebView with Button 4 or Button 5
  • 921389f: Merge
  • da99e24: 8237823: Mark TextTest.testTabSize as unstable
  • 66ac99f: Merge
  • 9ae37f1: 8236753: Animations do not play backwards after being stopped
  • b2d8564: 8233942: Update to 609.1 version of WebKit
  • f5ee963: 8157224: isNPOTSupported check is too strict
  • be22e85: 8237078: [macOS] Media build broken on XCode 11
  • 20325e1: Merge
  • 16cea41: Merge
  • 63520a0: Merge
  • 8a5344c: Merge
  • 2e0d01c: Merge
  • a96704e: Merge
  • 71fa9af: Merge
  • a3711e2: Merge
  • 2d5d7e0: Merge
  • 962bdd1: 8232214: Improved internal validations
  • 2d2b824: 8232128: Better formatting for numbers
  • 81f7738: 8232121: Better numbering system
  • 8e55294: Merge
  • 1325f11: Merge
  • f759dd9: Merge
  • fed185a: Merge
  • 592d745: Merge
  • a0b4b14: 8227473: Improve gstreamer media support
  • a557dd9: 8236733: Change JavaFX release version to 15
  • f907026: 8236808: javafx_iio can not be used in static environment
  • e6587ff: 8236448: Remove unused and repair broken Android/Dalvik code
  • c9519b6: 8232589: Remove CoreAudio Utility Classes
  • df8b3c5: 8233798: Ctrl-L character mistakenly removed from gstreamer.md
  • e0b45f8: 8236648: javadoc warning on Text::tabSizeProperty method
  • 587f195: 8234474: [macos 10.15] Crash in file dialog in sandbox mode
  • f1108b0: 8233747: JVM crash in com.sun.webkit.dom.DocumentImpl.createAttribute
  • 3c4d68d: 8236626: Update copyright header for files modified in 2019
  • 580a2a9: 8087980: Add property to disable Monocle cursor
  • 3bbcbfb: 8225571: Port DND source to use GTK instead of GDK
  • 1952606: 8232811: Dialog's preferred size no longer accommodates multi-line strings
  • 4c6ebfb: 8236484: Compile error in monocle dispman
  • 8367e1a: 8130738: Add tabSize property to Text and TextFlow
  • 69e4ef3: 8235627: Blank stages when running JavaFX app in a macOS virtual machine
  • 5e0fb91: 8235364: Update copyright header for files modified in 2019

Your commit was automatically rebased without conflicts.

Pushed as commit b81cf32.

@mlbridge
Copy link

mlbridge bot commented Mar 17, 2020

Mailing list message from Ambarish Rapte on openjfx-dev:

Changeset: b81cf32
Author: Florian Kirmaier <fkirmaier at openjdk.org>
Committer: Ambarish Rapte <arapte at openjdk.org>
Date: 2020-03-17 13:44:41 +0000
URL: https://git.openjdk.java.net/jfx/commit/b81cf32b

8236259: MemoryLeak in ProgressIndicator

Reviewed-by: kcr, arapte

! modules/javafx.controls/src/main/java/javafx/scene/control/skin/ProgressIndicatorSkin.java
+ tests/system/src/test/java/test/javafx/scene/control/ProgressIndicatorLeakTest.java

@jskov
Copy link
Contributor

jskov commented May 1, 2020

@FlorianKirmaier did you want to persue a common shared in-tree GC testing facility?

Otherwise I would like to give it a shot (I have also just added another copy of the attemptGC method to a test).

@FlorianKirmaier
Copy link
Author

FlorianKirmaier commented May 3, 2020

@jskov
Thank you for the reminder.
I've updated and improved my JMemoryBuddy, and now I'm working on a PR for JavaFX add it based on the feedback of this PR and simplifies various tests.

@FlorianKirmaier FlorianKirmaier mentioned this pull request May 3, 2020
2 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.