FragmentState writes 2 identical objects into Bundle using ViewModel with SavedStateHandle [253546214]

Fixed

Bug

Status Update

No update yet.

Description

yo...@luchegroup.com

created issue #1

Oct 14, 2022 06:45AM

Artifact used: androidx.fragment:fragment-ktx:1.5.3, androidx.lifecycle:lifecycle-viewmodel-ktx: 2.5.1, androidx.lifecycle:lifecycle-viewmodel-savedstate:2.5.1
Devices/Android versions reproduced on: Nothing A063 running Android 12

PoC project:

https://github.com/KeithYokoma/saved_state_poc

When saving the ViewModel state object using SavedStateHandle, the object is stored in the Bundle of the FragmentState.
The issue is that the Bundle contains 2 identical ViewModel state object with different keys(`androidx.lifecycle.internal.SavedStateHandlesProvider` and `androidx.lifecycle.BundlableSavedStateRegistry.key`), so it takes up twice as much as the ViewState object size. If the ViewState object size gets bigger, it can easily cause `TransactionTooLargeException` whereas the single ViewState size is small enough to fit in the Bundle limit.

as_screenshot_debugger_showing_identical_objects.png

1.4 MB

View

Download

Comments

il...@google.com <il...@google.com> #2Oct 18, 2022 05:07PM

Project: platform/frameworks/support
Branch: androidx-master-dev

commit c60f33e229e31ab328ef6b59dab63b264954831c
Author: Alexandre Elias <aelias@google.com>
Date: Fri Jul 10 16:23:09 2020

Semantics no-op cleanups

Partly in response to lmr's broad code review, I did a pass of
superficial API/implementation cleanup. The main changes are:

- I changed each Boolean SemanticsProperty where false is equivalent to
not being present to take "Unit" instead. This is conceptually
clearer: it avoids questions like "can I cancel out a semantics from a
merged child by setting it to false?" Because "property = Unit" looks
weird, I also changed the style of these to "property()".

- I moved the Semantics id generator closer to where it's used, in
SemanticsModifierCore. I made it internal and an AtomicInt.
(Note that integer ids are heavily used in the Android
AccessibilityNodeInfo APIs so I can't simply remove them entirely.)

- I deleted dead code. Some examples include SemanticsHintOverrides,
a public API not connected to anything, and SemanticsPropertyKey
merge() open method which is never called. (In both cases I have
a different plan in mind for accessibility.)

Fixes: 145951226
Fixes: 145955412
Test: existing tests
Relnote: "Single-value semantics properties now use a calling style.
For example, 'semantics { hidden = true }' is now written as:
'semantics { hidden() }'."

Change-Id: Ic1afd12ea22c926babc9662f1804d80b33aa0cfc

M ui/integration-tests/benchmark/src/androidTest/java/androidx/ui/benchmark/test/LayoutNodeModifierBenchmark.kt
M ui/ui-core/api/0.1.0-dev15.txt
M ui/ui-core/api/current.txt
M ui/ui-core/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-core/api/public_plus_experimental_current.txt
M ui/ui-core/api/restricted_0.1.0-dev15.txt
M ui/ui-core/api/restricted_current.txt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/graphics/vector/VectorTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/semantics/SemanticsTests.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidActuals.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeView.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeViewAccessibilityDelegateCompat.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidPopup.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/Expect.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsConfiguration.kt
D ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsHintOverrides.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsModifier.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsNode.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsOwner.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsWrapper.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/semantics/SemanticsProperties.kt
M ui/ui-foundation/api/0.1.0-dev15.txt
M ui/ui-foundation/api/current.txt
M ui/ui-foundation/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-foundation/api/public_plus_experimental_current.txt
M ui/ui-foundation/api/restricted_0.1.0-dev15.txt
M ui/ui-foundation/api/restricted_current.txt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Clickable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Dialog.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Scroller.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Selectable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Toggleable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/semantics/FoundationSemanticsProperties.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ButtonTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CardTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CheckboxScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/RadioButtonScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ScaffoldTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SnackbarTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SurfaceTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ripple/RippleIndicationTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/textfield/TextFieldScreenshotTest.kt
M ui/ui-material/src/main/java/androidx/ui/material/AppBar.kt
M ui/ui-material/src/main/java/androidx/ui/material/TextFieldImpl.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/AssertsTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/CallSemanticsActionTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ErrorMessagesTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/FindersTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/PrintToStringTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ScrollToTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/TextActionsTest.kt
M ui/ui-test/src/main/java/androidx/ui/test/Actions.kt
M ui/ui-test/src/main/java/androidx/ui/test/Filters.kt
M ui/ui-text/api/0.1.0-dev15.txt
M ui/ui-text/api/current.txt
M ui/ui-text/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-text/api/public_plus_experimental_current.txt
M ui/ui-text/api/restricted_0.1.0-dev15.txt
M ui/ui-text/api/restricted_current.txt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/CoreTextField.kt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/TextSemanticsProperties.kt

https://android-review.googlesource.com/1360099

yo...@luchegroup.com <yo...@luchegroup.com> #3Oct 20, 2022 07:40AM

Thank you for your information about the fragment library updates. I've tried Fragment 1.6.0-alpha03 and unfortunately, this issue is still there. Saved state bundle structure is very different but it still contains duplicate state objects.

Here's what I get when TransactionTooLargeException occurs with Fragment 1.6.0-alpha03. Apparently, there are 2 androidx.lifecycle.ViewModelProvider.DefaultKey:com.github.keithyokoma.poc.fatbundle.ui.home.HomeViewModel [size=268520] entries in the saved state Bundle according to the ActivityStopInfo log.

ActivityStopInfo W  Bundle stats:
ActivityStopInfo W    android:viewHierarchyState [size=6704]
ActivityStopInfo W      android:views [size=6656]
ActivityStopInfo W    androidx.lifecycle.BundlableSavedStateRegistry.key [size=546284]
ActivityStopInfo W      androidx.lifecycle.internal.SavedStateHandlesProvider [size=1092]
ActivityStopInfo W      android:support:activity-result [size=2252]
ActivityStopInfo W        KEY_COMPONENT_ACTIVITY_REGISTERED_KEYS [size=1456]
ActivityStopInfo W      android:support:fragments [size=542572]
ActivityStopInfo W        fragment_02795c46-b71a-4ebf-b4d0-4f3e11676843 [size=542080]
ActivityStopInfo W          childFragmentManager [size=539668]
ActivityStopInfo W            fragment_bcb533f9-fd30-4807-8182-2b3510e88075 [size=539176]
ActivityStopInfo W              viewRegistryState [size=269804]
ActivityStopInfo W                androidx.lifecycle.BundlableSavedStateRegistry.key [size=269680]
ActivityStopInfo W                  androidx.lifecycle.internal.SavedStateHandlesProvider [size=268720]
ActivityStopInfo W                    androidx.lifecycle.ViewModelProvider.DefaultKey:com.github.keithyokoma.poc.fatbundle.ui.home.HomeViewModel [size=268520]
ActivityStopInfo W                      values [size=268428]
ActivityStopInfo W              registryState [size=268972]
ActivityStopInfo W                androidx.lifecycle.BundlableSavedStateRegistry.key [size=268848]
ActivityStopInfo W                  androidx.lifecycle.internal.SavedStateHandlesProvider [size=268720]
ActivityStopInfo W                    androidx.lifecycle.ViewModelProvider.DefaultKey:com.github.keithyokoma.poc.fatbundle.ui.home.HomeViewModel [size=268520]
ActivityStopInfo W                      values [size=268428]
ActivityStopInfo W          savedInstanceState [size=1284]

il...@google.com <il...@google.com> #4Oct 20, 2022 10:46PM

Assigned to jb...@google.com.

Thanks, that new structure makes it a lot more obvious about what is going on and confirms that there is something going wrong on our end. We'll take a look.

ap...@google.com <ap...@google.com> #5Nov 10, 2022 01:19AM

Project: platform/frameworks/support
Branch: androidx-main

commit 345d09ca4e1908954e8d38979cf9a63d0f030b1d
Author: Jeremy Woods <jbwoods@google.com>
Date: Tue Nov 08 17:15:50 2022

Prevent saving ViewModels in view saved state

Since converted fragents over to creation extras, we have a problem
where ViewModels were being saved in both the fragment and fragment view
saved state registries.

The cause of this is that the FragmentViewLifecycleOwner uses the
ViewModelStore of the fragment so when it calls enableSavedStateHandles
its registry also signs up to save all ViewModels in that store.

This is resolved by no longer calling enabledSavedStateHandles in
FragmentViewLifecycleOwner and making the default CreationExtras use the
fragment components directly.

RelNote: "Fragment will no longer incorrectly save ViewModels as part of
the view registry state."
Test: added test
Bug: 253546214

Change-Id: I10d2b5363d0abe967e92ad90a578d3bf88a2ca3b

M fragment/fragment/src/androidTest/java/androidx/fragment/app/FragmentViewLifecycleOwnerTest.kt
M fragment/fragment/src/main/java/androidx/fragment/app/FragmentViewLifecycleOwner.java

https://android-review.googlesource.com/2293203

jb...@google.com <jb...@google.com> #6Nov 10, 2022 01:28AM

Marked as fixed.

This has been fixed internally and will be available in the Fragment 1.6.0-alpha04 release.

ju...@google.com <ju...@google.com> #7Dec 7, 2022 06:35PM

The following release(s) address this bug.It is possible this bug has only been partially addressed:

androidx.fragment:fragment:1.6.0-alpha04

yo...@luchegroup.com <yo...@luchegroup.com> #8Dec 8, 2022 01:27AM

Thanks for the bug fix, I have confirmed that the issue is resolved.

eu...@gmail.com <eu...@gmail.com> #9Dec 22, 2022 09:32AM

We have a crash now with this change:

java.lang.IllegalStateException: enableSavedStateHandles() wasn't called prior to createSavedStateHandle() call
  at androidx.lifecycle.SavedStateHandleSupport.getSavedStateHandlesProvider(SavedStateHandleSupport.kt:115)
  at androidx.lifecycle.SavedStateHandleSupport.createSavedStateHandle(SavedStateHandleSupport.kt:65)
  at androidx.lifecycle.SavedStateHandleSupport.createSavedStateHandle(SavedStateHandleSupport.kt:103)
  at androidx.lifecycle.AbstractSavedStateViewModelFactory.create(AbstractSavedStateViewModelFactory.java:89)
  at dagger.hilt.android.internal.lifecycle.HiltViewModelFactory.create(HiltViewModelFactory.java:116)
  at androidx.lifecycle.ViewModelProvider.get(ViewModelProvider.kt:187)
  at androidx.lifecycle.ViewModelProvider.get(ViewModelProvider.kt:153)
  at androidx.lifecycle.viewmodel.compose.ViewModelKt.get(ViewModel.kt:215)
  at androidx.lifecycle.viewmodel.compose.ViewModelKt.viewModel(ViewModel.kt:156)
  at our.code.  (points to the place where the ViewModel is injected into a constructor of a Composable view

What we do in the composable code we call viewModel like:

    val extras = MutableCreationExtras()
    extras[SAVED_STATE_REGISTRY_OWNER_KEY] = LocalSavedStateRegistryOwner.current
    extras[VIEW_MODEL_STORE_OWNER_KEY] = checkNotNull(LocalViewModelStoreOwner.current) {
        "No ViewModelStoreOwner was provided via LocalViewModelStoreOwner"
    }
    extras[DEFAULT_ARGS_KEY] = bundleOf("PARAM_KEY" to "PARAM_VALUE")
    return viewModel(extras = extras)

I see that enableSavedStateHandles was deleted from FragmentViewLifecycleOwner.

Can you give some recommendations about situation?

jb...@google.com <jb...@google.com> #10Dec 22, 2022 11:32PM

Please file a separate bug with a minimal sample project that reproduces this issue.