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

[Bug] CollectionView throws in iOS with a simple example #12080

Closed
JohnLivermore opened this issue Sep 10, 2020 · 36 comments · Fixed by #13119
Closed

[Bug] CollectionView throws in iOS with a simple example #12080

JohnLivermore opened this issue Sep 10, 2020 · 36 comments · Fixed by #13119
Assignees
Labels
a/collectionview in-progress This issue has an associated pull request that may resolve it! m/high impact ⬛ p/iOS 🍎 t/bug 🐛

Comments

@JohnLivermore
Copy link

JohnLivermore commented Sep 10, 2020

Description

I have 2 collection views that are shown by toggling 2 buttons on a form. When switching between them, iOS throws an exception. Works fine on Android.

Steps to Reproduce

  1. Run the sample app
  2. Click the buttons at the bottom... first Meters, then Tanks, then Meters, exception occurs

image

image

I have seen an exception thrown from here as well...

image

The top level Load() call wraps everything with Xamarin.Forms.Device.BeginInvokeOnMainThread.

Basic Information

  • Version with issue: 4.8.0.1364
  • IDE: VS 2019

image

  • Nuget Packages:

image

  • Affected Devices:
    iOS only

Reproduction Link

Download sample code to reproduce the issue from my GitHub page. This code was created today with File/New Project (so it's not anything legacy related).

Workaround

none at this time

@JohnLivermore JohnLivermore added s/unverified New report that has yet to be verified t/bug 🐛 labels Sep 10, 2020
@samhouts samhouts added this to New in Triage Sep 10, 2020
@StephaneDelcroix StephaneDelcroix added a/collectionview p/iOS 🍎 and removed s/unverified New report that has yet to be verified labels Sep 11, 2020
@StephaneDelcroix StephaneDelcroix moved this from New to Ready For Work in Triage Sep 11, 2020
@samhouts samhouts moved this from Ready For Work to Needs Estimate in Triage Sep 14, 2020
@samhouts samhouts added this to the 5.0.0 milestone Sep 15, 2020
@NLGhofman
Copy link

I had the same problem on iOS phones, tablets were just fine.
While this is a issue that needs to be address, what seemed to work for me was
Items = new Observerable<Class>()

@JohnLivermore
Copy link
Author

Thanks for the tip. I tried Items = new ObservableCollection<T>() just now (instead of Items.Clear()). I didn't get the error, but the collectionviews don't show anything. I was hopeful that would work, but it didn't.

@NLGhofman
Copy link

Thanks for the tip. I tried Items = new ObservableCollection<T>() just now (instead of Items.Clear()). I didn't get the error, but the collectionviews don't show anything. I was hopeful that would work, but it didn't.

You did add the SetField(ref _items, value) for the ObservableCollection? (just to confirm)

@JohnLivermore
Copy link
Author

JohnLivermore commented Sep 15, 2020

I noticed this was labeled for the 5.0.0 milestone. It's critical we get this working soon as we are stuck on XF 4.5 until it gets resolved.

XF 4.5 has the depricated UIWebView. This will prevent us from updating our app starting in December 2020.

I did confirm the latest XF does not have the depricated UIWebView issue, so we need to get to that version quickly so we can ferret out any other issues we might encounter before December 2020.

Can this fix be bumped to an earlier release other than v5?

@JohnLivermore
Copy link
Author

Thanks for the tip. I tried Items = new ObservableCollection<T>() just now (instead of Items.Clear()). I didn't get the error, but the collectionviews don't show anything. I was hopeful that would work, but it didn't.

You did add the SetField(ref _items, value) for the ObservableCollection? (just to confirm)

I didn't, but just tried that and it fixed the crash. I still have some weird display issues where both panes have to be Visible in order for the collection view to render properly, but I think I can work around that. Thanks for the tip!

This issue does need to be resolved however. It's definitely a bug. I would think many would be encountering it? The example reflects a common design pattern for Xamarin apps.

@NLGhofman
Copy link

NLGhofman commented Sep 15, 2020

You are welcome 👍
I do however agree that this does need to be fixed so for everyone else that has the same issue.

@JohnLivermore
Copy link
Author

JohnLivermore commented Sep 15, 2020

For the MS team. There is another issue shown in this same example. Both issues might be related, not sure.

There are two ContentViews that are alternately displayed depending on which button is clicked at the bottom of the screen. The buttons toggle the ContentView IsVisible property true/false making it show/not show.

When the app comes up the collectionview on the panel that is not visible doesn't show it's contents properly. Only by using @NLGhofman suggestion above to new the collection each time, combined with initially making ALL the ContentViews visible solve the issue. Here is what it looks like when you click the Meters button the first time. There are some very faint dots that I believe are the collectionview trying to render itself.

image

Once you toggle back and forth (again, with @NLGhofman suggestion in place), they show up normally.

So the other issue is when the CollectionView is on a form not visible, it doesn't load properly. This is very problematic for our application, for in one section, it's a single page that shows/hides ContentViews based on tabs that are selected. To get around this bug, I think we will have to show every panel IsVisible=true on initialization, load all the collectionviews, and then hide the panels. At a minimum this will create anti-patterns just to get around this problem. Ultimately it will lead to an intractable situation where the app just doesn't work until this bug is fixed.

@JohnLivermore
Copy link
Author

With the iOS UIWebView deadline looming, this is a red hot issue for us. I have been reconfiguring our app to show all panels upon startup, load them all, and then hide them as needed. It's working better, but still not consistent. At this point there is no workaround. I would appreciate an update on when this will be fixed please.

@lafritay
Copy link

I can confirm that this happens on 4.7.0.1351. I was able to work around by not calling Clear() and instead using a new observable.

@timahrentlov
Copy link

We're now getting this as well in a production App. Please fix.

@sikkemap
Copy link

Same here, we now added a work around but it would be better to have this fixed.

@jormenjanssen
Copy link

Multiple crashes here also when clearing a collection, this should not lead to crash.
This is such a basic functionality, please prioritize this

@ThumbGen
Copy link

Just to confirm, what steps are required to workaround this bug?

SetField(ref _items, value) ?

Replace Items.Clear() with Items = new ObservableCollection() ?

TIA

@NLGhofman
Copy link

NLGhofman commented Oct 20, 2020

Just to confirm, what steps are required to workaround this bug?

SetField(ref _items, value) ?

Replace Items.Clear() with Items = new ObservableCollection() ?

TIA

Yes, that should work for a workaround :)

The "Setfield(ref _items, value)" was cause the original writer didn't have the binding set.

with kind regards,

@jormenjanssen
Copy link

@lafritay @hartez When trying to work-around this with replacing the collection the following occurs #8308

Any solution? at the moment all usages to collection views are leading to crashes

@lafritay
Copy link

@jormenjanssen I have a number of workarounds in place and I think I've got it to a point where it can limp along. I just posted them here: #8308 (comment). @hartez if you get a chance to look at the workarounds and give your opinion if they are safe or not, that would likely help a number of us.

@Codelisk
Copy link

We get the error on Android 10 too

@inimirpaz
Copy link

Same here, sporadically.

@samhouts samhouts removed this from the 5.0.0 milestone Nov 2, 2020
@RLittlesII
Copy link

I am seeing this on Xamarin.Forms version 4.7.0.968 with iOS on a CollectionView. I might be able to provide a reproduction if that will help.

I agree with the assessment here #12080 (comment) as I am getting bugs reported when we select an item on the CollectionView.

@chrisfoulds
Copy link

Same issue, XF 4.8.1687 and iOS 13.7 and iOS 14.2
This is a super critical bug, how does stuff like this keep slipping out, they busy doing podcasts and videos talking about XF5.0 yet they keep releasing garbage.

@Nelo-cool
Copy link

Nelo-cool commented Nov 25, 2020

Same issue, XF 4.8.1687 and iOS 13.7 and iOS 14.2
This is a super critical bug, how does stuff like this keep slipping out, they busy doing podcasts and videos talking about XF5.0 yet they keep releasing garbage.

Easy:) you are not their main customers :) Same issue 4.8.0.1687

@philipwilford
Copy link

I am getting this error too.
The app works fine on Android, but on IOS it is very unstable.
I have a Collectionview in a RefreshView.
I update some details using a menuswipe, and it seems to up date the unlying model, but sometimes updates the screen
and other times shows the old data or as above, it throws a fault.
This is very frustrating and my users are giving me a hard time.
i am having to use a work around of going to another screen to update the data and then return to the page.

@hartez hartez self-assigned this Dec 10, 2020
@praeclarum
Copy link
Contributor

praeclarum commented Dec 12, 2020

The problem lies with a race condition in the batch updater. It is failing to stay synchronized with the collection.

void CollectionChanged(object sender, NotifyCollectionChangedEventArgs args)
{
if (Device.IsInvokeRequired)
{
Device.BeginInvokeOnMainThread(() => CollectionChanged(args));
}
else
{
CollectionChanged(args);
}
}

BeginInvokeOnMainThread gives no guarantee that the function will run before the collection is modified again (your task might get scheduled for the beginning or end of the next update loop). Therefore it gets out of sync and bad things happen.

If you are running into this issue...

My best advice is to make sure you only mutate your view model collections on the main thread. Doing it on another thread will cause these kinds of problems among others (random Dispose errors).

While BeginInvoke is causing Xamarin problems here, you should absolutely use it in your own code when modifying these collections.

Suggested Xamarin.Forms improvement

I suggest that the function print a warning message to the console if it gets called off the main thread. This is never actually safe given the race conditions created. But it can be hard to track down what code is mutating the collection in a background thread. Printing to the console would help.

Perhaps a global flag ThrowOnBackgroundThreadAccess could help track these things down.

Full Xamarin improvement

If we want the collection to be thread safe then Xamarin will have to track a shadow copy of the collection that it mutates in a controlled manner.

@filipnavara
Copy link
Contributor

If we want the collection to be thread safe then Xamarin will have to track a shadow copy of the collection that it mutates in a controlled manner.

That's what was done on Android (ab55162). It broke our application really badly. We use our own virtualized collection that already did all the dispatching to main thread by itself. The shadow copy caused all the items to be materialized and loaded from DB which caused >30 second hangs.

@praeclarum
Copy link
Contributor

Instead of a shadow list, the UI thread could be synchronized:

void CollectionChanged(object sender, NotifyCollectionChangedEventArgs args) 
{ 
	if (Device.IsInvokeRequired) 
	{
		using var e = new ManualResetEvent(false);
		Device.BeginInvokeOnMainThread(() => { CollectionChanged(args); e.Set(); });
		e.Wait(); 
 	} 
 	else 
 	{ 
 		CollectionChanged(args); 
 	} 
 } 

This would block the event handler until the UI thread has fetched and synchronized with the collection.

I didn't propose it before because I generally don't like blocking on the UI thread. But in this case, it is a safe fix for people changing collections off thread and won't have @filipnavara's materialization problems.

@hartez
Copy link
Contributor

hartez commented Dec 13, 2020

@filipnavara Did you file an issue for that problem? Maybe we can make the shadow copy less eager. Alternatively, we might be able to add an option to disable the shadow copy.

@hartez hartez linked a pull request Dec 13, 2020 that will close this issue
2 tasks
@hartez hartez added this to Backlog in CollectionView via automation Dec 13, 2020
@hartez hartez removed this from Needs Estimate in Triage Dec 13, 2020
@hartez hartez moved this from Backlog to In Review in CollectionView Dec 13, 2020
@hartez hartez added the in-progress This issue has an associated pull request that may resolve it! label Dec 13, 2020
@filipnavara
Copy link
Contributor

@hartez We didn't make an issue yet. We only discovered it last week. I'll make sure to file an issue next week.

@hartez hartez closed this as completed in 626f756 Dec 17, 2020
CollectionView automation moved this from In Review to Done Dec 17, 2020
@rodrigojuarez
Copy link

So, is this fixed? Version?

@lor-olo
Copy link

lor-olo commented Feb 14, 2021

It's hard to believe, but this hasn't been fixed yet.
I can't upgrade to Xam Forms 5 until this works. :(

@Denrage
Copy link

Denrage commented Mar 3, 2021

I'm getting a MonoTouchException: 'Objective-C exception thrown. Name: NSRangeException (index 1 beyond bounds [0 ... 0])' in Xamarin.Forms 5

@meJevin
Copy link

meJevin commented Mar 7, 2021

This is still an issue.

@rodrigojuarez
Copy link

@meJevin is this fixed?

@meJevin
Copy link

meJevin commented Mar 8, 2021

@rodrigojuarez, in my case, the fix was simple. I just had a race condition in my code, which I fixed and the crash went away.

@meJevin
Copy link

meJevin commented Mar 8, 2021

@rodrigojuarez, refer to #12080 (comment)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a/collectionview in-progress This issue has an associated pull request that may resolve it! m/high impact ⬛ p/iOS 🍎 t/bug 🐛
Projects
Development

Successfully merging a pull request may close this issue.