Navigation screens seem to be duplicated

[mvarnagiris]

I'm trying out your library (0.5.0) and it's great. But I ran into an issue. I will post code here to reproduce it. I might be using the library wrong though.

class MainActivity : AppCompatActivity() {
    private val backPressHandler = BackPressHandler()

    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        setContent {
            MaterialTheme {
                Providers(AmbientBackPressHandler provides backPressHandler) {
                    MyRouter(startRoute = SplashRoute)
                }
            }
        }
    }

    override fun onBackPressed() {
        if (!backPressHandler.handle()) {
            super.onBackPressed()
        }
    }

    override fun onSaveInstanceState(outState: Bundle) {
        super.onSaveInstanceState(outState)
        outState.saveAmbient()
    }
}

sealed class MyRoute {
    object SplashRoute : MyRoute()
    object HomeRoute : MyRoute()
    object DetailsRoute : MyRoute()
}

@Composable
fun MyRouter(startRoute: MyRoute) {
    Router(contextId = "app", defaultRouting = startRoute) { backStack ->
        when (backStack.last()) {
            SplashRoute -> SplashRouteScreen(onInitialized = { backStack.newRoot(HomeRoute) })
            HomeRoute -> HomeRouteScreen(onShowDetails = { backStack.push(DetailsRoute) })
            DetailsRoute -> DetailsRouteScreen()
        }
    }
}

@Composable
fun SplashRouteScreen(onInitialized: () -> Unit) {
    onActive {
        onInitialized()
    }
}

@Composable
fun HomeRouteScreen(onShowDetails: () -> Unit) {
    onActive {
        Log.d("HomeRouteScreen", "onActive ${System.currentTimeMillis()}")

        onDispose {
            Log.d("HomeRouteScreen", "onDispose ${System.currentTimeMillis()}")
        }
    }

    Center {
        Button(onClick = onShowDetails) {
            Text(text = "Show details")
        }
    }
}

@Composable
fun DetailsRouteScreen() {
    Center {
        Text("Details")
    }
}

So if you open the app and you navigate through screens everything is fine. Logs in HomeRouteScreen work as intended. But if you leave the app with back button (don't kill the app), then:

1. Open app again
2. Tap "Show details"
3. Press back

Logs will look like this:

D/HomeRouteScreen: onActive 1582533598703
D/HomeRouteScreen: onDispose 1582533600692
D/HomeRouteScreen: onDispose 1582533600697
D/HomeRouteScreen: onActive 1582533602550
D/HomeRouteScreen: onActive 1582533602551

If you leave the app again, and repeat those steps:

HomeRouteScreen: onActive 1582533684493
HomeRouteScreen: onDispose 1582533699006
HomeRouteScreen: onDispose 1582533699009
HomeRouteScreen: onDispose 1582533699011
HomeRouteScreen: onActive 1582533699988
HomeRouteScreen: onActive 1582533699989
HomeRouteScreen: onActive 1582533699989

Seems like every time you close the app with back button then reopen, something gets duplicated.

[zsoltk]

Nice catch! Will investigate, thanks!

[zsoltk]

Findings so far:

• There's a backStackMap inside Router.kt which holds reference to BackStack instances
• Since BackStack is @Model, it seems it has references to Composables under the hood (it makes sense that it does), that's the source of the leak
• Removing backStackMap means it's lost with DKA
• Making it ambient again makes no difference, leaks the same
• Instead of storing BackStack, an option is to only store its elements (which are then not @Model instances) and restore BackStack from elements, but this has further problems:
  • If elements are immutable list, and list operations create new lists inside BackStack, then the held reference to list won't get updated, so upon restoring from backStackMap it will restore old state
  • If elements are mutable list, then @Model cannot track changes inside that, and will not trigger a recompose.

Options at this point that I don't really want to do:

• Force extending either Serializable or Parcelable on all sealed classes that Router can accept, so that they are persistable without app-level map directly to Bundle
• Drop support for persisting back stack

Optimally:

• Find a way to persist BackStack on app-level without references to Composables

Fallback:

• Maybe make separate packages, then client code can pick whether it wants transient / serializable / parcelable Router - I've had this idea earlier

[mvarnagiris]

What do you mean by DKA?

[zsoltk]

DKA = Don't Keep Activities

[mvarnagiris]

Ah OK thanks for clarification.

It seems to me that backStackMap is a global value, so technically there should be no need for storing anything in savedInstanceState? Although this would not help with the leak, just trying to understand why it's necessary now

[mvarnagiris]

Would it be possible to cache a non @Model class inside backStackMap and wrap it with a @Model class before passing it down to children?

[zsoltk]

Yes, now it's global. Original intention was to store it in Bundle, but 1. it means all sealed classes need to be either Serializable or Parcelable 2. Actual elements are lightweight objects so I saw no hurt in storing them global in a trade-off for simpler client code.

Bundle on the other hand is used for more than that. You have the BundleScope @Composable which allows client code to store and retrieve data from a scoped Bundle. This is demonstrated in the example app found in app-nested-containers module. There you have the "Local data: 0" labels which are actually clickable to increase the counter. When you leave a routing behind (parent level pushes something to back stack) but come back to it (pop), that data is restored from Bundle.

What you described as caching non-@Model class is what I tried to describe above with storing only the elements of BackStack where I wrote about why neither a mutable nor an immutable list would work unfortunately.

[mvarnagiris]

I just noticed something in those logs. I think the issue is not with the @Model class. Here's what happens:

1. Start app - onActive called
2. Press back to quit app - onDispose NOT called
3. Start app again - onActive called

Seems like this way Router gets leaked

[mvarnagiris]

Actually if you do this you will see same issue for onDispose not called

setContent {
    onActive {
        Log.d("ASD", "onActive")
    }
    onDispose {
        Log.d("ASD", "onDispose")
    }
}

[ShikaSD]

It is partially related to the model class, but you are right, I have checked it yesterday and noticed the same issue.

[mvarnagiris]

There's more discussion in Slack, but the workaround for now is to store a reference to Composition returned by setContent {} and then call Composition.dispose() inside onDestroy in Activity

[zsoltk]

Nice! Do you want to make a PR?

[mvarnagiris]

Well this problem cannot really be solved in Router and has to be solved in client code. Plus seems like Google team is aware and said it should be fixed with next release.

[zsoltk]

I meant there are 2 example apps and the main README could also make a mention of it.

[zsoltk]

I just thought to have your name on it since you found the solution. If you don't feel like it, I can do it too.

[mvarnagiris]

I cannot do it just now, so you go ahead and do it :) The solution was suggested on Slack #compose ;)

[mvarnagiris]

Closing this, because this bug is in Compose not in this library