8241840: Memoryleak: Closed focused Stages are not collected with Monocle. #153

FlorianKirmaier · 2020-03-30T13:37:51Z

Closed focused Stages are not collected with Monocle

This commit adds a unit-test and a fix for collecting focused closed stages.

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

Progress

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

Issue

JDK-8241840: Memoryleak: Closed focused Stages are not collected with Monocle.

Reviewers

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

Download

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

Closed focused Stages are not collected with Monocle This commit adds a unit-test and a fix for collecting focused closed stages.

bridgekeeper · 2020-03-30T13:38:16Z

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

mlbridge · 2020-03-30T13:47:11Z

Webrevs

kevinrushforth · 2020-03-30T14:02:13Z

/reviewers 2

openjdk · 2020-03-30T14:02:30Z

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

kevinrushforth · 2020-03-30T14:02:35Z

@arapte can you also review?

arapte

Suggested some changes and query. I still need to verify the fix in detail.

tests/system/src/test/java/test/javafx/stage/FocusedWindowTest.java

modules/javafx.graphics/src/main/java/com/sun/glass/ui/Window.java

tests/system/src/test/java/test/javafx/stage/FocusedWindowTest.java

Some code cleanups

tests/system/src/test/java/test/javafx/stage/FocusedWindowTest.java

modules/javafx.graphics/src/main/java/com/sun/glass/ui/monocle/MonocleWindowManager.java

tests/system/src/test/java/test/javafx/stage/FocusedWindowTest.java

small cleanup based on code review

removed change to the window class, because it wasn't required for the fix.

The tests are now reused for native and monocle tests.

FlorianKirmaier · 2020-04-22T12:00:03Z

I don't think the class MeshManagerCacheLeakTest is a good base to write monocle + native tests.
It required input/output parsing which would be a bit too much.

In my latest commit, I've added a simple solution to how the test can be reused.
On my side, both tests are always green, but I'm using a mac.

If one of the tests is unstable on windows, then it would be great if we could consider it as 2 bugs, so this PR can be finished.

Afterwards it would be great if someone else could continue the windows-bug because I don't have a very productive setup to work on it and I also don't really understand the native windows code.

arapte · 2020-04-27T15:09:17Z

modules/javafx.graphics/src/main/java/com/sun/glass/ui/Window.java

        if (this.isFocused != focused) {
            this.isFocused = focused;
-            if (this.isFocused) {
+            if (this.isFocused && this.isVisible) {


On my Window10 machine, with this change, Window.focusedWindow remains null even after the first window (I have not verified with multiple windows though) is shown onto the screen and is focused. And It continues to remain null until some mouse or key action is performed on the window.
I am not sure if this causes any side effects. It looks like the Window.focusedWindow is mostly(only) used for Monocle.
Can you please confirm the behavior that Window.focusedWindow remain null and check for any side effects.

As mentioned - I don't have a good setup to test this code on Windows.

But I've checked where focusedWindow/getFocusedWindow is used, and I can verify your assumption. I've searched through the whole project and the variable is only used in the MonocleCode.

The fact that focusedWindow get's sometimes set is probably the cause of the irregular happening memoryleak on Window.

In reading the comments, I thought you were going to revert the changes to Window.java? Or did I misinterpret what you said earlier?

I can confirm that focusedWindow is no longer correctly set to the focused window when that Window is first shown (I tried this on Windows). This is true for apps with multiple windows as well as single Stage apps. I can also confirm that focusedWindow isn't used on any platform other than Monocle. Even so, since this change isn't needed it seems best to revert it.

The motivation for this change was, that I've seen a similar bug on native-windows without monocle.
In this case a leak happened related to the focusedWindow variable.
Sadly I don't have a test for this bug, because it is undeterministic.
For that reason, I thought it would make sense to keep this change, because I think it fixes it.
If I remember correctly, it was related to Stages which were focused but closed at the same time, which only weren't collected because of the focusedWindow variable.
Because the variable also isn't used on NativeWindows it doesn't seem to do much harm.

I've now removed it, but it might be considered to be readded.

FlorianKirmaier · 2020-07-21T12:33:28Z

Any updates about this PR?
I run into this bug basically every time I check something for a memory leak, which is quite annoying.
Edit: The rerequest review button seems to be not working

kevinrushforth · 2020-07-21T20:21:53Z

Once the last of the jfx15 reviews and other critical reviews are out of the way, we'll get back to this for JavaFX 16.

kevinrushforth

The fix to Monocle looks good. So does the test (I left some minor comments). I can confirm that the test catches the leak (it passes with your fix and fails without it).

My only question is regarding the change to Window.java.

kevinrushforth · 2020-08-07T20:39:56Z

modules/javafx.graphics/src/main/java/com/sun/glass/ui/Window.java

        if (this.isFocused != focused) {
            this.isFocused = focused;
-            if (this.isFocused) {
+            if (this.isFocused && this.isVisible) {


In reading the comments, I thought you were going to revert the changes to Window.java? Or did I misinterpret what you said earlier?

I can confirm that focusedWindow is no longer correctly set to the focused window when that Window is first shown (I tried this on Windows). This is true for apps with multiple windows as well as single Stage apps. I can also confirm that focusedWindow isn't used on any platform other than Monocle. Even so, since this change isn't needed it seems best to revert it.

tests/system/src/test/java/test/javafx/stage/FocusedWindowMonocleTest.java

tests/system/src/test/java/test/javafx/stage/FocusedWindowNativeTest.java

tests/system/src/test/java/test/javafx/stage/FocusedWindowTestBase.java

Minor cleanups

Removed unnecessary change.

fixed tests

kevinrushforth

The fix looks good now. I'll file a follow-up issue for the potential leak on the other platforms.

My thinking is that if focusedWindow is only used in Monocle, we should push the tracking down to the platform rather than tracking it in the base class. Alternatively, we could have a platform-specific attribute that indicates whether to track (and leave it null otherwise).

tests/system/src/test/java/test/javafx/stage/FocusedWindowMonocleTest.java

tests/system/src/test/java/test/javafx/stage/FocusedWindowNativeTest.java

tests/system/src/test/java/test/javafx/stage/FocusedWindowTestBase.java

kevinrushforth · 2020-08-13T19:49:45Z

I filed JDK-8251555 to track the follow-on issue.

small formatting changes based on codereview

FlorianKirmaier · 2020-08-14T10:05:09Z

Great, I've added the changes and commented on the now ticket.

openjdk · 2020-08-14T13:01:28Z

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

8241840: Memoryleak: Closed focused Stages are not collected with Monocle.

Reviewed-by: arapte, kcr

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 /issue command.

Since the source branch of this PR was last updated there have been 140 commits pushed to the master branch:

f50d218: Merge
208d828: 8228570: Add various documentation clarifications
7a8708b: 8250799: NumberStringConverter and its subclasses are missing documentation for all their constructors
aefed81: 8249777: build.gradle: project.version should not contain time stamps
f216c5f: 8181775: JavaFX WebView does not calculate border-radius properly
5c596b1: 8196079: Remove obsolete Pisces rasterizer
af000b2: Merge
5d34d72: 8245053: Keyboard doesn't show when TextInputControl has focus
487854c: 8246343: Fix mistakes in FX API docs
fc38ce6: 8249647: Many classes in package javafx.beans.binding in module javafx.base have implicit no-arg constructors
... and 130 more: https://git.openjdk.java.net/jfx/compare/d7f13f457ebbf8608dc5169737b34b8cc9c9d242...master

As 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 f50d2188f6853263b00d3fb1cbd9f7db24314b2e.

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 (@arapte, @kevinrushforth) 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).

FlorianKirmaier · 2020-08-14T17:19:28Z

/integrate

openjdk · 2020-08-14T17:20:15Z

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

kevinrushforth · 2020-08-14T17:42:38Z

/sponsor

openjdk · 2020-08-14T17:43:35Z

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

f50d218: Merge
208d828: 8228570: Add various documentation clarifications
7a8708b: 8250799: NumberStringConverter and its subclasses are missing documentation for all their constructors
aefed81: 8249777: build.gradle: project.version should not contain time stamps
f216c5f: 8181775: JavaFX WebView does not calculate border-radius properly
5c596b1: 8196079: Remove obsolete Pisces rasterizer
af000b2: Merge
5d34d72: 8245053: Keyboard doesn't show when TextInputControl has focus
487854c: 8246343: Fix mistakes in FX API docs
fc38ce6: 8249647: Many classes in package javafx.beans.binding in module javafx.base have implicit no-arg constructors
a46b250: Merge
926b5b6: 8249839: Cherry pick GTK WebKit 2.28.3 changes
6b00892: 8250238: Media fails to load libav 58 library when using modules from maven central
aae8b6b: Merge
3cc29e3: 8220484: JFXPanel renders a slanted image with a hidpi monitor scale of 125% or 175%
5f60ea5: 8248381: Create a daemon thread for MonocleTimer
9260fd1: Merge
f056d24: 8248908: Printer.createPageLayout() returns 0.75" margins instead of hardware margins
5e7e452: 8245284: Update to 610.1 version of WebKit
2f4666a: 8248490: [macOS] Undecorated stage does not minimize
281f5c2: Merge
5de99be: Merge
946590e: 8248365: Debug build crashes on Windows when playing media file
e2d1c02: 8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException
82f66ca: Merge
d67c47f: 8201567: QuantumRenderer modifies buffer in use by JavaFX Application Thread
cd57bb5: Merge
a4f3150: 8248317: Change JavaFX release version to 16
126637f: 8201570: Get two bytes for the Linux input event type, not four
f3a0446: 8247963: Update SQLite to version 3.32.3
32584db: 8238954: Improve performance of tiled snapshot rendering
869ea40: 8244212: Optionally download media and webkit libraries from latest openjfx EA build
62f8cee: 8247947: Build DirectShow Samples (Base Classes) from source checked into repo
527cc2b: 8248551: [TestBug] Ignore two failing FXML unit tests which use Nashorn script engine
45c9854: 8238080: FXMLLoader: if script engines implement javax.script.Compilable compile scripts
15f97ed: 8240264: iOS: Unnecessary logging on every pulse when GL context changes
2ca509a: 8193800: TreeTableView selection changes on sorting
a5878e0: 8214699: Node.getPseudoClassStates must return the same instance on every call
8440b64: 8208169: can not print selected pages of web page
1727dfa: 8247163: JFXPanel throws exception on click when no Scene is set
54e2507: 8247360: Add missing license file for Microsoft DirectShow Samples
fb962ac: 8244418: MenuBar: IOOB exception on requestFocus on empty bar
bf2e972: 8246348: Crash in libpango on Ubuntu 20.04 with some unicode chars
b200891: 8244824: TableView : Incorrect German translation
f6ec5f1: Merge
b2b46eb: 8242892: SpinnerValueFactory has an implicit no-arg constructor
afa805f: 8245575: Show the ContextMenu of input controls with long press gesture on iOS
5304266: 8245635: GlassPasteboard::getUTFs fails on iOS
ba501ef: 8246357: Allow static build of webkit library on linux
a02e09d: 8246195: ListViewSkin/Behavior: misbehavior on switching skin
9749982: 8246204: No 3D support for newer Intel graphics drivers on Linux
6bd0e22: 8239095: Upgrade libFFI to the latest 3.3 version
853ac78: 8245282: Button/Combo Behavior: memory leak on dispose
a78b3fb: 8242523: Update the animation and clip envelope classes
1ab653c: 8244657: ChoiceBox/ToolBarSkin: misbehavior on switching skin
804ccce: 8244195: [TEST_BUG] Convert the system tests TabPanePermuteGetTabsTest to unit test
1663624: Merge
13f42e1: Merge
9edba9c: 8243110: SVGTest.testSVGRenderingWithPattern fails intermittently
168b7f7: 8246099: Intermittent test failures in SandboxAppTest
78bf7b7: 8245422: Better Pisces rasterizing
c41777e: 8245634: [TestBug] Enable and fix tests ignored with message "impl_cssSet API removed"
3ceee69: 8245499: Text input controls should show handles on iOS
8914bd2: 8234540: javafx.web LeakTest.testGarbageCollectability fails intermittently
16f446a: 8234876: Unit test classes should not extend Application
2d98fe6: 8245601: TESTBUG] Disable TabPaneDragPolicyTest on Mac until JDK-8213136 is fixed and fix ISE
f3190db: 8244531: Tests: add support to identify recurring issues with controls et al
1971c70: 8245457: TestBug] Enable and fix ignored tests in ButtonBaseTest & ButtonTest
6e0b45a: 8245183: Two fxml unit tests log warnings about deprecated escape sequences
a13a642: 8244579: Windows "User Objects" leakage with WebView
37b5edc: 8245456: MacPasteboard throws ClassCastException on static builds
6e03930: 8237602: TabPane doesn't respect order of TabPane.getTabs() list
f8cc090: Merge
bb24322: 8244112: Skin implementations: must not violate contract of dispose
dbb6437: 8244647: Wrong first layout pass of Scrollbar controls on touch supported devices
7b06190: 8242548: Wrapped labeled controls using -fx-line-spacing cut text off
435671e: 8202296: Monocle MouseInput doesn't send keyboard modifiers in events.
c14cc44: 8244417: support static build for Windows
b14e085: 8244735: Error on iOS passing keys with unicode values greater than 255
b0d66d0: 8242508: Upgrade to Visual Studio 2019 version 16.5.3
0f87d20: 8244487: One Windows 10 SDK file missing from FX build
4597437: Merge
4ec163d: 8242001: ChoiceBox: must update value on setting SelectionModel, part2
236e2d6: 8244421: Wrong scrollbar position on touch enabled devices
0385563: 8243255: Font size is large in JavaFX app with enabled Monocle on Raspberry Pi
2e90076: 8242507: Add support for Visual Studio 2019
39d9c3b: 8244110: NPE in MenuButtonSkinBase change listener
99f7747: 8241999: ChoiceBox: incorrect toggle selected for uncontained
1cae6a8: 8176499: Dependence on java.util.Timer freezes screen when OS time resets backwards
3130fc8: 8198402: ToggleButton.setToggleGroup causes memory leak when button is removed via ToggleGroup.getToggles()
8ad5805: 8191758: Match WebKit's font weight rendering with JavaFX
66c3b38: 8227425: Add support for e-paper displays on i.MX6 devices
e30049f: 8242077: Add information about HTTP/2 and HttpClient usage in WebEngine
e0ffca3: 8242505: Some WebKit tests might fail because Microsoft libraries are not loaded
ceb3fce: 8087555: [ChoiceBox] uncontained value not shown
818ac00: 8175358: Memory leak when moving MenuButton into another Scene
f1ac39b: Merge
91d4c8b: 8241737: TabPaneSkin memory leak on replacing selectionModel
48476eb: 8241582: Infinite animation does not start from the end when started with a negative rate
dedf7cb: 8242490: Upgrade to gcc 9.2 on Linux
5e9fb82: 8242577: Cell selection fails on iOS most of the times
69e4266: 8242489: ChoiceBox: initially toggle not sync'ed to selection
1d88180: 8243112: Skip failing test SVGTest.testSVGRenderingWithPattern
ec8608f: 8223298: SVG patterns are drawn wrong
e82046e: 8242530: [macos] Some audio files miss spectrum data when another audio file plays first
7044cef: 8241476: Linux build warnings issued on gcc 9
bb5ce2c: Merge
9d50c4c: Merge
4d69a0d: 8241629: [macos10.15] Long startup delay playing media over https on Catalina
b1fdc45: 8242209: Increase web native thread stack size for x86 mode
e1cb191: 8240694: [macos 10.15] JavaFX Media hangs on some video files on Catalina
c154538: 8242106: [macos] Remove obsolete GlassView2D.m class
3f663e3: 8240262: iOS refresh rate is capped to 30 Hz
231879a: 8241710: NullPointerException while entering empty submenu with "arrow right"
470c7d0: 8230809: HTMLEditor formatting lost when selecting all (CTRL-A)
fda015c: 8242167: ios keyboard handling
844460b: 8242163: Android keyboard integration fails
364c64a: 8241249: NPE in TabPaneSkin.perfromDrag
418675a: 8236840: Memory leak when switching ButtonSkin
247a65d: 8236971: [macos] Gestures handled incorrectly due to missing events
560ef17: 8241455: Memory leak on replacing selection/focusModel
d299465: Merge
ef37669: Merge
5906521: 8241370: Crash in JPEGImageLoader after fix for JDK-8212034
159f651: 8240542: Switch FX build to use JDK 14 as boot JDK
6d098fe: 8234959: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV
d2346b8: Merge
f2bca9f: Merge
397587d: 8241108: Glib improvements
6900d29: Merge
e91bec4: Merge
66a8f49: Merge
fde42da: Merge
e21fd1f: Merge
443c845: Merge
31e63de: Merge
14c6938: 8236798: Enhance FX scripting support
bfb2d0e: Merge
39f6127: Merge

Your commit was automatically rebased without conflicts.

Pushed as commit 1c54e61.

FlorianKirmaier added 3 commits March 30, 2020 15:18

JDK-8241840

26660df

Closed focused Stages are not collected with Monocle This commit adds a unit-test and a fix for collecting focused closed stages.

cleanup

7f535de

fixing accidentally commited test

991d3ff

FlorianKirmaier changed the title ~~JDK-8241840 Memoryleak: Closed focused Stages are not collected with Monocle.~~ JDK-8241840: Memoryleak: Closed focused Stages are not collected with Monocle. Mar 30, 2020

FlorianKirmaier changed the title ~~JDK-8241840: Memoryleak: Closed focused Stages are not collected with Monocle.~~ 8241840: Memoryleak: Closed focused Stages are not collected with Monocle. Mar 30, 2020

openjdk bot added the rfr Ready for review label Mar 30, 2020

kevinrushforth self-requested a review March 30, 2020 14:02

arapte suggested changes Apr 7, 2020

View reviewed changes

JDK-8241840

20865db

Some code cleanups

arapte suggested changes Apr 17, 2020

View reviewed changes

FlorianKirmaier added 3 commits April 22, 2020 12:57

JDK-8241840

5992550

small cleanup based on code review

JDK-8241840

d5cb902

removed change to the window class, because it wasn't required for the fix.

JDK-8241840

3f618c1

The tests are now reused for native and monocle tests.

arapte reviewed Apr 27, 2020

View reviewed changes

FlorianKirmaier mentioned this pull request Jun 23, 2020

Possible memory leak: instances not released from memory after closing JPro-one/JPro#57

Closed

kevinrushforth reviewed Aug 7, 2020

View reviewed changes

FlorianKirmaier added 3 commits August 9, 2020 19:03

JDK-8241840

b4c0651

Minor cleanups

JDK-8241840

4cfe3ca

Removed unnecessary change.

JDK-8241840

a7c9d83

fixed tests

kevinrushforth reviewed Aug 13, 2020

View reviewed changes

JDK-8241840

2971a11

small formatting changes based on codereview

kevinrushforth approved these changes Aug 14, 2020

View reviewed changes

arapte approved these changes Aug 14, 2020

View reviewed changes

openjdk bot added the ready Ready to be integrated label Aug 14, 2020

openjdk bot added the sponsor Ready to sponsor label Aug 14, 2020

openjdk bot closed this Aug 14, 2020

openjdk bot added integrated Pull request has been integrated and removed sponsor Ready to sponsor ready Ready to be integrated rfr Ready for review labels Aug 14, 2020

jperedadnr mentioned this pull request Oct 12, 2020

Android: NPE at MonocleWindowManager when launching the app gluonhq/substrate#810

Closed

johanvos mentioned this pull request Oct 16, 2020

8254605: repaint on Android broken #322

Closed

3 tasks

8241840: Memoryleak: Closed focused Stages are not collected with Monocle. #153

8241840: Memoryleak: Closed focused Stages are not collected with Monocle. #153

Uh oh!

Conversation

FlorianKirmaier commented Mar 30, 2020 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Download

Uh oh!

bridgekeeper bot commented Mar 30, 2020

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

kevinrushforth commented Mar 30, 2020

Uh oh!

openjdk bot commented Mar 30, 2020

Uh oh!

kevinrushforth commented Mar 30, 2020

Uh oh!

arapte left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

FlorianKirmaier commented Apr 22, 2020

Uh oh!

arapte Apr 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FlorianKirmaier Apr 28, 2020

Choose a reason for hiding this comment

Uh oh!

kevinrushforth Aug 7, 2020

Choose a reason for hiding this comment

Uh oh!

FlorianKirmaier Aug 9, 2020

Choose a reason for hiding this comment

Uh oh!

FlorianKirmaier commented Jul 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kevinrushforth commented Jul 21, 2020

Uh oh!

kevinrushforth left a comment

Choose a reason for hiding this comment

Uh oh!

kevinrushforth Aug 7, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kevinrushforth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kevinrushforth commented Aug 13, 2020

Uh oh!

FlorianKirmaier commented Aug 14, 2020

Uh oh!

FlorianKirmaier commented Mar 30, 2020 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 30, 2020 •

edited

Loading

arapte Apr 27, 2020 •

edited

Loading

FlorianKirmaier commented Jul 21, 2020 •

edited

Loading

openjdk bot commented Aug 14, 2020 •

edited

Loading