Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

Snapshot grid structure info instead of modifying it in place #12646

Merged
merged 1 commit into from
Nov 11, 2020

Conversation

hartez
Copy link
Contributor

@hartez hartez commented Oct 28, 2020

Description of Change

Issue #6247, issue #10470, and https://forums.xamarin.com/discussion/88367/nullreferenceexception-on-grid-calculateautocells-only-xf-ios describe NullReferenceExceptions accessing the _rows and _columns members during Grid layouts. These members are lists of RowDefinition and ColumnDefinition information used during the measurement and layout of the Grid. The information in these lists is not for caching purposes; lists are reset during every measurement and layout operation, and also when computing constraints for the Grid.

Because the lists are nullable, certain layout and update conditions (which are very difficult to consistently reproduce) can result in the lists being null while a measurement operation is taking place, crashing the application.

In https://github.com/xamarin/Duplo/pull/3675, a similar issue during LayoutChildren was addressed by snapshotting the lists before handling layout, in case InvalidateMeasure was called during the layout process. These changes formalize that snapshotting by creating a separate class to handle creating the snapshot and using it in LayoutChildren, OnSizeRequest, and ComputeConstraintForView. The nullable lists of row/column definitions have been removed entirely, so a NullReferenceException is no longer an issue.

Issues Resolved

API Changes

None

Platforms Affected

  • Core/XAML (all platforms)

Behavioral/Visual Changes

None

Before/After Screenshots

Not applicable

Testing Procedure

No new tests

PR Checklist

  • Targets the correct branch
  • Tests are passing (or failures are unrelated)

@hartez
Copy link
Contributor Author

hartez commented Oct 29, 2020

The failed UI tests are on UWP, and both of them pass locally. I'm certain the test failures are unrelated to these changes.

@hartez hartez added i/high Completely doesn't work, crashes, or is unusably slow, has no obvious workaround; occurs less often m/high impact ⬛ a/grid a/layout labels Nov 2, 2020
@samhouts samhouts added Core 4.1.0 regression on 4.1.0 blocker Issue blocks next stable release. Prioritize fixing and reviewing this issue. t/bug 🐛 labels Nov 2, 2020
@mattiascibien
Copy link

I had the exact same problem. For our project this PR is actually what can fix the problems we are getting in 4.8 with this.

We backported this to 4.8 in a custom build but it seems to be generating other problems.

I think this PR should be a major priority for 5.0.

@rmarinho rmarinho added this to the 5.0.0 milestone Nov 5, 2020
@rmarinho rmarinho merged commit c6d03ed into 5.0.0 Nov 11, 2020
@rmarinho rmarinho deleted the grid-crash branch November 11, 2020 19:37
@PawKanarek
Copy link
Contributor

PawKanarek commented Nov 19, 2020

Nice <3 gz @hartez

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
4.1.0 regression on 4.1.0 a/grid a/layout blocker Issue blocks next stable release. Prioritize fixing and reviewing this issue. Core i/high Completely doesn't work, crashes, or is unusably slow, has no obvious workaround; occurs less often m/high impact ⬛ t/bug 🐛
Projects
None yet
6 participants