Repository: arkivanov/Decompose (kotlin)

Issue:
[StackNavigation] InstanceKeeper instances of component are destroyed earlier than component itself, leading to access to released resources.
Hello. I found that in `StackNavigation` `Instance`s of component are destroyed first, and then lifecycle of component is going pause-stop-destroy. I found that when implemented pause of video play on pause of component (which IMO is good as video is not playing in background) and also "releasing" (i.e. destroying with release of resources like threads etc) ExoPlayer in `onDestroy` method of `Instance`.

So, when I navigate back from screen with video I see this warning in logs:

```
Handler (android.os.Handler) {c3b8656} sending message to a Handler on a dead thread
java.lang.IllegalStateException: Handler (android.os.Handler) {c3b8656} sending message to a Handler on a dead thread
```

That is a clear signal that Instance::onDestroy is called first, and then Lifecycle.Callbacks::onPause is called (because stack trace leads to onPause).

I also found the source of this exception in Decompose code. Lines 189 and 190 are in wrong order and that is the problem. I think reproductions steps are not needed as the error is clear from code below:

https://github.com/arkivanov/Decompose/blob/3fbf7c8cee02f89aed15a86897ae0fdcff124cbd/decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildrenNavigator.kt#L181-L193

Why this should not happen: because instances of classes should be destroyed only when there are no external links to them, i.e. noone can access already destroyed classes or releases resources. On android, ViewModel is destroyed later than Activity, for example.

Btw, I use 2.0.0-alpha-01 version, and tried with 2.0.0-alpha-02 - the error is still here. Artifacts used: `decompose` and `extensions-compose-jetpack` on Android 13 (Pixel Experience).

Pull request:
Fix #383

Patch:
diff --git a/decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildrenNavigator.kt b/decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildrenNavigator.kt
index 231d47c09..2dad11de1 100644
--- a/decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildrenNavigator.kt
+++ b/decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildrenNavigator.kt
@@ -2,7 +2,6 @@ package com.arkivanov.decompose.router.children
 
 import com.arkivanov.decompose.Child
 import com.arkivanov.decompose.router.children.ChildItem.Created
-import com.arkivanov.decompose.router.children.ChildItem.Created.*
 import com.arkivanov.decompose.router.children.ChildNavState.Status
 import com.arkivanov.essenty.instancekeeper.InstanceKeeper
 import com.arkivanov.essenty.lifecycle.Lifecycle
@@ -185,9 +184,7 @@ internal class ChildrenNavigator<out C : Any, out T : Any, N : NavState<C>>(
         for (item in oldItems) {
             val child = item as? Created ?: continue
             if (item.configuration !in newConfigurations) {
-                child.backHandler.stop()
-                child.instanceKeeperDispatcher.destroy()
-                child.lifecycleRegistry.destroy()
+                child.destroy()
             }
         }
     }
@@ -203,10 +200,7 @@ internal class ChildrenNavigator<out C : Any, out T : Any, N : NavState<C>>(
                         when (status) {
                             Status.DESTROYED -> {
                                 val savedState = item.stateKeeperDispatcher.save()
-                                item.backHandler.stop()
-                                item.instanceKeeperDispatcher.destroy()
-                                item.lifecycleRegistry.destroy()
-
+                                item.destroy()
                                 ChildItem.Destroyed(configuration = item.configuration, savedState = savedState)
                             }
 
@@ -241,6 +235,12 @@ internal class ChildrenNavigator<out C : Any, out T : Any, N : NavState<C>>(
         }
     }
 
+    private fun Created<*, *>.destroy() {
+        backHandler.stop()
+        lifecycleRegistry.destroy()
+        instanceKeeperDispatcher.destroy()
+    }
+
     private class RetainedInstance<C : Any, T : Any> : InstanceKeeper.Instance {
         val items: MutableList<Created<C, T>> = ArrayList()
 


Test patch:
diff --git a/decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/TestInstance.kt b/decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/TestInstance.kt
index a22ea9f3e..1db8722d1 100644
--- a/decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/TestInstance.kt
+++ b/decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/TestInstance.kt
@@ -2,11 +2,14 @@ package com.arkivanov.decompose.router
 
 import com.arkivanov.essenty.instancekeeper.InstanceKeeper
 
-class TestInstance : InstanceKeeper.Instance {
+class TestInstance(
+    var onDestroyed: () -> Unit = {},
+) : InstanceKeeper.Instance {
 
     var isDestroyed: Boolean = false
 
     override fun onDestroy() {
         isDestroyed = true
+        onDestroyed()
     }
 }
diff --git a/decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/children/ChildrenBasicTest.kt b/decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/children/ChildrenBasicTest.kt
index 189c0a07c..acdda8e15 100644
--- a/decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/children/ChildrenBasicTest.kt
+++ b/decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/children/ChildrenBasicTest.kt
@@ -1,10 +1,13 @@
 package com.arkivanov.decompose.router.children
 
 import com.arkivanov.decompose.DefaultComponentContext
+import com.arkivanov.decompose.router.TestInstance
 import com.arkivanov.decompose.router.children.ChildNavState.Status.ACTIVE
 import com.arkivanov.decompose.router.children.ChildNavState.Status.DESTROYED
 import com.arkivanov.decompose.router.children.ChildNavState.Status.INACTIVE
 import com.arkivanov.decompose.statekeeper.TestStateKeeperDispatcher
+import com.arkivanov.decompose.value.getValue
+import com.arkivanov.essenty.instancekeeper.getOrCreate
 import com.arkivanov.essenty.lifecycle.destroy
 import com.arkivanov.essenty.lifecycle.doOnDestroy
 import kotlin.test.Test
@@ -111,4 +114,32 @@ internal class ChildrenBasicTest : ChildrenTestBase() {
 
         assertContentEquals(listOf(1, 2, 3), createEvents)
     }
+
+    @Test
+    fun GIVEN_active_child_with_retained_instance_WHEN_child_removed_THEN_component_destroyed_before_instance() {
+        val destroyEvents = ArrayList<String>()
+        val children by context.children(initialState = stateOf(1 by INACTIVE, 2 by INACTIVE, 3 by ACTIVE))
+        val component = children.getById(id = 3).requireInstance()
+        val instance = component.instanceKeeper.getOrCreate(key = "key", factory = ::TestInstance)
+        component.lifecycle.doOnDestroy { destroyEvents += "component" }
+        instance.onDestroyed = { destroyEvents += "instance" }
+
+        navigate { listOf(1 by DESTROYED, 2 by INACTIVE) }
+
+        assertContentEquals(listOf("component", "instance"), destroyEvents)
+    }
+
+    @Test
+    fun GIVEN_active_child_with_retained_instance_WHEN_child_switched_to_destroyed_THEN_component_destroyed_before_instance() {
+        val destroyEvents = ArrayList<String>()
+        val children by context.children(initialState = stateOf(1 by INACTIVE, 2 by INACTIVE, 3 by ACTIVE))
+        val component = children.getById(id = 3).requireInstance()
+        val instance = component.instanceKeeper.getOrCreate(key = "key", factory = ::TestInstance)
+        component.lifecycle.doOnDestroy { destroyEvents += "component" }
+        instance.onDestroyed = { destroyEvents += "instance" }
+
+        navigate { listOf(1 by DESTROYED, 2 by INACTIVE, 3 by DESTROYED) }
+
+        assertContentEquals(listOf("component", "instance"), destroyEvents)
+    }
 }