fabulous-dev/Fabulous

NavigationPage crash

twop opened this issue · 14 comments

twop commented

Repro project

https://github.com/reigam/Fab2Demo

Repro branch in Fabulous

https://github.com/fsprojects/Fabulous/tree/wip-bug-nav-page-crash-repro
Note that it has a bunch of printfn statement because I couldn't attach a debugger

Steps

  1. Start the app (either iOS or android)
  2. Click "Go to page 2"
  3. Click "Close all"
  4. Click "Go to page 2" again on the fist page
  5. Clcik "Close all" again on the second page
  6. crash incoming!

My thoughts

It seems that there is a logical disconnect somewhere in nav page wrapper for Fabulous.

When there is an event "Popped" the update function clears up the navigation stack completely, and there are no pages in XF after "Clear All". Maybe that is a logical error that we should throw if there are no pages returned?

But also the "Popped" event can be triggered either from "Back Button", e.g. from user interaction in XF internals or via change in the stack property, thus it seems there is no definitive source of truth in here :(

twop commented

So if you comment out this piece of code from update function:

let update msg model =
    match msg with
    | FirstPageMsg m ->
        let l, g = FirstPage.update m model.FirstPage model.Global
        { model with FirstPage = l; Global = g }       
    | SecondPageMsg m ->
        let l, g = SecondPage.update m model.SecondPage model.Global
        { model with SecondPage = l; Global = g }       
    | ThirdPageMsg m ->
        let l, g = ThirdPage.update m model.ThirdPage model.Global
        { model with ThirdPage = l; Global = g }   
    | NavigationPopped ->
        let stash = Helpers.reshuffle model.Global.PageStash
        //{ model with Global = { PageStash = stash} } <-------
        model

Then everything works fine because Fabulous state and XF state are in sync.

BUT

If you use system controls to navigate instead of click "Clear All" then you get a different crash. For that you will need to click "Go to Page 2" to trigger

image

Error: System.IndexOutOfRangeException: Index was outside the bounds of the array.
  at Fabulous.XamarinForms.NavigationPageUpdaters.applyDiffNavigationPagePages (System.ValueTuple`2[T1,T2] prev, Fabulous.WidgetCollectionItemChanges diffs, Fabulous.IViewNode node) [0x002c1] in /Users/twop/work/Fabulous/src/Fabulous.XamarinForms/Views/Pages/NavigationPage.fs:55 
  at Fabulous.XamarinForms.NavigationPage+Pages@130.Invoke (System.ValueTuple`2[T1,T2] prev, Fabulous.WidgetCollectionItemChanges diffs, Fabulous.IViewNode node) [0x00000] in /Users/twop/work/Fabulous/src/Fabulous.XamarinForms/Views/Pages/NavigationPage.fs:130 
  at Fabulous.ViewNode.Fabulous.IViewNode.ApplyDiff (Fabulous.WidgetDiff& diff) [0x00202] in /Users/twop/work/Fabulous/src/Fabulous/ViewNode.fs:140 
  at Fabulous.XamarinForms.Application+MainPage@14-2.Invoke (Fabulous.WidgetDiff diff, Fabulous.IViewNode node) [0x00000] in /Users/twop/work/Fabulous/src/Fabulous.XamarinForms/Views/Application.fs:14 
  at Fabulous.ViewNode.Fabulous.IViewNode.ApplyDiff (Fabulous.WidgetDiff& diff) [0x000ec] in /Users/twop/work/Fabulous/src/Fabulous/ViewNode.fs:139 
  at Fabulous.Reconciler.update (Microsoft.FSharp.Core.FSharpFunc`2[T,TResult] canReuseView, Microsoft.FSharp.Core.FSharpValueOption`1[T] prevOpt, Fabulous.Widget next, Fabulous.IViewNode node) [0x000c3] in /Users/twop/work/Fabulous/src/Fabulous/Reconciler.fs:23 
  at Fabulous.ViewAdapters+OnStateChanged@185[model,msg,marker].Invoke (Microsoft.FSharp.Core.Unit unitVar0) [0x00000] in /Users/twop/work/Fabulous/src/Fabulous/Runners.fs:187
twop commented

tagging @reigam for visibility

twop commented

So it seems that the fundamental issue is that Popped or other events can come from multiple sources while Fabulous "thinks" that it owns the source of truth.

This problem is akin to React controlled components https://reactjs.org/docs/forms.html#controlled-components

But to my knowledge there is no way to tell XF to suppress events that are coming from Fabulous but let the ones coming directly from OS to go through.

Is there a way to identify these?

reigam commented

The same approach works perfectly fine in fabulous v1 (https://github.com/reigam/FabulousMultiNavPages), so wouldn't that suggest this NOT being a XF problem?

twop commented

to be clear: this is NOT XF problem but rather the way Fabulous v2 models interactions with it.

In V2 the diffing is done against virtual DOM nodes, aka Widget. Thus it takes actions against what it thinks is the "true" state, which in this case happens to be NOT the actual state of XF runtime.

Unless, there is something glaringly obvious I'm missing here.

twop commented

There is one potential workaround in user space:

When a page is popped it is being passed into the "Pop" event. So a developer can check if that page was already removed from the stack.

Like so:

In view function

.onPopped(fun (page: XF.Page) -> UserEventPopped page.Title )

And then in update

| UserEventPopped title -> model |> List.filter (fun page -> page.Title <> title)

The code above will work correctly in both cases (click on <- and via code).

There is one minor issue though, we don't expose event args in .onPopped yet.

 [<Extension>]
    static member inline onPopped(this: WidgetBuilder<'msg, #INavigationPage>, onPopped: 'msg) =
        this.AddScalar(NavigationPage.Popped.WithValue(fun _ -> box onPopped))

Thus, if we can't fully fix the issue (fingers crossed that we can), at least we can educate people how to work around it.

twop commented

I just verified the approach locally and it doesn't crash, although I'm not 100% sure if it works as intended

TimLariviere commented

But to my knowledge there is no way to tell XF to suppress events that are coming from Fabulous but let the ones coming directly from OS to go through.

Is there a way to identify these?

For this, I had to create Attributes.defineBindableWithEvent that handle both a property and its related event.

image

It allows us to unsubscribe to the event, set the new value, resubscribe to the event -- this helps filtering out programmatic events triggered by Fabulous.

TimLariviere commented

I'm wondering if we can force Fabulous to be the source of truth by preventing internal back navigation.

Xamarin.Forms has a PopRequested event (meant for internal use only according to MSDN) where it seems we can set a bool to prevent navigation.
https://github.dev/xamarin/Xamarin.Forms/blob/5.0.0/Xamarin.Forms.Core/NavigationPage.cs#L339

Tried it on iOS but seems it's called only after the navigation already occurred 🤔

I was thinking we can create a CustomNavigationPage inheriting from NavigationPage.
Override PopRequested to return false by default, and only when Fabulous tries to pop pages, we allow it.

TimLariviere commented

We can set to false but the platform pops anyway :|

TimLariviere commented

Random idea: NavigationPage already has a specific diffing logic because it's a stack and not a random access collection.
What if we create a CustomNavigationPage, listen for navigation events to keep track of back navigation, and in the custom diffing logic we check for that flag to know if we should trust Fabulous or XF?

Or better yet: compare the number of pages currently in NavigationPage and the number of pages inside the previous Widget.
If there is a disconnect, it means Xamarin.Forms updated on its own already and Fabulous needs to catch up

twop commented

It allows us to unsubscribe to the event, set the new value, resubscribe to the event -- this helps filtering out programmatic events triggered by Fabulous.

@TimLariviere I see async logic in here

navigationPage.PopAsync() |> ignore

I assume that's why the event is still fired

TimLariviere commented

Yeah, the async methods of NavigationPage have always been such a pain to use with Fabulous.
Before we used NavigationPage as the source of truth, and it was buggy as well: #158