Skip to content

Conversation

@SamBent
Copy link
Contributor

@SamBent SamBent commented Dec 16, 2021

Addresses #2854
This is a port of a servicing fix in .NET 4.7-4.8.

Description

The crash arises when adding an item to the items-source collection underlying the ItemsControl, when the VSP is in an odd state: viewport.Y > extent.Height and <another relationship between viewport, extent, and last-computed "extended viewport", whose exact details I haven't yet determined>. The logic that determines whether such an addition requires a re-measurement predicts the height of the new extended viewport; in the odd state this sets the height to (extent.Height - viewport.Y), a negative number that's not allowed.

The first condition cannot happen during Measure, or immediately after; Measure starts by initializing viewport.Y to a value <= extent.Height. But it can occur after collection changes, as these can change extent.Height. Collection changes can also affect the quantities in the second condition. The first kind of change puts the height-assignment at risk; the second kind of change can allow the logic to reach the height-assignment. It's a Goldilocks situation - if the changes are too big or too small the logic doesn't need the height-assignment, which happens only when the changes are "just right". In fact, I've been unable to produce a fully-deterministic repro suitable for use in a test case; the "just-rightness" depends on the exact scrolling and virtualization history in a way that I've only seen in large datasets after non-deterministic scrolling (e.g. dragging the thumb when the data is so large that a single pixel of thumb motion corresponds to more than a screenful of data motion: extent.Height > viewport.Height^2 ).

The fix is to replace negative height by 0 in the height-assignment. This obviously avoids the crash, but it's not so obvious whether doing so can cause other problems later on. In the collection-change case, we only care whether the predicted extended viewport is taller than the last-computed one, we don't actually use the new height in any other way. So it's OK to use 0 instead of a negative number - they both have the same effect.

Customer Impact

Crash while adding/removing items from the underlying collection.

Regression

No.

Testing

Ad-hoc around customer scenario.
Standard regression testing.

Risk

Low. Port of a .NETFx servicing fix released earlier this year.

@SamBent SamBent requested a review from a team as a code owner December 16, 2021 02:34
@ghost ghost added the PR metadata: Label to tag PRs, to facilitate with triage label Dec 16, 2021
@ghost ghost requested review from fabiant3 and ryalanms December 16, 2021 02:34
@singhashish-wpf singhashish-wpf self-assigned this Dec 27, 2021
@singhashish-wpf singhashish-wpf merged commit 8343712 into dotnet:main Jan 19, 2022
dipeshmsft added a commit that referenced this pull request Feb 7, 2022
Co-authored-by: Sam Bent <sambent@microsoft.com>
dipeshmsft added a commit that referenced this pull request Feb 7, 2022
Co-authored-by: Sam Bent <sambent@microsoft.com>
dipeshmsft pushed a commit that referenced this pull request Feb 7, 2022
Co-authored-by: Sam Bent <sambent@microsoft.com>
@ghost ghost locked as resolved and limited conversation to collaborators Apr 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

PR metadata: Label to tag PRs, to facilitate with triage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants