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

8232824: Removing TabPane with strong referenced content causes memory leak from weak one #79

Closed
wants to merge 3 commits into from

Conversation

@arapte
Copy link

arapte commented Jan 7, 2020

This PR is a fix for JDK-8232824
This issue is regression of JDK-8187074.

Issue.

  • Parent.viewOrderChildren is a list of children sorted according to view order.

  • Parent.viewOrderChildren is cleared and computed in Parent.computeViewOrderChildren() which is called from Parent.doUpdatePeer() when a pulse is received.

  • Below is the scenario mentioned in this issue,

  1. TabPane is created with few tabs.
  2. For each tab, a TabHeaderSkin is created with setViewOrder(1) and is added to TabHeaderArea.headersRegion
  3. All these TabHeaderSkins are added to Parent.viewOrderChildren on pulse.
  4. When the TabPane is removed from scene, then on the next pulse the method Parent.doUpdatePeer() does not get called for TabHeaderArea.headersRegion, because it is not part for scenegraph anymore.
    So Parent.computeViewOrderChildren() does not get called and the Parent.viewOrderChildren does not get cleared, which causes the leak.

Fix:
Clear the Parent.viewOrderChildren list when marking DirtyBits.PARENT_CHILDREN_VIEW_ORDER as dirty.
Parent.viewOrderChildren will be computed on next pulse.
This will maintain lazy computation of Parent.viewOrderChildren.

Added a system test, fails without fix and passes with. No failures in existing tests.

Progress

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

Issue

JDK-8232824: Removing TabPane with strong referenced content causes memory leak from weak one

Approvers

  • Kevin Rushforth (kcr - Reviewer)
  • Ajit Ghaisas (aghaisas - Reviewer)
… memory leak from weak one
@bridgekeeper
Copy link

bridgekeeper bot commented Jan 7, 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 (refresh this page to view it).

@arapte arapte changed the title [WIP] 8232824: Removing TabPane with strong referenced content causes memory leak from weak one 8232824: Removing TabPane with strong referenced content causes memory leak from weak one Jan 7, 2020
@openjdk openjdk bot added the rfr label Jan 7, 2020
@mlbridge
Copy link

mlbridge bot commented Jan 7, 2020

Webrevs

@kevinrushforth kevinrushforth self-requested a review Jan 11, 2020
@kevinrushforth
Copy link
Member

kevinrushforth commented Jan 11, 2020

@arapte - This bug looks like a good candidate for JavaFX 14. Can you retarget this PR to the jfx14 branch?

@arapte arapte changed the base branch from master to jfx14 Jan 13, 2020
@kevinrushforth
Copy link
Member

kevinrushforth commented Jan 13, 2020

This will need a second reviewer.

@aghaisas can you review this, too?

@arapte
Copy link
Author

arapte commented Jan 13, 2020

@arapte - This bug looks like a good candidate for JavaFX 14. Can you retarget this PR to the jfx14 branch?

Thanks for guiding Kevin, PR is now targeted to jfx14.

@kevinrushforth
Copy link
Member

kevinrushforth commented Jan 17, 2020

The fix looks good. I'll take a closer look at the unit test later.

Speaking of tests...since the addition of the TabPane reordering logic was a victim of the already-existing leak in the viewOrderChildren list in Parent, it should be possible to write a test case using a Group node and a few Shape nodes, using setViewOrder directly on the Group node (this would be in addition to the system test you wrote). Can you take a look at adding one? It might even be possible to do it as a javafx.graphics module unit test rather than a system test, although you would need to see if the bug reproduced there (I suspect it will).

@arapte
Copy link
Author

arapte commented Jan 22, 2020

The fix looks good. I'll take a closer look at the unit test later.

Speaking of tests...since the addition of the TabPane reordering logic was a victim of the already-existing leak in the viewOrderChildren list in Parent, it should be possible to write a test case using a Group node and a few Shape nodes, using setViewOrder directly on the Group node (this would be in addition to the system test you wrote). Can you take a look at adding one? It might even be possible to do it as a javafx.graphics module unit test rather than a system test, although you would need to see if the bug reproduced there (I suspect it will).

Hello Kevin,
The bug can be reproduced with system test written using Group and Shape which is very similar to TabPaneHeaderLeakTest test. but it seems the bug is not reproducible with unit test. I tried a unit test very similar to the newly added system test ShapeViewOrderLeakTest, but looks like Parent.viewOrderChildren list does not get populated and so the issue does not occur.

@kevinrushforth
Copy link
Member

kevinrushforth commented Jan 22, 2020

The bug can be reproduced with system test written using Group and Shape which is very similar to TabPaneHeaderLeakTest test. but it seems the bug is not reproducible with unit test. I tried a unit test very similar to the newly added system test ShapeViewOrderLeakTest, but looks like Parent.viewOrderChildren list does not get populated and so the issue does not occur.

Did you run a pulse? That would be needed in order to sync the changes down to the peer. In any event, it is fine to use a system test if you can't get it to fail with a unit test.

I have three cleanup comments that apply to both of the new tests. The first is the most important of these.

  1. Test classes should not extend from javafx.application.Application. You should use a nested class that extends Application. See this comment on PR #34 for at least one reason why.

  2. The initFX method can be simplified using a pattern we've adopted in our newer tests. See QuadraticCssTimeTest.java

  3. Most tests run startupLatch::countDown in a Platform.runLater call.

Ambarish Rapte
@arapte
Copy link
Author

arapte commented Jan 23, 2020

Did you run a pulse? That would be needed in order to sync the changes down to the peer. In any event, it is fine to use a system test if you can't get it to fail with a unit test.

Yes Kevin, I had tried using Toolkit.getToolkit().firePulse();. But the test did not fail without fix.
I am not sure if I am missing anything with unit test, Ideally it should reproduce with unit test too.

Have updated the PR to fix the other review comments.

Copy link
Member

kevinrushforth left a comment

Looks good.

@openjdk openjdk bot removed the rfr label Jan 23, 2020
@openjdk
Copy link

openjdk bot commented Jan 23, 2020

@arapte 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:

8232824: Removing TabPane with strong referenced content causes memory leak from weak one

Reviewed-by: kcr, aghaisas
  • 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 40 commits pushed to the jfx14 branch. Since there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to do this manually, please merge jfx14 into your branch first.

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

@openjdk openjdk bot added the ready label Jan 23, 2020
@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).

@openjdk openjdk bot added rfr and removed ready labels Jan 24, 2020
@openjdk openjdk bot added the ready label Jan 28, 2020
@arapte
Copy link
Author

arapte commented Jan 28, 2020

/integrate

@openjdk openjdk bot closed this Jan 28, 2020
@openjdk openjdk bot added the integrated label Jan 28, 2020
@openjdk
Copy link

openjdk bot commented Jan 28, 2020

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

  • aa6f3a4: 8236912: NullPointerException when clicking in WebView with Button 4 or Button 5
  • da99e24: 8237823: Mark TextTest.testTabSize as unstable
  • 9ae37f1: 8236753: Animations do not play backwards after being stopped
  • 16cea41: Merge
  • 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
  • 63520a0: Merge
  • 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
  • 8a5344c: Merge
  • 935e99d: 8207957: TableSkinUtils should not contain actual code implementation
  • 4e005e4: 8227808: Make GTK3 libraries mandatory for building on Linux
  • 1140d34: 8196587: Remove use of deprecated finalize method from JPEGImageLoader
  • d2d44b4: 8207759: VK_ENTER not consumed by TextField when default Button exists
  • fc539b5: 8223296: NullPointerException in GlassScene.java at line 325
  • 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

Your commit was automatically rebased without conflicts.

Pushed as commit 79fc0d0.

@openjdk openjdk bot removed the ready label Jan 28, 2020
@mlbridge
Copy link

mlbridge bot commented Jan 28, 2020

Mailing list message from Ambarish Rapte on openjfx-dev:

Changeset: 79fc0d0
Author: Ambarish Rapte <arapte at openjdk.org>
Date: 2020-01-28 12:35:52 +0000
URL: https://git.openjdk.java.net/jfx/commit/79fc0d0d

8232824: Removing TabPane with strong referenced content causes memory leak from weak one

Reviewed-by: kcr, aghaisas

! modules/javafx.graphics/src/main/java/javafx/scene/Parent.java
+ tests/system/src/test/java/test/javafx/scene/control/TabPaneHeaderLeakTest.java
+ tests/system/src/test/java/test/javafx/scene/shape/ShapeViewOrderLeakTest.java

@arapte arapte deleted the arapte:TabPaneHeaderLeak branch Jan 28, 2020
@mlbridge
Copy link

mlbridge bot commented Jan 29, 2020

Mailing list message from Ambarish Rapte on openjfx-dev:

Changeset: 79fc0d0
Author: Ambarish Rapte <arapte at openjdk.org>
Date: 2020-01-28 12:35:52 +0000
URL: https://git.openjdk.java.net/jfx/commit/79fc0d0d

8232824: Removing TabPane with strong referenced content causes memory leak from weak one

Reviewed-by: kcr, aghaisas

! modules/javafx.graphics/src/main/java/javafx/scene/Parent.java
+ tests/system/src/test/java/test/javafx/scene/control/TabPaneHeaderLeakTest.java
+ tests/system/src/test/java/test/javafx/scene/shape/ShapeViewOrderLeakTest.java

@kevinrushforth kevinrushforth removed the rfr label Feb 19, 2020
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

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