Crash on NullReferenceException with measurement cells in CollectionView

filipnavara commented 3 weeks ago

Description

We are occasionally (but frequently enough) seeing this crash in NativeAOT build of our app:

Application Specific Information:
_preferredLayoutAttributesFittingAttributes:isAnimatingExistingView: >
Attempted to dereference null pointer.

Thread 0 Crashed:
0   MailClient.Mobile.iOS           0x2012fa9bc         RhpInterfaceDispatchAVLocation1
1   MailClient.Mobile.iOS           0x2021836dc         _Microsoft_Maui_Controls_Microsoft_Maui_Controls_Handlers_Items_HorizontalCell__Measure (HorizontalCell.cs:21)
2   MailClient.Mobile.iOS           0x20218bcd8         _Microsoft_Maui_Controls_Microsoft_Maui_Controls_Handlers_Items_TemplatedCell__UpdateCellSize (TemplatedCell.cs:113)
3   MailClient.Mobile.iOS           0x20218bc4c         _Microsoft_Maui_Controls_Microsoft_Maui_Controls_Handlers_Items_TemplatedCell__PreferredLayoutAttributesFittingAttributes (TemplatedCell.cs:107)
4   MailClient.Mobile.iOS           0x20218c870         _Microsoft_Maui_Controls_Microsoft_Maui_Controls_Handlers_Items_TemplatedCell__UpdateSelectionColor_0 (TemplatedCell.cs:346)
5   MailClient.Mobile.iOS           0x201268864         -[Microsoft_Maui_Controls_Handlers_Items_TemplatedCell preferredLayoutAttributesFittingAttributes:] (registrar.mm:5547)
6   UIKitCore                       0x30bb5c5b8         <redacted>
7   UIKitCore                       0x30bb9b6ac         <redacted>
8   UIKitCore                       0x30bb9b108         <redacted>
9   UIKitCore                       0x30bc4dc08         <redacted>
10  UIKitCore                       0x30bb7f498         <redacted>
11  UIKitCore                       0x30bb0eaa0         <redacted>
12  UIKitCore                       0x30bea1a34         <redacted>
13  MailClient.Mobile.iOS           0x2011d0804         xamarin_dyn_objc_msgSendSuper
14  MailClient.Mobile.iOS           0x2022ff3a0         _Microsoft_iOS_ObjCRuntime_LinkWithAttribute__set_SmartLink (LinkWithAttribute.cs:114)
15  MailClient.Mobile.iOS           0x2022aa418         _Microsoft_iOS_UIKit_UICollectionView__InsertItems (UICollectionView.g.cs:545)
16  MailClient.Mobile.iOS           0x20218a988         _Microsoft_Maui_Controls_Microsoft_Maui_Controls_Handlers_Items_ObservableItemsSource__Update (ObservableItemsSource.cs:304)

(ignore frame 4 and 14, they are bogus and incorrectly translated)

The mechanism how this seems to happen is that TemplatedCell.PlatformHandler is weak reference since commit 05697a6b1fc8a068ad537cbf455d2407e22e7e72. The measurement cells are cached in ItemsViewController and returned here: https://github.com/dotnet/maui/blob/61e984eb01fa8fa7fd7133a68bd76008865a01a0/src/Controls/src/Core/Handlers/Items/iOS/ItemsViewController.cs#L367-L374

These cells may have the platform handlers collected by GC and this will later result in the NRE above.

Steps to Reproduce

No response

Link to public reproduction project repository

No response

Version with bug

8.0.70 SR7

Is this a regression from previous behavior?

Yes, this used to work in .NET MAUI

Last version that worked well

Unknown/Other

Affected platforms

iOS

Affected platform versions

No response

Did you find any workaround?

No response

Relevant log output

No response

github-actions[bot] commented 3 weeks ago

Hi I'm an AI powered bot that finds similar issues based off the issue title.

Please view the issues below to see if they solve your problem, and if the issue describes your problem please consider closing this one and thumbs upping the other issue to help us prioritize it. Thank you!

Open similar issues:

Android - NullReferenceException thrown from ItemContentView (#23394), similarity score: 0.80

Closed similar issues:

[iOS] NullReferenceException in TemplatedCell (#20606), similarity score: 0.84
NRE in GridViewLayout.HandlePropertyChanged (#18828), similarity score: 0.77
[iOS] Crash during Render after adding items to an ObservableCollection (#10163), similarity score: 0.75
iOS App crashes when populating CollectionView (#22754), similarity score: 0.75

Note: You can give me feedback by thumbs upping or thumbs downing this comment.

filipnavara commented 3 weeks ago

cc @jonathanpeppers

Presumably the fix is to change this code: https://github.com/dotnet/maui/blob/61e984eb01fa8fa7fd7133a68bd76008865a01a0/src/Controls/src/Core/Handlers/Items/iOS/ItemsViewController.cs#L365-L378

to something like

            var bindingContext = ItemsSource[indexPath];

            // If we've already created a cell for this index path (for measurement), re-use the content
            if (_measurementCells != null && _measurementCells.TryGetValue(bindingContext, out TemplatedCell measurementCell))
            {
                var platformHandler = measurementCell.PlatformHandler;
                if (platformHandler is not null)
                { 
                    _measurementCells.Remove(bindingContext);
                    measurementCell.ContentSizeChanged -= CellContentSizeChanged;
                    measurementCell.LayoutAttributesChanged -= CellLayoutAttributesChanged;
                    cell.UseContent(measurementCell);
                    GC.KeepAlive(platformHandler);
                }
                else
                {
                    cell.Bind(ItemsView.ItemTemplate, bindingContext, ItemsView);
                }   
            }
            else
            {
                cell.Bind(ItemsView.ItemTemplate, bindingContext, ItemsView);
            }

However, I am not sure how is it guaranteed that this whole thing stays alive while the view is constructed and added to the hierarchy.

filipnavara commented 3 weeks ago

The more I dig deep into it the more I think the whole fix in 05697a6b1fc8a068ad537cbf455d2407e22e7e72 is misguided. The issue with UICollectionView holding to cells for later reuse and keeping the references alive for too long was addressed with 170a7a9832b2179c661a8999a79b9e77b24f9825.

jonathanpeppers commented 3 weeks ago

@filipnavara do you want to send a PR that reverts https://github.com/dotnet/maui/commit/05697a6b1fc8a068ad537cbf455d2407e22e7e72, and see if the tests pass?

filipnavara commented 3 weeks ago

@filipnavara do you want to send a PR that reverts 05697a6, and see if the tests pass?

Sure, can give it a try.

I spent couple of hours trying to make a reliable repro for this issue but it's tricky even if I induce the GC manually at some points. Unfortunately, it's reproducible enough that we crashed it at least once on each test device, but it was not easy to crash on simulator or by repeating the same steps.

In the initial scenario from the crash report above I removed couple of items. Then went with "Undo" which re-added the same item back to the collection view which triggered the crash.

filipnavara commented 1 week ago

I finally have a reliable repro: https://github.com/filipnavara/maui-collection-view-refs

Turns out the reason why we were getting constant crashes is even more sinister than I originally thought.

Here's the rough scenario our app does:

We create a carousel on a separate page an bind it to an observable items source.
This in turn creates the UICollectionView, ObservableItemsSource, and hooks to the CollectionChanged callback (strong reference!).
Now, the item source we bind outlives the page lifetime, and it's modified later on, and sends the CollectionChanged notifications.
We call DisconnectHandler on the whole tree when the page goes away (much like the default .NET 9 MAUI behavior). This causes all the platform handlers for the collection view cell to be disconnected and everything is eligible for GC, including the weak references in TemplatedCell.
However, ItemsViewHandler.DisconnectHandler doesn't remove the ItemsSource in the UICollectionView! Remember the strong reference from CollectionChanged? That one stays alive and keeps half of the zombie tree in existence. It also means that UICollectionView still has references to its cells which now point to disposed handlers.
When a specific sequence collection change operations happens in the observable items source, the zombie UICollectionView starts calling into the zombie cells and the app crashes...

RoiChen001 commented 4 days ago

I can repro this issue at iOS platform on the latest 17.12.0 Preview 1.0(8.0.82 & 8.0.70 & 8.0.61).

dotnet / maui