Re: RFR: 8279228 Leak in ScrollPaneSkin, related to touch events
On Thu, 23 Dec 2021 17:43:19 GMT, Florian Kirmaier wrote: > Fixing memoryleak, related to touch events in ScrollPaneWhen touchDetected or > mouseDown is true, the sbTouch animation is running, > and the node is removed from the Scene, then the animation will never stop, > causing a memory leak. > A simple fix is to also check, whether the Node is visible, by checking the > "isTreeShowing" property. Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.java.net/jfx/pull/701
Re: RFR: 8279228 Leak in ScrollPaneSkin, related to touch events
On Thu, 23 Dec 2021 17:43:19 GMT, Florian Kirmaier wrote: > Fixing memoryleak, related to touch events in ScrollPaneWhen touchDetected or > mouseDown is true, the sbTouch animation is running, > and the node is removed from the Scene, then the animation will never stop, > causing a memory leak. > A simple fix is to also check, whether the Node is visible, by checking the > "isTreeShowing" property. This simple fix looks good to me. - Marked as reviewed by aghaisas (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/701
Re: RFR: 8279228 Leak in ScrollPaneSkin, related to touch events
On Thu, 23 Dec 2021 17:43:19 GMT, Florian Kirmaier wrote: > Fixing memoryleak, related to touch events in ScrollPaneWhen touchDetected or > mouseDown is true, the sbTouch animation is running, > and the node is removed from the Scene, then the animation will never stop, > causing a memory leak. > A simple fix is to also check, whether the Node is visible, by checking the > "isTreeShowing" property. 1.) It's not only called at the beginning of the `startSBReleasedAnimation`, but whenever the animation is repeated, which is the cause for the leak. It happens when `touchDetected == true || mouseDown == true` is true. The animation is then basically restarted every frame. With my change, it also checks whether the node is still visible, which fixes the problem. 2.) It's probably possible if there is a way to simulate Touch Events. The Robot doesn't seem to support it. A test would also require a way to set the "IS_TOUCH_SUPPORTED" property. Honestly, I don't think it's a good time investment, because the fix is quite simple. I don't even know exactly when it happens - I've just made sure it no longer loops the animation when the ScrollPane is no longer visible. I've also applied the fix in an affected application, and it clearly fixes the problem. - PR: https://git.openjdk.java.net/jfx/pull/701
Re: RFR: 8279228 Leak in ScrollPaneSkin, related to touch events
On Thu, 23 Dec 2021 17:43:19 GMT, Florian Kirmaier wrote: > Fixing memoryleak, related to touch events in ScrollPaneWhen touchDetected or > mouseDown is true, the sbTouch animation is running, > and the node is removed from the Scene, then the animation will never stop, > causing a memory leak. > A simple fix is to also check, whether the Node is visible, by checking the > "isTreeShowing" property. Two quick questions: 1. The fix checks whether or not the node is treeShowing at the time the `startSBReleasedAnimation` method is called. Is this sufficient? If a node's tree showing state changes, is it guaranteed that this method will be called? 2. Can you provide an automated test for this? - PR: https://git.openjdk.java.net/jfx/pull/701
RFR: 8279228 Leak in ScrollPaneSkin, related to touch events
Fixing memoryleak, related to touch events in ScrollPaneWhen touchDetected or mouseDown is true, the sbTouch animation is running, and the node is removed from the Scene, then the animation will never stop, causing a memory leak. A simple fix is to also check, whether the Node is visible, by checking the "isTreeShowing" property. - Commit messages: - JDK-8279228 Changes: https://git.openjdk.java.net/jfx/pull/701/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx&pr=701&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8279228 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/701.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/701/head:pull/701 PR: https://git.openjdk.java.net/jfx/pull/701
