Skip to content

Conversation

@singhashish-wpf
Copy link
Contributor

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.

@singhashish-wpf singhashish-wpf requested a review from a team as a code owner January 27, 2022 09:50
@ghost ghost added the PR metadata: Label to tag PRs, to facilitate with triage label Jan 27, 2022
@ghost ghost requested review from SamBent, fabiant3 and ryalanms January 27, 2022 09:50
@leecow leecow added this to the 6.0.3 milestone Feb 3, 2022
@dipeshmsft dipeshmsft merged commit 69c38e3 into dotnet:release/6.0 Feb 7, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Apr 5, 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 Servicing-approved

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants