8303680 Virtual Flow freezes after calling scrollTo and scrollPixels in succession #1052

FlorianKirmaier · 2023-03-06T16:04:02Z

Possible fix for VirtualFlow freeze.

I encountered the problem when experimenting with VirtualFlow.

Guess @johanvos should take a look.
All tests are still green, so with some luck, this doesn't break anything but fixes some known and unknown bugs.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8303680: Virtual Flow freezes after calling scrollTo and scrollPixels in succession

Reviewers

Andy Goryachev (@andy-goryachev-oracle - Committer)
Johan Vos (@johanvos - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx pull/1052/head:pull/1052
$ git checkout pull/1052

Update a local copy of the PR:
$ git checkout pull/1052
$ git pull https://git.openjdk.org/jfx pull/1052/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1052

View PR using the GUI difftool:
$ git pr show -t 1052

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1052.diff

Possible fix for VirtualFlow freeze

updated test name

bridgekeeper · 2023-03-06T16:05: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. There are additional pull request commands available for use with this pull request.

mlbridge · 2023-03-06T16:10:37Z

Webrevs

00: Full (28bdb0bd)

kevinrushforth · 2023-03-06T16:12:37Z

You will need to modify the title of this PR to match the JBS bug title (which I slightly modified to change "crashed" to "freezes" to better reflect the problem).

/reviewers 2

openjdk · 2023-03-06T16:13:59Z

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

andy-goryachev-oracle · 2023-03-06T16:39:22Z

modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java

+
+        // scroll to 50 and scroll 1 pixel
+        vf.scrollTo(50);
+        vf.scrollPixels(1);


this is a rather artificial test.
is it possible to reproduce the condition using control's public APIs?

This is the public API of VirtualFlow. I don't know whether it's possible with the API of the Control - But the API of the VirtualFlow should be stable too, right?

You are right, it is public API. One can't get directly to it, but it is possible to subclass the ListView and use the protected getVirtualFlow() method.

We have many tests that access VirtualFlow, so that seems ok.

would it be possible to update the JBS ticket with a valid code example?

That would be very helpful.

Oh, I didn't notice that I did upload the wrong file.
I've now uploaded the correct test application!

kevinrushforth

I'll let others do the formal review of this, but I would like to see an evaluation of what the root cause is, why this is the right fix, and whether there are any side effects. As it is, all I see is a removed line with no explanation of the problem or the solution.

kevinrushforth · 2023-03-06T21:01:11Z

modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java

+
+        // scroll to 50 and scroll 1 pixel
+        vf.scrollTo(50);
+        vf.scrollPixels(1);


We have many tests that access VirtualFlow, so that seems ok.

kevinrushforth · 2023-03-06T21:02:43Z

modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java

@@ -1715,7 +1715,6 @@ public double scrollPixels(final double delta) {

        // Finally, update the scroll bars
        updateScrollBarsAndCells(false);
-        lastPosition = getPosition();


This is not a formal review -- I'll let Andy and Johan do that -- but can you explain why this is the right fix? Presumably there was at least some reasoning behind updating lastPosition. Are there any side effects of not doing that?

I've written more about the explanation below. Actually, it's a good question whether there is reasoning behind updating lastPosition. Wouldn't be surprised, if it is just an artifact of some bug-fixing attempt.

I'll review this.
One of the challenges with the VirtualFlow class is that there are a number of parameters that contain some state, and the behavior depends very often on what parameters are modified in which method, and when (in which order).

FlorianKirmaier · 2023-03-07T09:13:59Z

Some explanation:

The layout method contains logic, to skip the layouting.

            if (width == lastWidth &&
                height == lastHeight &&
                cellCount == lastCellCount &&
                isVertical == lastVertical &&
                position == lastPosition &&
                ! cellSizeChanged)
            {
                // TODO this happens to work around the problem tested by
                // testCellLayout_LayoutWithoutChangingThingsUsesCellsInSameOrderAsBefore
                // but isn't a proper solution. Really what we need to do is, when
                // laying out cells, we need to make sure that if a cell is pressed
                // AND we are doing a full rebuild then we need to make sure we
                // use that cell in the same physical location as before so that
                // it gets the mouse release event.
                return;
            }

So, when the position is changed, then the content should be layout again.
But when we reset the oldPosition, to the current position, somewhere else in the code - then we interfere with this logic.
This has the effect, that the layout is skipped.
For that reason, it seems wrong to me, to change the "last-something" values outside of this method.

So I removed the resetting of the lastPosition in the scrollPixels method.
lastPosition = getPosition();
I also don't see a reason, why the lastPosition should be set anyways.

But I have to admit, that I don't understand the class well enough to ensure this is correct based on logic. This fixes the unit test and doesn't seem to break tests or something obvious in applications according to my tests.

ebresie · 2023-03-11T14:43:04Z

Some explanation:

The layout method contains logic, to skip the layouting.

So, when the position is changed, then the content should be layout again. But when we reset the oldPosition, to the current position, somewhere else in the code - then we interfere with this logic. This has the effect, that the layout is skipped. For that reason, it seems wrong to me, to change the "last-something" values outside of this method.

So I removed the resetting of the lastPosition in the scrollPixels method. lastPosition = getPosition(); I also don't see a reason, why the lastPosition should be set anyways.

But I have to admit, that I don't understand the class well enough to ensure this is correct based on logic. This fixes the unit test and doesn't seem to break tests or something obvious in applications according to my tests.

Just curious…based on the explanation above about change/not changes, does the call to getPosition help in the case state may (or may not) have changed to ensure there is no stale state at that time?

andy-goryachev-oracle

Tested this fix with the MonkeyTester app on Macos Ventura 13.1
https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/monkey/MonkeyTesterApp.java

Could not see any ill effects - navigation works, selection works, variable height cells work.

johanvos · 2023-03-16T13:25:53Z

The patch looks ok, and the test fails before and passes after. No regression is observed. Also, the explanation about the changed position outside the layout process makes sense.
The only hesitation I still have is why the lastPosition = getPosition(); is there in the first place.
As I wrote before, there are many implicit rules in this class, e.g. only change layout-related properties inside the layout cycle ; but there are methods that are called from within the layoutChildren method as well as from methods that are triggered by different inputs.

I'm approving this, as the removal of this line is in line with the general approach here where we don't change the lastXXX properties outside the layout process -- as Florian mentioned as well.

openjdk · 2023-03-16T13:28:14Z

@FlorianKirmaier This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8303680: Virtual Flow freezes after calling scrollTo and scrollPixels in succession

Reviewed-by: angorya, jvos

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 7 new commits pushed to the master branch:

935c7b7: 8301009: Update libxml2 to 2.10.3
147d71f: 8178368: Right alignment of text fields and alignment of prompt text works incorrectly
4051f16: 8299968: Second call to Stage.setScene() create sizing issue with uiScale > 1.0
df22c41: 8302355: Public API for Toolkit.canStartNestedEventLoop()
8cda7bc: 8302797: ArrayIndexOutOfBoundsException in TextRun.getWrapIndex()
4178ad7: 8090123: Items are no longer visible when collection is changed
bbdc599: 8303217: Webview loaded webpage is not showing play, volume related buttons for embeded Audio/Video elements

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

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, @andy-goryachev-oracle, @johanvos) 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 · 2023-03-16T18:30:58Z

/integrate
Thank you for the quick review!

FlorianKirmaier · 2023-03-16T18:31:32Z

/integrate

openjdk · 2023-03-16T18:32:10Z

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

andy-goryachev-oracle · 2023-03-16T18:32:54Z

/sponsor

openjdk · 2023-03-16T18:33:48Z

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

openjdk · 2023-03-16T18:34:42Z

Going to push as commit 6be8703.
Since your change was applied there have been 7 commits pushed to the master branch:

935c7b7: 8301009: Update libxml2 to 2.10.3
147d71f: 8178368: Right alignment of text fields and alignment of prompt text works incorrectly
4051f16: 8299968: Second call to Stage.setScene() create sizing issue with uiScale > 1.0
df22c41: 8302355: Public API for Toolkit.canStartNestedEventLoop()
8cda7bc: 8302797: ArrayIndexOutOfBoundsException in TextRun.getWrapIndex()
4178ad7: 8090123: Items are no longer visible when collection is changed
bbdc599: 8303217: Webview loaded webpage is not showing play, volume related buttons for embeded Audio/Video elements

Your commit was automatically rebased without conflicts.

openjdk · 2023-03-16T18:34:57Z

@andy-goryachev-oracle @FlorianKirmaier Pushed as commit 6be8703.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

FlorianKirmaier added 2 commits March 6, 2023 16:42

JDK-8303680

8c07822

Possible fix for VirtualFlow freeze

JDK-8303680

28bdb0b

updated test name

openjdk bot added the rfr Ready for review label Mar 6, 2023

FlorianKirmaier changed the title ~~JDK-8303680 VirtualFlow freeze~~ 8303680 VirtualFlow freeze Mar 6, 2023

kevinrushforth requested a review from johanvos March 6, 2023 16:12

FlorianKirmaier changed the title ~~8303680 VirtualFlow freeze~~ 8303680 Virtual Flow freezes after calling scrollTo and scrollPixels in succession Mar 6, 2023

andy-goryachev-oracle reviewed Mar 6, 2023

View reviewed changes

kevinrushforth reviewed Mar 6, 2023

View reviewed changes

andy-goryachev-oracle approved these changes Mar 15, 2023

View reviewed changes

johanvos approved these changes Mar 16, 2023

View reviewed changes

openjdk bot added the ready Ready to be integrated label Mar 16, 2023

openjdk bot added the sponsor Ready to sponsor label Mar 16, 2023

openjdk bot added the integrated Pull request has been integrated label Mar 16, 2023

openjdk bot closed this Mar 16, 2023

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Mar 16, 2023

openjdk bot removed the sponsor Ready to sponsor label Mar 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8303680 Virtual Flow freezes after calling scrollTo and scrollPixels in succession #1052

8303680 Virtual Flow freezes after calling scrollTo and scrollPixels in succession #1052

FlorianKirmaier commented Mar 6, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Mar 6, 2023

mlbridge bot commented Mar 6, 2023

kevinrushforth commented Mar 6, 2023

openjdk bot commented Mar 6, 2023

andy-goryachev-oracle Mar 6, 2023

FlorianKirmaier Mar 6, 2023

andy-goryachev-oracle Mar 6, 2023

kevinrushforth Mar 6, 2023

andy-goryachev-oracle Mar 6, 2023

kevinrushforth Mar 7, 2023

FlorianKirmaier Mar 7, 2023

kevinrushforth left a comment

kevinrushforth Mar 6, 2023

kevinrushforth Mar 6, 2023

FlorianKirmaier Mar 7, 2023

johanvos Mar 9, 2023

FlorianKirmaier commented Mar 7, 2023 •

edited

Loading

ebresie commented Mar 11, 2023

andy-goryachev-oracle left a comment

johanvos commented Mar 16, 2023

openjdk bot commented Mar 16, 2023

FlorianKirmaier commented Mar 16, 2023

FlorianKirmaier commented Mar 16, 2023

openjdk bot commented Mar 16, 2023

andy-goryachev-oracle commented Mar 16, 2023

openjdk bot commented Mar 16, 2023

openjdk bot commented Mar 16, 2023

openjdk bot commented Mar 16, 2023

8303680 Virtual Flow freezes after calling scrollTo and scrollPixels in succession #1052

8303680 Virtual Flow freezes after calling scrollTo and scrollPixels in succession #1052

Conversation

FlorianKirmaier commented Mar 6, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Mar 6, 2023

mlbridge bot commented Mar 6, 2023

Webrevs

kevinrushforth commented Mar 6, 2023

openjdk bot commented Mar 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinrushforth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FlorianKirmaier commented Mar 7, 2023 • edited Loading

ebresie commented Mar 11, 2023

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

johanvos commented Mar 16, 2023

openjdk bot commented Mar 16, 2023

FlorianKirmaier commented Mar 16, 2023

FlorianKirmaier commented Mar 16, 2023

openjdk bot commented Mar 16, 2023

andy-goryachev-oracle commented Mar 16, 2023

openjdk bot commented Mar 16, 2023

openjdk bot commented Mar 16, 2023

openjdk bot commented Mar 16, 2023

FlorianKirmaier commented Mar 6, 2023 •

edited by openjdk bot

Loading

FlorianKirmaier commented Mar 7, 2023 •

edited

Loading