Xamarin.forms: [Bug] InvalidOperationException in TypedBinding`2[TSource,TProperty].Apply

[mfeingol]

Description

Seeing these unhandled exceptions in my app. I believe they occur while adding and removing leaf items in a grouped ListView bound to an ObservableCollection of ObservableCollections.

I am using compiled bindings, in case that helps.

System.InvalidOperationException: Operation is not valid due to the current state of the object.

This is the stack trace I see in AppCenter:

TypedBinding`2[TSource,TProperty].Apply (System.Boolean fromTarget)
TypedBinding`2+PropertyChangedProxy[TSource,TProperty].<OnPropertyChanged>b__16_0 ()
Thread+RunnableImplementor.Run ()
IRunnableInvoker.n_Run (System.IntPtr jnienv, System.IntPtr native__this)
(wrapper dynamic-method) Android.Runtime.DynamicMethodNameCounter.39(intptr,intptr)

Steps to Reproduce

I do not yet have a solid repro, sorry.

Expected Behavior

Actual Behavior

Basic Information

• Version with issue: 3.6 latest
• Last known good version: Unknown
• IDE: VS 2019
• Platform Target Frameworks:
  
  
  
  • Android: 9.0
  
  
• Android Support Library Version: Latest
• Nuget Packages:
• Affected Devices:

Screenshots

Reproduction Link

[samhouts]

@mfeingol If you're able to narrow this down, can you please attach a small project that demonstrates this issue? Thanks!

[mfeingol]

I'll try to do that this weekend, but this one's kind of hard to reproduce on demand. Any clues from your side, based on your understanding of the code?

[samhouts]

It looks like this is related to XAML binding using a DataType.

[mfeingol]

I'm traveling now, but fwiw my temporary workaround was to disable compiled bindings.

[samhouts]

@mfeingol Okay, that sounds about right. If you get a chance, please provide a sample project to help us narrow the issue. Thanks!

[mtirona]

I am seeing this same issue on both iOS and Android. I am not using compiled bindings.

AppCenter issue:

TypedBinding2[TSource,TProperty].Apply (System.Boolean fromTarget) TypedBinding2+PropertyChangedProxy[TSource,TProperty].b__16_0 ()
Thread+RunnableImplementor.Run ()
IRunnableInvoker.n_Run (System.IntPtr jnienv, System.IntPtr native__this)
(wrapper dynamic-method) System.Object.26(intptr,intptr)

Xamarin forms project, Xaml page with ViewModel - Grid containing ScrollView - ScrollView contains labels. Pretty simple.

[mfeingol]

@samhouts: I've tried my best, but I'm unable to reproduce the issue in a sample app. I do, however, have a 100% repro in my production app.

Can I give someone on your team permissions to my Azure DevOps project so they can reproduce the problem?

[PureWeen]

shneuvil at microsoft.com

[mfeingol]

You're in: https://dev.azure.com/sideroads/Sideroads

[mfeingol]

Repro steps:

1. git checkout 6698-repro and build. You may need to add the External directory to your list of NuGet directories to pick up a package.
2. Run the app and create a new trip from the trips page. Name the trip something like "Test" and leave all defaults except specifying departure and arrival and airports (e.g. SEA and LAS).
3. Tap the trip to select it.
4. From the hamburger menu, go to the weather page, confirm weather reports are showing up for the relevant locations.
5. Go to settings, change the weather provider from the NWS to HERE.
6. Go back to the weather page. You should see a crash pretty quickly. Restarting the app should re-open in the weather page, refresh weather, and generate another crash.

If for some reason you can't repro with such a simple trip, I'll export a more sophisticated one for you with more locations.

[mfeingol]

@PureWeen: let me know if this isn't working for you.

[mfeingol]

@mtirona: I missed your comment earlier, sorry. That's odd that you're seeing this without using compiled bindings. I don't suppose you have a simple repro project that triggers the crash on demand?

[mtirona]

@mfeingol This issue is appearing randomly within our application. I don't have a simple to recreate on demand - I do have hundreds of crashes in AppCenter documenting the issue. I would be grateful for any directions of how to prevent the issue...

[mfeingol]

@mtirona: are you 100% sure you're not using x:DataType anywhere in your XAML?

[mtirona]

@mfeingol There was x:DataType on parent pages so I have been through the entire application and created a branch where I have removed the x:DataType. I am in the process of having our QA test this branch for crashes to see if this resolved the issue.

[StephaneDelcroix]

I'm five-nine certain that this is related to XamlC, unless you create TypedBindings in your own code (those are _only_ created by the Xaml compiler)

[mfeingol]

Side note: I've been on Xamarin Forms 3.6, and I just updated to the latest 4.1. On first run of my app, I immediately saw this crash while rapidly adding items to a grouped listview page. So the issue is still present with 4.1.0.618606.

[mfeingol]

@PureWeen: I've updated the repro steps above to reference a branch you can checkout that should repro the issue immediately for you.

[PureWeen]

@mfeingol can you do this for me?

If for some reason you can't repro with such a simple trip, I'll export a more sophisticated one for you with more locations.

I tried your steps and nothing crashed for me

[PureWeen]

Alright, so @mfeingol I'm not sure how or why your data lines up to cause this issue but this is what I know so far

The TypedBindings uses a WeakReference to the BindableObject so if it's the only thing that references the BindableObject then it will lose that reference.

Here's the code that crashes
```C#
if (!_weakTarget.TryGetTarget(out target))
throw new InvalidOperationException();

If you look at the output of the application while it's running the exception always happens after a GC which makes sense why you are only seeing this after loading a larger data set.

As a test with the TypeBindings I created a nuget where it stores the source and target to a local variable just to see what would happen 

```C#
            _weakSource.SetTarget(source);
            _weakTarget.SetTarget(bindObj);
            _bindObj = bindObj;
            _source = source;
            ApplyCore(source, bindObj, targetProperty);
        }

        BindableObject _bindObj;
        object _source;

And if I do that then the crash doesn't happen.

My thinking is that somehow your data is getting to a place where the only thing that is referencing the instance of LocationDayWeatherViewModel that's bound to the ViewCell is the TypeBinding and that's it. I still haven't quite tracked down if this is somehow an exception on our side vs yours. Can you make sure you're maintaining a reference to all created LocationDayWeatherViewModel that are bound to the ListView?

[mfeingol]

Thanks for looking into this! This issue has been pretty hard to reproduce outside of the full app, so it makes sense that it would be something involving weak references. I wonder if adding memory pressure to a simpler repro might result in the same issue.

Anyway, the only time an instance of LocationDayWeatherViewModel would be unreferenced by the viewmodel would be during a weather day refresh, in which the code clears the bound ObservableCollection of items prior to inserting fresh items. This happens deep in ExpandableGroupCollectionViewModel.Refresh in case you want to set bps.

However, I'd think that clearing a bound ObservableCollection should be a safe operation, and it's not obvious to me why the XF bindings would be using weak refs. That sounds like it would have exactly this kind of race.

Aside: I tried copying the items in this into a temporary list prior to clearing in Refresh, but surprisingly, that didn't seem to work around the issue. I did reference the list at the end of the method to make sure the GC didn't nuke it as well. I guess this would make sense if XF is attempting to use the stale binding "later", and it would suggest there's no good way for an app to stabilize bound references. But I may be misunderstanding things.

(And yes, the weather/expander code is a bit out of control. I wish XF had a native Expander control...)

[PureWeen]

I think adding memory pressure or just calling GC.Collect might recreate the issue on a smaller project. It might also happen because the OnPropertyChanged on TypedBinding is marshaled to the UI Thread

https://github.com/xamarin/Xamarin.Forms/blob/7cc9a282bdeb76405c793574ebe0d096072f4228/Xamarin.Forms.Core/TypedBinding.cs#L275

So with a clear I'm thinking what might be happening

PropertyChanged queue'd on UI Thread
Clear
GC
WeakReference loses reference
PropertyChanged now resolves and throws an exception

Maybe it's related to this issue?
https://github.com/xamarin/xamarin-android/issues/2049

@StephaneDelcroix might have some additional insights at this point :-)

[mfeingol]

I had a little free time last night, and I tried adding a GC.Collect() after the call to Clear() in my simplified repro. Unfortunately I was unable to reproduce the problem that way.

[PureWeen]

Did you wait for the pending finalizers?

We do it like this in our tests just to be super sure
https://github.com/xamarin/Xamarin.Forms/blob/a76db1407a76fccbf487425db1bca5d15f925127/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Helpers/GarbageCollectionHelper.cs#L9

[mfeingol]

That's a really good call. But sadly, even after that, the Clear() doesn't trigger a repro.

So taking a step back... how should this work? If a binding is holding a weak reference, then by definition that object can be GC'd. So that's something that, well, can happen, and XF shouldn't be throwing an uncatchable exception. Instead, it should probably just ignore the PropertyChanged notification and trust the app knows what it's doing. I mean, what else can it do?

[PureWeen]

Can you attach the repro you are using to try and recreate?

[mfeingol]

I'll attach the code tonight. It's basically the in-app repro you've already seen, extracted into a standalone app. But I have yet to see the problem repro inside it.

[mfeingol]

TypedBindingCrashes.zip

[mfeingol]

@PureWeen: anything else I can do to help diagnose this?

[PureWeen]

@mfeingol not right now unless you get any ideas how to repro with the standalone app. It's just a bit tricky to whittle it down in the larger one so we'll bit a little bit slower with resolving this one.

[mfeingol]

Is it possible we can think this one through conceptually, as I mentioned in https://github.com/xamarin/Xamarin.Forms/issues/6698#issuecomment-519359760? Or are there still missing pieces on what's actually causing the problem?

[ysmoradi]

I've the same issue and I'm also unable to reproduce that easily )-:

[mfeingol]

@ysmoradi: I don't suppose you're able to upload a simple repro?

[ysmoradi]

It's hard to reproduce even in my production app!

[mfeingol]

I have a repro in production as well, but as per @PureWeen it's hard for them to understand the issue without a simpler repro. I've tried and failed to produce one. :-(

[ysmoradi]

I think it's a good idea to have property name + view type name + binding context's type name in exception's message, so we can have better insights.

[shoyheim]

I'm having the exact same issue, iOS and Android, using xamarin forms 4.2.0.709249.
I have a ListView using a DataTemplate to visualize the objects, defined in my xaml.
I have DataType set on the xaml page and then a different one on the listview datatemplate.

I don't have to call clear or replace or anyting on the list I'm binding to, it seems to be enough to call GC.Collect and have an await to another method to give me the error above. (If I don't have the GC.Collect call it fails very rarely, but it does every once in a while, with the call it loads successfully, but fails on refresh.)

However, I did find something interesting, if I remove my binding between my viewModel.IsBusy to the listview.IsRefreshing the error goes away, (but the refreshing indicator keeps running after pull to refresh of course).

Also, if I remove the datatype on the contentpage, and only set it on the datatemplate, the error also goes away, and I can use binding on the listview IsRefresing.

To summarize what I need to have in my production app to reproduce:

1. Set DataType on the ContentPage
2. Set Binding on IsRefreshing in ListView
3. Have a GC.Collect call followed by await Task.Delay(250); in the method bound to the listview RefreshCommand

[mfeingol]

@shoyheim: do you have a simple repro you can post here?

[shoyheim]

@shoyheim: do you have a simple repro you can post here?

@mfeingol Sorry, I've been testing/debugging my production code. I'll see if I can squeeze some time in to make something to reproduce the error...

[mfeingol]

@shoyheim: did you ever have any luck with this?

[shoyheim]

@mfeingol
No sorry, I tried, but whenever I make a simple repro, I can't make it crash, but my production app keeps crashing and crashing.
Now i removed all my compiled-bindings and just use runtime bindings in my xaml files, and the crashes has disappeared.
I've spent way to much time trying to figure out what goes wrong, and the only thing I'm certain of is that there's a connection between using ListView with a binding on "isRefreshing" to show when the list is refreshing, and using compiled bindings... it also seems that the crashes happens around the time of garbage collection.

[ysmoradi]

1- A property of binding context ( view model ) gets changed.
2- We pop view so it gets destroyed.
3- GC gets called.

1. Binding.Apply gets called and it tries to get access required objects which are gone so it throws InvalidOperationException.
   You might ask why step 4 gets executed so late? Because it was called in Device.BeginInvokeOnMainThread, and such an action will be added to the end of list of main thread's actions queue.
   I was able to handle that exception by providing custom PlatformServices to xf and there is no crash anymore, but it throws an exception more than 800 times! For almost all typed binding of destroyed page

[mfeingol]

@ysmoradi: I spent some time attempting to reproduce your steps, and I was unable to trigger any crashes. I don't suppose you have a sample project.

[ysmoradi]

I've said before that I don't have any simple repo and it's happening in my production app, and it also happens randomly.
The first time I started to create a repo, I wasn't aware of many things, but tomorrow I'll try to create another one using my new assumptions.
Another tips which might help: Based on my assumptions, having concurrent GC enabled results into increased chance to reproduce the crash. Because it can collect objects on a separated thread. We also need to change a view model's property on a separated thread/task because Device.BeginInvokeOnMainThread passes the action to the end of main threads' queue only when it's called on a thread other than main thread.
Try again and let me know if you found something, and I'll try tomorrow.

[mfeingol]

I have concurrent GC enabled. Using your suggestions, the snippet of code I'm using does this:

            Page1 page = new Page1();
            await this.Navigation.PushModalAsync(page);
            await Task.Run(() => { page.TXT = "Foo"; });
            await this.Navigation.PopModalAsync();

            GC.Collect();
            GC.WaitForPendingFinalizers();

            GC.Collect();
            GC.WaitForPendingFinalizers();

TXT is a property on Page1, implemented using a BindableProperty. Page1 is using compiled bindings.

Still no luck.

[fmanseau]

Getting the same issue randomly here also in my production app. After going through the tedious process of setting x:Datatype everywhere as recommended in the performance guidelines, can't say that I'm thrilled to potentially have to remove them all because of this issue.

I'll update this with a repro app if I can manage to get one working.

Note: I doubt this is related, but it seems I only started seeing the issue after I also started to use compressed layouts. Probably a coincidence since the issue is very random, but who knows.

[maxkoshevoi]

Just updated my app to MVVM with compiled bindings and getting the same error now.
Running latest Xamarin.Forms: 4.2.0.848062

With following config:

Also don't have repro steps (this version of the app is in beta for a day and I already have two reports via AppCenter about it).
Can share app repository, but without repro steps, I guess it wouldn't help much.

[fmanseau]

I might not be understanding the whole picture, but isn't the easy solution for this is simply to Unapply and return when the target got GCed, like it's already the case without the DO_NOT_CHECK_FOR_BINDING_REUSE define in TypedBinding.cs ? I don't see how throwing is a good idea here.

[mfeingol]

@fmanseau: I have the same view. @PureWeen?

[fmanseau]

Also if it helps, an issue related to this in Prism's repo seems to have repro steps (it requires a Prism app, but probably easy to follow a similar pattern without).

https://github.com/PrismLibrary/Prism/issues/1688

[gentledepp]

@StephaneDelcroix @kingces95 @wachs
You guys seem to be behind the TypedBinding implementation.

As you can see in this thread, many devs out there (including me) have their app crash now and then because of an InvalidOperationException thrown by the TypedBinding.
It seems @samhouts is right in her assumption, that after the GC kicks in and removes the target of a TypedBinding (as is very likely the case in our app, where we have a rather big ListView with many complex DataTemplates), the TypedBinding throws an InvalidOperationException which is never caught and leads to an application crash on Android (and potentially any other platform??) like this:

TypedBinding2[TSource,TProperty].Apply (System.Boolean fromTarget) System.InvalidOperationException: Operation is not valid due to the current state of the object. Stack traces TypedBinding2[TSource,TProperty].Apply (System.Boolean fromTarget)
TypedBinding`2+PropertyChangedProxy[TSource,TProperty].b__16_0 ()
Thread+RunnableImplementor.Run ()
IRunnableInvoker.n_Run (System.IntPtr jnienv, System.IntPtr native__this)
(wrapper dynamic-method) Android.Runtime.DynamicMethodNameCounter.43(intptr,intptr)

Now @mfeingol reasonably asks why there is an InvalidOperationException that causes this crash after all. I mean it was conceptually not allowed to have the target be garbage collected, why use a weak reference?

I myself are a WPF developer since .NET 3.0 and know that binding exceptions never caused a crash, but were only logged.

So my question:
Is there any good reason why the TypedBinding throws an exception and does not just ignore the target if it was collected?

Or maybe the binding system itself should catch all binding exceptions and allow us developers to swallow them or react appropriately?

This is really an important bug for our production app and if required, I'd create an intermediary Xamarin.Forms release for us that fixes this. But for that I want to know what unwanted effects this could have!

@bruzkovsky - this may be of interest to you too

[gentledepp]

Moreover, @StephaneDelcroix @kingces95 @wachs , I can see a flag
DO_NOT_CHECK_FOR_BINDING_REUSE
What exactly is it for?

I would very well like to compile Xamarin.Forms with DO_NOT_CHECK_FOR_BINDING_REUSE set to true to get rid of this bug.
But what is the thought process behind it? There must be a good reason to have this flag present, right?

[mrjavicho]

This have just happened to me in a simple ContentPage with a ListView and DataTemplateSelector.
Is there any update on this issue?
How come it is currently recommended to use compiled bindings if this issue is open??
https://docs.microsoft.com/es-es/xamarin/xamarin-forms/app-fundamentals/data-binding/compiled-bindings

[mfeingol]

@mrjavicho: any chance you can post a simple project that reproduces the problem consistently?

[usausa]

I was able to reproduce the problem frequently with this sample.
https://github.com/usausa/TypedBindingIssue

Run the application and click the Test button.
Also, if you remove the comment in MainPage.Cleanup(), the problem will not occur.

[mfeingol]

Here's the stack trace I'm seeing using @usausa's code. Can't tell if it's the same issue as above, but it's definitely an issue:

    0x16 in Xamarin.Forms.Internals.TypedBinding<TypedBindingIssueApp.View1ViewModel,string>.Apply at D:\a\1\s\Xamarin.Forms.Core\TypedBinding.cs:99,5  C#  Annotated Frame
    0x7 in Xamarin.Forms.Internals.TypedBinding<TypedBindingIssueApp.View1ViewModel,string>.PropertyChangedProxy.<OnPropertyChanged>b__16_0 at D:\a\1\s\Xamarin.Forms.Core\TypedBinding.cs:277,31   C#  Annotated Frame
    0x29 in Xamarin.Forms.DispatcherExtensions.Dispatch at D:\a\1\s\Xamarin.Forms.Core\DispatcherExtensions.cs:53,6 C#  Annotated Frame
    0x3F in Xamarin.Forms.Internals.TypedBinding<TypedBindingIssueApp.View1ViewModel,string>.PropertyChangedProxy.OnPropertyChanged at D:\a\1\s\Xamarin.Forms.Core\TypedBinding.cs:277,5    C#  Annotated Frame
    0x15 in Xamarin.Forms.BindingExpression.WeakPropertyChangedProxy.OnPropertyChanged at D:\a\1\s\Xamarin.Forms.Core\BindingExpression.cs:645,6    C#  Annotated Frame
>   0x14 in TypedBindingIssueApp.NotificationObject.RaisePropertyChanged at C:\Operations\Build\External\TypedBindingIssue\TypedBindingIssueApp\TypedBindingIssueApp\NotificationObject.cs:13,13    C#  Symbols loaded.
    0x5D in TypedBindingIssueApp.LongLifecycleModel.Next at C:\Operations\Build\External\TypedBindingIssue\TypedBindingIssueApp\TypedBindingIssueApp\LongLifecycleModel.cs:19,13    C#  Symbols loaded.
    0x2A in TypedBindingIssueApp.MainPage.Button_OnClicked at C:\Operations\Build\External\TypedBindingIssue\TypedBindingIssueApp\TypedBindingIssueApp\MainPage.xaml.cs:29,17   C#  Symbols loaded.
    0x6 in System.Runtime.CompilerServices.AsyncMethodBuilderCore.MoveNextRunner.InvokeMoveNext at /Users/builder/jenkins/workspace/archive-mono/2019-08/android/release/mcs/class/referencesource/mscorlib/system/runtime/compilerservices/AsyncMethodBuilder.cs:1092,17   C#  Annotated Frame
    0x73 in System.Threading.ExecutionContext.RunInternal at /Users/builder/jenkins/workspace/archive-mono/2019-08/android/release/mcs/class/referencesource/mscorlib/system/threading/executioncontext.cs:968,17   C#  Annotated Frame
    0x4 in System.Threading.ExecutionContext.Run at /Users/builder/jenkins/workspace/archive-mono/2019-08/android/release/mcs/class/referencesource/mscorlib/system/threading/executioncontext.cs:910,13    C#  Annotated Frame
    0x32 in System.Runtime.CompilerServices.AsyncMethodBuilderCore.MoveNextRunner.Run at /Users/builder/jenkins/workspace/archive-mono/2019-08/android/release/mcs/class/referencesource/mscorlib/system/runtime/compilerservices/AsyncMethodBuilder.cs:1073,25 C#  Annotated Frame
    0x6 in System.Threading.Tasks.SynchronizationContextAwaitTaskContinuation.<>c.<.cctor>b__7_0 at /Users/builder/jenkins/workspace/archive-mono/2019-08/android/release/external/corert/src/System.Private.CoreLib/src/System/Threading/Tasks/TaskContinuation.cs:379,78  C#  Annotated Frame
    0xC in Android.App.SyncContext. C#  Annotated Frame

[bruzkovsky]

The repro is spot on! The first thing I noticed was that the crash occurs at completely random times, sometimes it pressing the button multiple times and waiting, so I tweaked it a little bit to make it crash sooner (consistently after 29 entities):

Just raise the PC event 80 times like this:
c# for (var i = 0; i < 80; i++) RaisePropertyChanged(nameof(Entity));

This schedules a lot more events to the dispatcher, which increases the failure rate.

[bruzkovsky]

When disabling XAML compilation for the View1 and View2 classes, then there is a NullReferenceException thrown in BindingExpression.cs:

Still searching for a simple workaround...