[BUG] Router.NavigateBack is still broken in some cases

[garyng]

Describe the bug

This is related to #2286 and #2315, with some more test cases

Steps To Reproduce

Sample: Test.zip

The sample is based on the one provided by #2286

There are three cases (all can be found inside ItemViewModel):

1. NavigateBack inside Subscribe

   var completedInternally = Observable.Amb(
       Confirm.Select(_ => _),
       Cancel.Select(_ => _),
       Delete.Select(_ => _));
   
   completedInternally.Subscribe(_ =>
   {
       if (HostScreen.Router.NavigationStack.LastOrDefault() == this)
       {
           HostScreen.Router.NavigateBack.Execute().Subscribe(); 
           if (NextViewModel != null)
               HostScreen.Router.Navigate.Execute(NextViewModel).Subscribe();
       }
   });

2. NavigateBack inside SelectMany

   var completedInternally = Observable.Amb(
           Confirm.Select(_ => _),
           Cancel.Select(_ => _),
           Delete.Select(_ => _))
       .Where(_ => HostScreen.Router.NavigationStack.LastOrDefault() == this)
       .SelectMany(_ => HostScreen.Router.NavigateBack.Execute());
   completedInternally.Subscribe(_ =>
   {
       if (NextViewModel != null)
           HostScreen.Router.Navigate.Execute(NextViewModel).Subscribe();
   });

3. NavigateBack inside Do

   var completedInternally = Observable.Amb(
           Confirm.Select(_ => _),
           Cancel.Select(_ => _),
           Delete.Select(_ => _))
       .Where(_ => HostScreen.Router.NavigationStack.LastOrDefault() == this)
       .Do(_ => HostScreen.Router.NavigateBack.Execute().Subscribe());
   completedInternally.Subscribe(_ =>
   {
       if (NextViewModel != null)
           HostScreen.Router.Navigate.Execute(NextViewModel).Subscribe();
   });

Case	RxUI 11.1.11	RxUI 11.1.6
1	✔	❌
2	❌	❌
3	✔	❌

Expected behavior

It should return to the Main Page when Save is tapped on the Production page.

Environment

• OS: Android 7-API 24, physical device
• Version 11.1.11 and 11.1.6
• Device: Samsung S6

Additional context

I'm quite new to ReactiveUI (using this for my uni's final year project), so it might the case that I am using it all wrong.

Thanks!

[open-collective-bot]

Hey @garyng 👋,

Thank you for opening an issue. We will get back to you as soon as we can. Also, check out our Open Collective and consider contributing financially.

https://opencollective.com/reactiveui

PS.: We offer priority support for all financial contributors. Don't forget to add priority label once you start contributing 😄

ReactiveUI - Open Collective

An advanced, composable, functional reactive model-view-viewmodel framework for all .NET platforms!

[garyng]

After some investigation, seems like

ReactiveUI/src/ReactiveUI.XamForms/RoutedViewHost.cs

Line 97 in ab3feed

((IViewFor)CurrentPage).ViewModel = Router.GetCurrentViewModel();

is throwing invalid cast exception randomly, which caused the observable pipeline to fail - that's why navigate back won't work in some cases

{System.InvalidCastException: Specified cast is not valid.
  at (wrapper castclass) System.Object.__castclass_with_cache(object,intptr,intptr)
  at ReactiveUI.XamForms.ReactiveContentPage`1[TViewModel].ReactiveUI.IViewFor.set_ViewModel (System.Object value) [0x00000] in <7e4db093e10040f994f24eaae639ebd2>:0 
  at Patronus.Ui.Forms.RoutedViewHostEx+<>c__DisplayClass1_0.<.ctor>b__13 (<>f__AnonymousType0`2[<Delta>j__TPar,<Current>j__TPar] x) [0x001f7] in F:\**\RoutedViewHostEx.cs:130 }

[garyng]

Using the sample above, after clicking the checkout item on the main page:

It's trying to assign ProductionItemViewModel to MainPage.ViewModel of type MainViewModel - which cause the invalid cast exception to occur. The Router's NavigationStack is also out of sync with NavigationPage's Pages...

---

Update:
Seems like this is a race condition:
between this (from the sample, A):

completedInternally.Subscribe(_ =>
{
	if (HostScreen.Router.NavigationStack.LastOrDefault() == this)
	{
		HostScreen.Router.NavigateBack.Execute().Subscribe();
		if (NextViewModel != null)
			HostScreen.Router.Navigate.Execute(NextViewModel).Subscribe();
	}
});

and B

ReactiveUI/src/ReactiveUI.XamForms/RoutedViewHost.cs

Lines 72 to 98 in ab3feed

	async x =>
	{
	// XF doesn't provide a means of navigating back more than one screen at a time apart from navigating right back to the root page
	// since we want as sensible an animation as possible, we pop to root if that makes sense. Otherwise, we pop each individual
	// screen until the delta is made up, animating only the last one
	var popToRoot = x.Current == 1;
	currentlyPopping = true;
	try
	{
	if (popToRoot)
	{
	await PopToRootAsync(true);
	}
	else if (!popToRootPending)
	{
	for (var i = 0; i < x.Delta; ++i)
	{
	await PopAsync(i == x.Delta - 1);
	}
	}
	}
	finally
	{
	currentlyPopping = false;
	((IViewFor)CurrentPage).ViewModel = Router.GetCurrentViewModel();
	}

NavigationStack: P, Q
NavigationPage's Pages: P, Q

1. A: HostScreen.Router.NavigateBack.Execute().Subscribe(); - P; P, Q
2. B: NavigationChanged fired - P; P, Q
3. B: await PopToRootAsync(true); - P; P
4. A: HostScreen.Router.Navigate.Execute(NextViewModel).Subscribe(); P, R; P
5. B: Router.GetCurrentViewModel() <- this returns view model for R
6. B: ((IViewFor)CurrentPage).ViewModel = Router.GetCurrentViewModel(); <- crash

Seems like

ReactiveUI/src/ReactiveUI.XamForms/RoutedViewHost.cs

Lines 104 to 126 in ab3feed

	d(this.WhenAnyObservable(x => x.Router.Navigate)
	.SelectMany(_ => PageForViewModel(Router.GetCurrentViewModel()))
	.SelectMany(async page =>
	{
	bool animated = true;
	var attribute = page.GetType().GetCustomAttribute<DisableAnimationAttribute>();
	if (attribute != null)
	{
	animated = false;
	}
	if (popToRootPending && Navigation.NavigationStack.Count > 0)
	{
	Navigation.InsertPageBefore(page, Navigation.NavigationStack[0]);
	await PopToRootAsync(animated);
	}
	else
	{
	await PushAsync(page, animated);
	}
	popToRootPending = false;
	return page;

didn't manage to fire before B assign the view model

[glennawatson]

Your in a different time zone than @RLittlesII so worth probably waiting for a bit.

Thanks for the updates.

[garyng]

Sorry for spamming! I'm using this library for my uni's project, and the deadline is next week (aka tomorrow)... I want to help in fixing this asap.

Sorry again!

[andrejpanjan]

I'm seeing the same issue in my app.

[RLittlesII]

I'll look at this again today. This was the point that I made to @glennawatson about the Routing on Xamarin.Forms needing an overhaul. I thought I stumbled on an easy fix. I guess not.

[garyng]

I think I fixed it... But I'm not sure...
forgive me for pasting it here because I have to submit my assignment later 😅

/// <summary>
/// This is a <see cref="NavigationPage"/> that serves as a router.
/// </summary>
/// <seealso cref="Xamarin.Forms.NavigationPage" />
/// <seealso cref="ReactiveUI.IActivatableView" />
public class RoutedViewHost : NavigationPage, IActivatableView, IEnableLogger
{
    /// <summary>
    /// The router bindable property.
    /// </summary>
    public static readonly BindableProperty RouterProperty = BindableProperty.Create(
        nameof(Router),
        typeof(RoutingState),
        typeof(RoutedViewHost),
        default(RoutingState),
        BindingMode.OneWay);

    /// <summary>
    /// Initializes a new instance of the <see cref="Exception"/> class.
    /// </summary>
    /// <exception cref="RoutedViewHost">You *must* register an IScreen class representing your App's main Screen.</exception>
    public RoutedViewHost()
    {
        this.WhenActivated(d =>
        {
            bool currentlyPopping = false;
            bool popToRootPending = false;
            bool userInstigated = false;

            Router.NavigationChanged
                .Where(_ => Router.NavigationStack.Count == 0)
                .Select(x =>
                {
                    // Xamarin Forms does not let us completely clear down the navigation stack
                    // instead, we have to delay this request momentarily until we receive the new root view
                    // then, we can insert the new root view first, and then pop to it
                    popToRootPending = true;
                    return x;
                })
                .Subscribe()
                .DisposeWith(d);

            Router.NavigationChanged
                .CountChanged()
                .Select(_ => Router.NavigationStack.Count)
                .StartWith(Router.NavigationStack.Count)
                .Buffer(2, 1)
                .Select(counts => new
                {
                    Delta = counts[0] - counts[1],
                    Previous = counts[0],
                    Current = counts[1], 
                    // cache current viewmodel as it might change if some other Navigation command is executed midway
                    CurrentViewModel = Router.GetCurrentViewModel()
                })
                .Where(_ => !userInstigated)
                .Where(x => x.Delta > 0)
                .SelectMany(
                    async x =>
                    {
                        // XF doesn't provide a means of navigating back more than one screen at a time apart from navigating right back to the root page
                        // since we want as sensible an animation as possible, we pop to root if that makes sense. Otherwise, we pop each individual
                        // screen until the delta is made up, animating only the last one
                        var popToRoot = x.Current == 1;
                        currentlyPopping = true;

                        try
                        {
                            if (popToRoot)
                            {
                                await PopToRootAsync(true);
                            }
                            else if (!popToRootPending)
                            {
                                for (var i = 0; i < x.Delta; ++i)
                                {
                                    await PopAsync(i == x.Delta - 1);
                                }
                            }
                        }
                        finally
                        {
                            currentlyPopping = false;
                            if (CurrentPage is IViewFor page && x.CurrentViewModel != null)
                            {
                                page.ViewModel = x.CurrentViewModel;
                            }
                        }

                        return Unit.Default;
                    })
                .Subscribe()
                .DisposeWith(d);


            Router.Navigate
                .SelectMany(_ => PageForViewModel(Router.GetCurrentViewModel()))
                .SelectMany(async page =>
                {
                    bool animated = true;
                    var attribute = page.GetType().GetCustomAttribute<DisableAnimationAttribute>();
                    if (attribute != null)
                    {
                        animated = false;
                    }

                    if (popToRootPending && Navigation.NavigationStack.Count > 0)
                    {
                        Navigation.InsertPageBefore(page, Navigation.NavigationStack[0]);
                        await PopToRootAsync(animated);
                    }
                    else
                    {
                        await PushAsync(page, animated);
                    }

                    popToRootPending = false;
                    return page;
                })
                .Subscribe()
                .DisposeWith(d);

            var poppingEvent = Observable.FromEvent<EventHandler<NavigationEventArgs>, Unit>(
                eventHandler =>
                {
                    void Handler(object sender, NavigationEventArgs e) => eventHandler(Unit.Default);
                    return Handler;
                },
                x => Popped += x,
                x => Popped -= x);

            // NB: Catch when the user hit back as opposed to the application
            // requesting Back via NavigateBack
            poppingEvent
                .Where(_ => !currentlyPopping && Router != null)
                .Subscribe(_ =>
                {
                    userInstigated = true;

                    try
                    {
                        Router.NavigationStack.RemoveAt(Router.NavigationStack.Count - 1);
                    }
                    finally
                    {
                        userInstigated = false;
                    }

                    var vm = Router.GetCurrentViewModel();
                    if (CurrentPage is IViewFor page && vm != null)
                    {
                        // don't replace view model if vm is null
                        page.ViewModel = vm;
                    }
                })
                .DisposeWith(d);
        });

        var screen = Locator.Current.GetService<IScreen>();
        if (screen == null)
        {
            throw new Exception("You *must* register an IScreen class representing your App's main Screen");
        }

        Router = screen.Router;

        this.WhenAnyValue(x => x.Router)
            .SelectMany(router =>
            {
                return router.NavigationStack.ToObservable()
                    .Select(x => (Page) ViewLocator.Current.ResolveView(x))
                    .SelectMany(x => PushAsync(x).ToObservable())
                    .Finally(() =>
                    {
                        var vm = router.GetCurrentViewModel();
                        if (vm == null)
                        {
                            return;
                        }

                        ((IViewFor) CurrentPage).ViewModel = vm;
                        CurrentPage.Title = vm.UrlPathSegment;
                    });
            })
            .Subscribe();
    }

    /// <summary>
    /// Gets or sets the <see cref="RoutingState"/> of the view model stack.
    /// </summary>
    public RoutingState Router
    {
        get => (RoutingState) GetValue(RouterProperty);
        set => SetValue(RouterProperty, value);
    }

    /// <summary>
    /// Pages for view model.
    /// </summary>
    /// <param name="vm">The vm.</param>
    /// <returns>An observable of the page associated to a <see cref="IRoutableViewModel"/>.</returns>
    [SuppressMessage("Design", "CA1822: Can be made static", Justification = "Might be used by implementors.")]
    protected IObservable<Page> PageForViewModel(IRoutableViewModel vm)
    {
        if (vm == null)
        {
            return Observable.Empty<Page>();
        }

        var ret = ViewLocator.Current.ResolveView(vm);
        if (ret == null)
        {
            var msg =
                $"Couldn't find a View for ViewModel. You probably need to register an IViewFor<{vm.GetType().Name}>";

            return Observable.Throw<Page>(new Exception(msg));
        }

        ret.ViewModel = vm;

        var pg = (Page) ret;
        pg.Title = vm.UrlPathSegment;

        return Observable.Return(pg);
    }
}

1. I used Buffer for a sliding window on NavigationStack count changes
2. The current view model is cached when there is a new count changes, instead of calling later. This is imo a workaround for the race condition, but I'm not sure how to fix it properly though
3. A few null checks on view model to prevent it from replacing the existing view model

[RLittlesII]

@garyng No apologies needed. I am digesting all the words and trying to find the sense in all this. Thank you for all the details. I will plug all this in and run some more tests. I am almost wondering if the previous change did anything, or just made it worst.

[garyng]

I think I found a solution:

Router.NavigationChanged
	.CountChanged()
	.Select(_ => Router.NavigationStack.Count)
	.StartWith(Router.NavigationStack.Count)
	.Buffer(2, 1)
	.Select(counts => new
	{
		Delta = counts[0] - counts[1],
		Previous = counts[0],
		Current = counts[1],
		// cache current viewmodel as it might change if some other Navigation command is executed midway
		CurrentViewModel = Router.GetCurrentViewModel()
	})
	.Where(_ => !userInstigated)
	.Where(x => x.Delta > 0)
*	.Select(
		async x =>
		{
			// XF doesn't provide a means of navigating back more than one screen at a time apart from navigating right back to the root page
			// since we want as sensible an animation as possible, we pop to root if that makes sense. Otherwise, we pop each individual
			// screen until the delta is made up, animating only the last one
			var popToRoot = x.Current == 1;
			currentlyPopping = true;

			try
			{
				if (popToRoot)
				{
					await PopToRootAsync(true);
				}
				else if (!popToRootPending)
				{
					for (var i = 0; i < x.Delta; ++i)
					{
						await PopAsync(i == x.Delta - 1);
					}
				}
			}
			finally
			{
				currentlyPopping = false;
				if (CurrentPage is IViewFor page && x.CurrentViewModel != null)
				{
					page.ViewModel = x.CurrentViewModel;
				}
			}

			return Unit.Default;
		})
*	.Concat()
	.Subscribe()
	.DisposeWith(d);

The Concat at the end will force the Task returned by Pop*Async to run synchronously - which probably can avoid the race condition. The original code from before 11.1.11 can probably be reused I think.

[andrejpanjan]

Any plans on fixing it soon?

[garyng]

@andrejpanjan Maybe you can try my solution above? I pasted it here:
https://gist.github.com/garyng/4f0a461eb7524b37b8a440d8bbff5a2a

I'm using this to replace the original RoutedViewHost, no problem so far -- but I can't guarantee.

Gist

Fix for ReactiveUI #2317

Fix for ReactiveUI #2317. GitHub Gist: instantly share code, notes, and snippets.

[andrejpanjan]

Thanks @garyng