Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] MultiBinding converter use throwing TargetInvocationException #11058

Closed
davidbritch opened this issue Jun 15, 2020 · 5 comments
Closed
Assignees
Labels
a/binding ⛓ i/high Completely doesn't work, crashes, or is unusably slow, has no obvious workaround; occurs less often in-progress This issue has an associated pull request that may resolve it! s/unverified New report that has yet to be verified t/bug 🐛
Projects

Comments

@davidbritch
Copy link

Description

I wrote MultiBinding demos against 4.7-pre1 and they all made sense and worked. In 4.7-pre4, post MultiBinding overhaul, a number of them no longer work. One of the issues seems to be that IMultiValueConverter implementations aren't receiving data.

For example, given the following code the AllTrueConverter should receive an array of 3 items of data. Instead, the converter is receiving an array of 3 null items.

        <CheckBox Grid.Row="1"
                  Grid.Column="1"
                  HorizontalOptions="End">
            <CheckBox.IsChecked>
                <MultiBinding Converter="{StaticResource AllTrueConverter}">
                    <Binding Path="Employee1.IsOver16" />
                    <Binding Path="Employee1.HasPassedTest" />
                    <Binding Path="Employee1.IsSuspended"
                             Converter="{StaticResource InverterConverter}" />
                </MultiBinding>
            </CheckBox.IsChecked>
        </CheckBox>

The result is a TargetInvocationException with an inner exception of NullReferenceException:

{System.NullReferenceException: Object reference not set to an instance of an object
at Xamarin.Forms.CheckBox.get_IsChecked () [0x00000] in D:\a\1\s\Xamarin.Forms.Core\CheckBox.cs:31
at Xamarin.Forms.CheckBox.ChangeVisualState () [0x00000] in D:\a\1\s\Xamarin.Forms.Core\CheckBox.cs:37
at Xamarin.Forms.CheckBox+<>c.<.cctor>b__33_0 (Xamarin.Forms.BindableObject bindable, System.Object oldValue, System.Object newValue) [0x00022] in D:\a\1\s\Xamarin.Forms.Core\CheckBox.cs:16
at Xamarin.Forms.BindableObject.SetValueActual (Xamarin.Forms.BindableProperty property, Xamarin.Forms.BindableObject+BindablePropertyContext context, System.Object value, System.Boolean currentlyApplying, Xamarin.Forms.Internals.SetValueFlags attributes, System.Boolean silent) [0x00120] in D:\a\1\s\Xamarin.Forms.Core\BindableObject.cs:463
at Xamarin.Forms.BindableObject.SetValueCore (Xamarin.Forms.BindableProperty property, System.Object value, Xamarin.Forms.Internals.SetValueFlags attributes, Xamarin.Forms.BindableObject+SetValuePrivateFlags privateAttributes) [0x00173] in D:\a\1\s\Xamarin.Forms.Core\BindableObject.cs:397
at Xamarin.Forms.MultiBinding.Apply (System.Object context, Xamarin.Forms.BindableObject targetObject, Xamarin.Forms.BindableProperty targetProperty, System.Boolean fromBindingContextChanged) [0x00162] in D:\a\1\s\Xamarin.Forms.Core\MultiBinding.cs:147
at Xamarin.Forms.BindableObject.SetBinding (Xamarin.Forms.BindableProperty targetProperty, Xamarin.Forms.BindingBase binding, System.Boolean fromStyle) [0x0008a] in D:\a\1\s\Xamarin.Forms.Core\BindableObject.cs:181
at Xamarin.Forms.BindableObject.SetBinding (Xamarin.Forms.BindableProperty targetProperty, Xamarin.Forms.BindingBase binding) [0x00000] in D:\a\1\s\Xamarin.Forms.Core\BindableObject.cs:157
at DataBindingDemos.MultiBindingConverterPage.InitializeComponent () [0x00012] in /Users/davidbritch/repos/xamarin-forms-samples/DataBindingDemos/DataBindingDemos/DataBindingDemos/obj/Debug/netstandard2.0/Views/MultiBindingConverterPage.xaml.g.cs:22
at DataBindingDemos.MultiBindingConverterPage..ctor () [0x00008] in /Users/davidbritch/repos/xamarin-forms-samples/DataBindingDemos/DataBindingDemos/DataBindingDemos/Views/MultiBindingConverterPage.xaml.cs:9
at (wrapper managed-to-native) System.Reflection.RuntimeConstructorInfo.InternalInvoke(System.Reflection.RuntimeConstructorInfo,object,object[],System.Exception&)
at System.Reflection.RuntimeConstructorInfo.InternalInvoke (System.Object obj, System.Object[] parameters, System.Boolean wrapExceptions) [0x00005] in /Library/Frameworks/Xamarin.iOS.framework/Versions/Current/src/Xamarin.iOS/mcs/class/corlib/System.Reflection/RuntimeMethodInfo.cs:936 }

Steps to Reproduce

  1. Run the attached sample.
  2. Browse to the "Multi Value Converters" page.

Expected Behavior

A page of UI displays.

Actual Behavior

TargetInvocationException with an inner exception of NullReferenceException.

Basic Information

  • Version with issue: 4.7-pre4
  • Last known good version: 4.7-pre1
  • IDE: VSMac 8.6.3
  • Platform Target Frameworks:
    • iOS: 13.5
    • Android: API 28

Reproduction Link

DataBindingDemos.zip

@davidbritch davidbritch added t/bug 🐛 s/unverified New report that has yet to be verified labels Jun 15, 2020
@pauldipietro pauldipietro added this to New in Triage Jun 15, 2020
@davidbritch
Copy link
Author

Not sure if it's related, but given the following code I'd expect the Convert method of StringConcatConverter to get called once. Instead it's getting called 7 times.

        <Label>
            <Label.Text>
                <MultiBinding FallbackValue="Name unavailable"
                              TargetNullValue="Data undefined"
                              Converter="{StaticResource StringConcatConverter}">
                    <Binding Path="Employee1.Forename" />
                    <Binding Path="Employee1.MiddleName" />
                    <Binding Path="Employee1.Surname" />
                </MultiBinding>
            </Label.Text>
        </Label>

It's the 5th call to Convert before the values array is fully populated.

@StephaneDelcroix
Copy link
Member

given the following code the AllTrueConverter should receive an array of 3 items of data. Instead, the converter is receiving an array of 3 null items.

when the binding context is null (the first time the binding is applied), each binding is resolved to null, hence the null array.

Instead it's getting called 7 times.

I know that the converter is invoked a few extra time, but that's room for enhancement, not a real bug. The expectation of a single convert is too low. At the very best, if the bindings are synchronised (which they aren't), it should be at least 2 (once with a null array, once with the array of populated values).

I'll look at providing a perf fix in the coming weeks

@StephaneDelcroix
Copy link
Member

StephaneDelcroix commented Jun 15, 2020

@davidbritch I think I've finished this investigation:

  • your Converter doesn't throw 👍
  • your Converter handle null or invalid values by returning UnsetValue 👍
  • your Converter has a very nice comment, for documentation purpose, saying
              // Return UnsetValue to use the binding FallbackValue
                return BindableProperty.UnsetValue;
  • but your MultiBinding
                <MultiBinding Converter="{StaticResource AllTrueConverter}">
                    <Binding Path="Employee1.IsOver16" />
                    <Binding Path="Employee1.HasPassedTest" />
                    <Binding Path="Employee1.IsSuspended"
                             Converter="{StaticResource InverterConverter}" />
                </MultiBinding>

doesn't set the FallBackValue 👎

adding FallbackValue="false" should fix it (but doesn't not, because there's another hidden bug that doesn't convert the string literal to bool value)

@StephaneDelcroix
Copy link
Member

StephaneDelcroix commented Jun 15, 2020

FallbackValue is unit-tested... in code... so there's no need to convert the string literal :(

@StephaneDelcroix
Copy link
Member

and that hidden issue is now fixed

@samhouts samhouts added a/binding ⛓ i/high Completely doesn't work, crashes, or is unusably slow, has no obvious workaround; occurs less often in-progress This issue has an associated pull request that may resolve it! labels Jun 15, 2020
@StephaneDelcroix StephaneDelcroix moved this from New to Ready For Work in Triage Jun 17, 2020
Triage automation moved this from Ready For Work to Closed Jun 17, 2020
@samhouts samhouts added this to In Progress in 4.7.0 Jun 20, 2020
@samhouts samhouts moved this from In Progress to Done in 4.7.0 Jun 20, 2020
@samhouts samhouts removed this from Closed in Triage Jul 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a/binding ⛓ i/high Completely doesn't work, crashes, or is unusably slow, has no obvious workaround; occurs less often in-progress This issue has an associated pull request that may resolve it! s/unverified New report that has yet to be verified t/bug 🐛
Projects
No open projects
4.7.0
  
Done
Development

No branches or pull requests

3 participants