search

xamarin / Xamarin.Forms

Xamarin.Forms is no longer supported. Migrate your apps to .NET MAUI.
https://aka.ms/xamarin-upgrade
Other
5.62k stars 1.87k forks source link

[Bug] Crash in Layout. ResolveLayoutChanges #9062

Open slang25 opened 4 years ago

slang25 commented 4 years ago

Description

Crash in Layout.ResolveLayoutChanges: System.InvalidOperationException: Collection was modified; enumeration operation may not execute.

Steps to Reproduce

Steps are unknown yet, we are trying to reliably recreate it. We are seeing the crashes through AppCenter and are getting plenty of them across different Android devices.

I have a console app that replicates the underlying bug: https://gist.github.com/slang25/f26349340f74300d67f7c552cb2b2493

Expected Behavior

No crash.

Actual Behavior

In a StackLayout, in Layout.OnChildMeasureInvalidated, it should queue up layout update operations in s_resolutionList for Layout.ResolveLayoutChanges to process.

Layout.ResolveLayoutChanges "copies" s_resolutionList, however it copies the reference, this means that if GetElementDepth is slow during OnChildMeasureInvalidated, it could have dereferenced the old list. By adding an item to the old s_resolutionList reference, it risks this exception for occurring.

Other Information

Console app that replicates basic non-thread-safe underlying issue can be found here: https://gist.github.com/slang25/f26349340f74300d67f7c552cb2b2493

There are a few quick fixes, including this: https://github.com/xamarin/Xamarin.Forms/compare/xamarin:master...slang25:fix-layout-crash?expand=1

But I'd like to work on a more complete fix.

slang25 commented 4 years ago

This is our top crasher at the moment: image

slang25 commented 4 years ago

We think this maybe due to us constructing ContentPages off from the main thread. Well report back when we have verified. Can you confirm OnChildMeasureInvalidates should only ever be called from the main thread, the code certainly looks that way. I'd like a way to get debug time warnings for when we do this to avoid undefined behaviour.

slang25 commented 4 years ago

Some more context:

We have a hybrid Xamarin Forms app, by that I mean we have a native Xamarin app, and we embed the ContentPages ourselves, this means duplicating some of the XF boilerplate stuff, but it gives us a lot of power and control (we're quite excited about it as an approach).

We were constructing the ContentPage on a background thread for performance (as mentioned here: https://youtu.be/RZvdql3Ev0E?t=1096), then we call page.CreateSupportFragment() after on a UI thread.

I understand that CreateSupportFragment will add the page to the visual tree, we think we had a glitch where this was being run on a background thread, causing this issue. The PR I have would mitigate this exception, but ultimately it's fixing an unsupported scenario and trying to to control what is accepted as undefined behaviour, so I don't expect you to merge it and I'll close now.

Is my understanding right here with regards to what is supported? i.e. ContentPage can be constructed off main thread, but CreateSupportFragment must be called on main thread.

FabienBehra38 commented 4 years ago

Hi, I encountered this issue many times and I didn't fixed it yet. There is anyone found a solution or a work-around ?

Thank you for the answer.

slang25 commented 4 years ago

The problem for us was we were doing something we shouldn't from a background thread, if you are seeing this there's a good chance you're doing the same.

The workaround I found was to make a tiny change to XF itself, like this: https://github.com/xamarin/Xamarin.Forms/compare/xamarin:master...slang25:fix-layout-crash?expand=1

While this would mostly solve our crasher, it would be masking the fact that we would be doing something unsupported, so I closed it.