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

arapte · 2020-01-07T09:46:03Z

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,

TabPane is created with few tabs.
For each tab, a TabHeaderSkin is created with setViewOrder(1) and is added to TabHeaderArea.headersRegion
All these TabHeaderSkins are added to Parent.viewOrderChildren on pulse.
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 · 2020-01-07T09:46:12Z

👋 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).

mlbridge · 2020-01-07T09:47:15Z

Webrevs

kevinrushforth · 2020-01-11T17:53:40Z

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

kevinrushforth · 2020-01-13T16:49:40Z

This will need a second reviewer.

@aghaisas can you review this, too?

arapte · 2020-01-13T16:49:41Z

@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 · 2020-01-17T23:40:18Z

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 · 2020-01-22T17:53:38Z

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 · 2020-01-22T23:58:29Z

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.

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.
The initFX method can be simplified using a pattern we've adopted in our newer tests. See QuadraticCssTimeTest.java
Most tests run startupLatch::countDown in a Platform.runLater call.

arapte · 2020-01-23T12:26:34Z

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.

kevinrushforth

Looks good.

openjdk · 2020-01-23T12:52:19Z

@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.

kevinrushforth · 2020-01-24T14:56:17Z

/reviewers 2

openjdk · 2020-01-24T14:56:30Z

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

arapte · 2020-01-28T12:31:38Z

/integrate

openjdk · 2020-01-28T12:36:06Z

@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.

mlbridge · 2020-01-28T12:37:34Z

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

mlbridge · 2020-01-29T13:03:44Z

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

[WIP] 8232824: Removing TabPane with strong referenced content causes…

cbcc316

… memory leak from weak one

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 bot added the rfr Ready for review label Jan 7, 2020

kevinrushforth self-requested a review January 11, 2020 17:53

arapte changed the base branch from master to jfx14 January 13, 2020 16:45

Adding system test using Group and Shape

d64f6c8

Fixed review comments

7c20469

kevinrushforth approved these changes Jan 23, 2020

View reviewed changes

openjdk bot removed the rfr Ready for review label Jan 23, 2020

openjdk bot added the ready Ready to be integrated label Jan 23, 2020

openjdk bot added rfr Ready for review and removed ready Ready to be integrated labels Jan 24, 2020

aghaisas approved these changes Jan 28, 2020

View reviewed changes

openjdk bot added the ready Ready to be integrated label Jan 28, 2020

openjdk bot closed this Jan 28, 2020

openjdk bot added the integrated Pull request has been integrated label Jan 28, 2020

openjdk bot removed the ready Ready to be integrated label Jan 28, 2020

arapte deleted the TabPaneHeaderLeak branch January 28, 2020 16:22

kevinrushforth removed the rfr Ready for review label Feb 19, 2020

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

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

Uh oh!

Conversation

arapte commented Jan 7, 2020 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Approvers

Uh oh!

bridgekeeper bot commented Jan 7, 2020

Uh oh!

mlbridge bot commented Jan 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

kevinrushforth commented Jan 11, 2020

Uh oh!

kevinrushforth commented Jan 13, 2020

Uh oh!

arapte commented Jan 13, 2020

Uh oh!

kevinrushforth commented Jan 17, 2020

Uh oh!

arapte commented Jan 22, 2020

Uh oh!

kevinrushforth commented Jan 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arapte commented Jan 23, 2020

Uh oh!

kevinrushforth left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Jan 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kevinrushforth commented Jan 24, 2020

Uh oh!

openjdk bot commented Jan 24, 2020

Uh oh!

arapte commented Jan 28, 2020

Uh oh!

openjdk bot commented Jan 28, 2020

Uh oh!

mlbridge bot commented Jan 28, 2020

Uh oh!

mlbridge bot commented Jan 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

arapte commented Jan 7, 2020 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 7, 2020 •

edited

Loading

kevinrushforth commented Jan 22, 2020 •

edited

Loading

openjdk bot commented Jan 23, 2020 •

edited

Loading