rect with negative height is not allowed (#5842) #6013
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.