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

ReactiveUI binding crash Xamarin.Android #1129

Closed
RLittlesII opened this issue Jan 16, 2020 · 21 comments
Closed

ReactiveUI binding crash Xamarin.Android #1129

RLittlesII opened this issue Jan 16, 2020 · 21 comments

Comments

@RLittlesII
Copy link

Bug

Despite our best efforts, bugs can slip into releases or corner cases forgotten about. We will try our best to remedy the situation
and/or provide workarounds. Note that certain (odd) behaviors are by design and as such are not considered bugs.

Which library version?

Was reported against 4.1.6, still present in 4.3.2

What are the platform(s), environment(s) and related component version(s)?

Xamarin.Android

What is the use case or problem?

In ReacitveUI we use Reflection and Tuples to provide Two Way Binding to UI elements. It seems that the reflection spits a specific error now when it tries to bind on Xamarin.Android. The issue was reported after we migrated to Value Tuples.

This is related to reactiveui/ReactiveUI#2170

What is the expected outcome?

That the bindings are found and Android handles the Two Way binding.

What is the actual outcome?

The following error is produced. The Android App crashes.

What is the stacktrace of the exception(s) if any?

https://gist.github.com/RLittlesII/13c15d8047ae906aee0ad585c097f0f2

Do you have a code snippet or project that reproduces the problem?

reactiveui.2170.zip

@glennawatson
Copy link

This error seems to be related to android 9.0. It doesn't seem to happen with 8.1.

@clintonrocksmith-skedulo

We also have experienced this error and have had to refactor our code to use non-reactive approaches with Native Android Xamarin/Mono using .NetStandard. It fails at runtime and caused us delays in our release cycles.

@clintonrocksmith-skedulo

image

I've also created a Xamarin project solution that demonstrates the bug

https://github.com/clintonrocksmith/RxCrash

@glennawatson
Copy link

It also appears the ValueTuple is a red herring and it was the upgrade to Andoid 9 for rxui that caused the binding to break.

@KevinvdBurg
Copy link

KevinvdBurg commented Jan 17, 2020

@glennawatson I have tested it on Android 8, 8.1 and 9 and every version has this issue.

@akarnokd
Copy link
Collaborator

I'm not experienced with .NET at this depth. Could you create a test case that fails on a desktop runtime?

@glennawatson
Copy link

@akarnokd Only happens on xamarin android runtime. Works fine on other platforms

@danielcweber
Copy link
Collaborator

It seems there's a creation of an observable from an event pattern, and the event handler delegate doesn't match the actual method signature of the method to wrap.

If the method to create a delegate from is EditText.Text and ReactiveUI tries to get the right method from the x => x.Text expression, a good guess is that it just got the wrong method. But from what I see there haven't been any changes in signature or any additional signatures in Android 9.

Anyway, if ReactiveUI finds the wrong delegate for a given property expression, it's more probably due to a change in the Xamarin bindings vs. a change in Android. But there doesn't seem to be a change either.

@RLittlesII
Copy link
Author

@danielcweber That's as far as I've been able to gather. I will see if I can get closer to which method it is attempting to use.

@gerardvanderkruijs
Copy link

Any update on this issue?

@Lapinou42
Copy link

Lapinou42 commented Mar 5, 2020

I got the same issue here on Xamarin.Android.

  • Android target SDK: 10.0
  • VS for Mac : up to date
  • ReactiveUI: up to date

I tried to debug and I the exception is thrown here:

Capture d’écran 2020-03-05 à 09 48 01

If I checked this two var:

ParameterInfo[] delargs = invoke.GetParametersInternal ();
ParameterInfo[] args = method.GetParametersInternal ();

delargs has 2 values and args has 1 value.

Then, if we check the condition:

if (target != null)
{
    // delegate closed over target
    if (!method.IsStatic)
        // target is passed as this
        argLengthMatch = (args.Length == delargs.Length);
    else
        // target is passed as the first argument to the static method
        argLengthMatch = (args.Length == delargs.Length + 1);
}
else
{
    if (!method.IsStatic)
    {
        //
        // Net 2.0 feature. The first argument of the delegate is passed
        // as the 'this' argument to the method.
        //
        argLengthMatch = (args.Length + 1 == delargs.Length);

        if (!argLengthMatch)
            // closed over a null reference
            argLengthMatch = (args.Length == delargs.Length);
    }
    else
    {
        argLengthMatch = (args.Length == delargs.Length);

        if (!argLengthMatch)
            // closed over a null reference
            argLengthMatch = args.Length == delargs.Length + 1;
    }
}

=> target is not null
=> the method is not static.
So it goes to the condition:

// target is passed as this
argLengthMatch = (args.Length == delargs.Length);

So argLenghtMatch would be false and exception will be throw.

But I see few lines down this comment:

 //
// Net 2.0 feature. The first argument of the delegate is passed
// as the 'this' argument to the method.
//
argLengthMatch = (args.Length + 1 == delargs.Length);

If this code was executed, argLengthMatch will be true and no exception will be throw.

What about a code that seems like this, is this a possible solution ?

if (target != null)
{
    // delegate closed over target
    if (!method.IsStatic)
        // target is passed as this
        //
        // Net 2.0 feature. The first argument of the delegate is passed
        // as the 'this' argument to the method.
        //
        argLengthMatch = (args.Length + 1 == delargs.Length);

        if (!argLengthMatch)
            // closed over a null reference
            argLengthMatch = (args.Length == delargs.Length);
    else
        // target is passed as the first argument to the static method
        argLengthMatch = (args.Length == delargs.Length + 1);
}
else
{
    if (!method.IsStatic)
    {
        //
        // Net 2.0 feature. The first argument of the delegate is passed
        // as the 'this' argument to the method.
        //
        argLengthMatch = (args.Length + 1 == delargs.Length);

        if (!argLengthMatch)
            // closed over a null reference
            argLengthMatch = (args.Length == delargs.Length);
    }
    else
    {
        argLengthMatch = (args.Length == delargs.Length);

        if (!argLengthMatch)
            // closed over a null reference
            argLengthMatch = args.Length == delargs.Length + 1;
    }
}

Is this an issue of Xamarin teams ? (File: System.Delegate.cs)

@lwschan
Copy link

lwschan commented Mar 18, 2020

No reply from anyone? :(

@lwschan
Copy link

lwschan commented Mar 31, 2020

Do we have someone from Rx.NET or ReactiveUI looking into this? :(

@Lapinou42
Copy link

Lapinou42 commented Mar 31, 2020

I don't know. For now, to use the latest version of ReactiveUI on iOS and Android, I use it in this way on Android:

For example, for this (that crash):

this.Bind(ViewModel, vm => vm.FirstName, view => view._firstname.EditText.Text)
    .DisposeWith(disposeBag);

I changed it to:

this.WhenAnyValue(a => a.ViewModel.FirstName)
                    .Take(1) //Only the first one
                    .ObserveOn(RxApp.MainThreadScheduler)
                    .Subscribe(text =>
                    {
                        _firstname.EditText.Text = text;
                    })
                    .DisposeWith(disposeBag);

And for the "binding" Editext => ViewModel property, I add this in onCreate function:

_firstname.EditText.TextChanged += EditText_TextChanged;

and this in onDestroy function:

_firstname.EditText.TextChanged -= EditText_TextChanged;

For you information, here the function "EditText_TextChanged':

private void EditText_TextChanged(object sender, Android.Text.TextChangedEventArgs e)
        {
            var text = e.Text.ToString();

            if (sender.Equals(_firstname.EditText))
            {
                ViewModel.FirstName = text;
            }
            else if (sender.Equals(_lastname.EditText))
            {
                ViewModel.Lastname = text;
            }
            else
            {
                //should never be here
            }
        }

OR

If you don't have to use the two way binding (Bind), using the OneWayBind doesn't crash:

this.OneWayBind(ViewModel, vm => vm.FirstName, view => view._firstname.EditText.Text)
    .DisposeWith(disposeBag);

@lwschan
Copy link

lwschan commented Apr 4, 2020

That will be very costly for us to do, as the size of the project is too big now. :(
The only thing we can do is wait..............

@RLittlesII
Copy link
Author

@Lapinou42

Is the CreateDelegate method you posted in the Xamarin code? Can you reference where it is found?

We discussed that there could be something outside of ReactiveUI and/or Reactive Extensions that might have caused this bug. The upgrade of ReactiveUI also required an upgrade of a lot of dependencies, so isolation was difficult.

@LewisGauss I apologize for the lack of activity on this issue, things have been hectic the last few months for me. While I understand the frustration of having to wait, we have not been able to pinpoint any change in Reactive Extensions or ReactiveUI that would have caused this problem.

@Lapinou42
Copy link

@RLittlesII I'll check that and let know ASAP ;)

@lwschan
Copy link

lwschan commented Apr 25, 2020

Hi @RLittlesII & @Lapinou42

Would this help? According to the stack trace, it's at System.Reactive.Reflection.Utils.CreateDelegate, then it crashes at System.Delegate.CreateDelegate
image

@RLittlesII
Copy link
Author

@glennawatson was able to resolve this issue. via reactiveui/ReactiveUI#2390

Closing this issue.

@gsgou
Copy link

gsgou commented Apr 25, 2020

@glennawatson @RLittlesII great job, tks a lot for this tricky one!

@lwschan
Copy link

lwschan commented Apr 25, 2020

Fantastic, I just updated, and this bug is missing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants