Stop pretending everyone implements view state right.

Makes the two sites where we call `View.restoreHierarchyState` defensive, catching and logging thrown exceptions.

We normally prefer to let exceptions fly (https://en.wikipedia.org/wiki/Fail-fast_system), but `View.onRestoreInstanceState` is a particularly unforgiving method. Naive implementations throw if a view of one type attempts to restore state written by one of another type, which can happen if view ids are mismanaged. When these errors cause crashes in large apps, they are unlikely to be discovered before release and can be hellish to track down.

Crashing in such a case is an overreaction when the alternative, typically losing scroller positions or partially entered text values on rotation or when popping a back stack, is such a non-event.

Author: rjrjr

workflow-ui/core-android/src/androidTest/java/com/squareup/workflow1/ui/navigation/ViewStateCacheTest.kt

@@ -106,6 +106,38 @@ internal class ViewStateCacheTest {
     assertThat(firstViewRestored.viewState).isEqualTo("hello world")
   }
 
+  @Test fun doesnt_restore_child_states_on_error() {
+    val cache = ViewStateCache()
+    val firstRendering = NamedScreen(content = AScreen, name = "first")
+    val secondRendering = NamedScreen(content = AScreen, name = "second")
+    // Android requires ID to be set for view hierarchy to be saved or restored.
+    val firstView = createTestView(firstRendering, id = 1)
+    val secondView = createTestView(secondRendering)
+
+    // Set some state on the first view that will be saved.
+    firstView.viewState = "hello world"
+
+    // Show the first screen.
+    cache.update(retainedRenderings = emptyList(), oldHolderMaybe = null, newHolder = firstView)
+
+    // "Navigate" to the second screen, saving the first screen.
+    cache.update(
+      retainedRenderings = listOf(firstRendering),
+      oldHolderMaybe = firstView,
+      newHolder = secondView
+    )
+
+    // "Navigate" back to the first screen, restoring state.
+    val firstViewRestored = createTestView(firstRendering, id = 1, crashOnRestore = true)
+    firstViewRestored.viewState = "should not be clobbered"
+
+    cache.update(listOf(), oldHolderMaybe = secondView, newHolder = firstViewRestored)
+    // We didn't crash, that's already a good sign.
+
+    // Check that the "hard coded" view state is in tact due to the swallowed exception.
+    assertThat(firstViewRestored.viewState).isEqualTo("should not be clobbered")
+  }
+
   @Test fun doesnt_restore_state_when_restored_view_id_is_different() {
     val cache = ViewStateCache()
     val firstRendering = NamedScreen(content = AScreen, name = "first")
@@ -198,9 +230,10 @@ internal class ViewStateCacheTest {
 
   private fun createTestView(
     firstRendering: NamedScreen<*>,
-    id: Int? = null
+    id: Int? = null,
+    crashOnRestore: Boolean = false
   ): ScreenViewHolder<NamedScreen<*>> {
-    val view = ViewStateTestView(instrumentation.context).also { view ->
+    val view = ViewStateTestView(instrumentation.context, crashOnRestore).also { view ->
       id?.let { view.id = id }
       WorkflowLifecycleOwner.installOn(view, fakeOnBack)
     }

workflow-ui/core-android/src/androidTest/java/com/squareup/workflow1/ui/navigation/fixtures/ViewStateTestView.kt

@@ -13,7 +13,10 @@ import com.squareup.workflow1.ui.navigation.fixtures.BackStackContainerLifecycle
  * [onSaveInstanceState] and [onRestoreInstanceState] methods.
  */
 @OptIn(WorkflowUiExperimentalApi::class)
-internal class ViewStateTestView(context: Context) : LeafView<LeafRendering>(context) {
+internal class ViewStateTestView(
+  context: Context,
+  private val crashOnRestore: Boolean = false
+) : LeafView<LeafRendering>(context) {
 
   var viewState: String = ""
 
@@ -23,6 +26,7 @@ internal class ViewStateTestView(context: Context) : LeafView<LeafRendering>(con
   }
 
   override fun onRestoreInstanceState(state: Parcelable?) {
+    if (crashOnRestore) throw IllegalArgumentException("Crashing on restore.")
     (state as? SavedState)?.let {
       viewState = it.viewState
       super.onRestoreInstanceState(state.superState)
@@ -43,7 +47,10 @@ internal class ViewStateTestView(context: Context) : LeafView<LeafRendering>(con
       viewState = source.readString()!!
     }
 
-    override fun writeToParcel(out: Parcel, flags: Int) {
+    override fun writeToParcel(
+      out: Parcel,
+      flags: Int
+    ) {
       super.writeToParcel(out, flags)
       out.writeString(viewState)
     }

workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/WorkflowLayout.kt

@@ -6,6 +6,7 @@ import android.os.Parcel
 import android.os.Parcelable
 import android.os.Parcelable.Creator
 import android.util.AttributeSet
+import android.util.Log
 import android.util.SparseArray
 import android.widget.FrameLayout
 import androidx.lifecycle.Lifecycle
@@ -65,7 +66,11 @@ public class WorkflowLayout(
     showing.show(rootScreen, environment.withOnBackDispatcher())
     restoredChildState?.let { restoredState ->
       restoredChildState = null
-      showing.actual.restoreHierarchyState(restoredState)
+      try {
+        showing.actual.restoreHierarchyState(restoredState)
+      } catch (e: Exception) {
+        Log.w("Workflow", "WorkflowLayout failed to restore view state", e)
+      }
     }
   }
 

workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/navigation/ViewStateCache.kt

@@ -5,6 +5,7 @@ import android.os.Build.VERSION_CODES
 import android.os.Parcel
 import android.os.Parcelable
 import android.os.Parcelable.Creator
+import android.util.Log
 import android.util.SparseArray
 import android.view.View
 import androidx.annotation.VisibleForTesting
@@ -57,8 +58,8 @@ internal constructor(
 
   /**
    * @param retainedRenderings the renderings to be considered hidden after this update. Any
-   * associated view state will be retained in the cache, possibly to be restored to [newView]
-   * on a succeeding call to his method. Any other cached view state will be dropped.
+   * associated view state will be retained in the cache, possibly to be restored to the view
+   * of [newHolder] on a succeeding call to his method. Any other cached view state will be dropped.
    *
    * @param oldHolderMaybe the view that is being removed, if any, which is expected to be showing
    * a [NamedScreen] rendering. If that rendering is
@@ -90,7 +91,13 @@ internal constructor(
     stateRegistryAggregator.installChildRegistryOwnerOn(newHolder.view, newKey)
 
     viewStates.remove(newKey)
-      ?.let { newHolder.view.restoreHierarchyState(it.viewState) }
+      ?.let {
+        try {
+          newHolder.view.restoreHierarchyState(it.viewState)
+        } catch (e: Exception) {
+          Log.w("Workflow", "ViewStateCache failed to restore view state for $newKey", e)
+        }
+      }
 
     // Save both the view state and state registry of the view that's going away, as long as it's
     // still in the backstack.

workflow-ui/core-android/src/test/java/com/squareup/workflow1/ui/WorkflowLayoutTest.kt

@@ -5,10 +5,13 @@ import android.os.Bundle
 import android.os.Parcelable
 import android.util.SparseArray
 import android.view.View
+import androidx.activity.OnBackPressedDispatcher
+import androidx.activity.OnBackPressedDispatcherOwner
 import androidx.lifecycle.Lifecycle
 import androidx.lifecycle.testing.TestLifecycleOwner
 import androidx.test.core.app.ApplicationProvider
 import com.google.common.truth.Truth.assertThat
+import com.squareup.workflow1.ui.androidx.OnBackPressedDispatcherOwnerKey
 import com.squareup.workflow1.ui.navigation.WrappedScreen
 import kotlinx.coroutines.ExperimentalCoroutinesApi
 import kotlinx.coroutines.flow.flowOf
@@ -29,23 +32,49 @@ internal class WorkflowLayoutTest {
   private val workflowLayout = WorkflowLayout(context).apply { id = 42 }
 
   @Test fun ignoresAlienViewState() {
-    var saved = false
-    val weirdView = object : View(context) {
-      override fun onSaveInstanceState(): Parcelable {
-        saved = true
-        super.onSaveInstanceState()
-        return Bundle()
-      }
-    }.apply { id = 42 }
+    val weirdView = BundleSavingView(context)
 
     val viewState = SparseArray<Parcelable>()
     weirdView.saveHierarchyState(viewState)
-    assertThat(saved).isTrue()
+    assertThat(weirdView.saved).isTrue()
 
     workflowLayout.restoreHierarchyState(viewState)
     // No crash, no bug.
   }
 
+  @Test fun ignoresNestedViewStateMistakes() {
+    class AScreen : AndroidScreen<AScreen> {
+      override val viewFactory =
+        ScreenViewFactory.fromCode<AScreen> { _, initialEnvironment, context, _ ->
+          ScreenViewHolder(initialEnvironment, BundleSavingView(context)) { _, _ -> }
+        }
+    }
+
+    class BScreen : AndroidScreen<BScreen> {
+      override val viewFactory =
+        ScreenViewFactory.fromCode<BScreen> { _, initialEnvironment, context, _ ->
+          ScreenViewHolder(initialEnvironment, SaveStateSavingView(context)) { _, _ -> }
+        }
+    }
+
+    val onBack = object : OnBackPressedDispatcherOwner {
+      override fun getOnBackPressedDispatcher() = error("nope")
+      override val lifecycle: Lifecycle get() = error("also nope")
+    }
+
+    val env = ViewEnvironment.EMPTY + (OnBackPressedDispatcherOwnerKey to onBack)
+
+    val original = WorkflowLayout(context).apply { id = 1 }
+    original.show(AScreen(), env)
+
+    val viewState = SparseArray<Parcelable>()
+    original.saveHierarchyState(viewState)
+
+    val unoriginal = WorkflowLayout(context).apply { id = 1 }
+    unoriginal.restoreHierarchyState(viewState)
+    unoriginal.show(BScreen(), env)
+  }
+
   @Test fun usesLifecycleDispatcher() {
     val lifecycleDispatcher = UnconfinedTestDispatcher()
     val collectionContext: CoroutineContext = UnconfinedTestDispatcher()
@@ -62,4 +91,24 @@ internal class WorkflowLayoutTest {
 
     // No crash then we safely removed the dispatcher.
   }
+
+  private class BundleSavingView(context: Context) : View(context) {
+    var saved = false
+
+    init {
+      id = 42
+    }
+
+    override fun onSaveInstanceState(): Parcelable {
+      saved = true
+      super.onSaveInstanceState()
+      return Bundle()
+    }
+  }
+
+  private class SaveStateSavingView(context: Context) : View(context) {
+    init {
+      id = 42
+    }
+  }
 }