ViewModel leak in nested fragments added to a backstack [129593351]

Fixed

Bug

Status Update

No update yet.

Description

dh...@gmail.com

created issue #1

Mar 31, 2019 09:22AM

Component used: 'androidx.lifecycle:lifecycle-extensions:2.0.0'
Devices/Android versions reproduced on: robolectric, emulator, real devices

I have an activity, and a fragment in it (MasterFragment).
Than I add nested fragment (NestedFragmentWithViewModel) to a MasterFragment.
NestedFragmentWithViewModel obtains viewmodel by `ViewModelProviders.of(this)`
Than I replace it (with adding to a backstack) with another fragment (NestedFragment).
After that, activity gets recreated, which is causing:
- MasterFragment to recrete
- NestedFragment (as it is on top of backstack) to recreate
After that, activity gets destroyed, which is causing:
- MasterFragment to destroy and clear its ViewModelStore
- NestedFragment to destroy and clear its ViewModelStore

But no one cares neither about NestedFragmentWithViewModel in the backstack, nor about clearing its ViewModelStore.
It is not cleared when fragment is added to a backstack (cause backstack can be popped to show fragment again).
And it is not cleared when MasterFragment destroys, cause it calls onDestroy on all its 'created' nested fragments, but fragments in a backstack were not created after activity get recreated, so should not be destroyed.

Fragment's ViewModelStore is cleared in fragment's onDestroy() in case activity.isFinishing(). That never happens so, whole ViewModelStore gets leaked.

It is not a totally FragmentManager issue, cause its respects lifecycle of fragments - 'what is dead may never die'.
So it seems fragment's logic to clear its ViewModelStore should be fixed.

Project with described scenario is attached.

ViewModelLeak.zip

714 KB

Download

Comments

se...@google.com <se...@google.com> Mar 31, 2019 10:59AM

Reassigned to il...@google.com.

dh...@gmail.com <dh...@gmail.com> #2Apr 1, 2019 06:17PM

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

il...@google.com <il...@google.com> #3Apr 9, 2019 06:29PM

Reassigned to jb...@google.com.

Yeah, looks like Fragments has *never* cleared the non-config state for Fragments that don't reach CREATED - turns out that the one place that is true is Fragments on the back stack after a configuration changes (they stay in the default INITIALIZING internal state).

This applies to both ViewModels not receiving onCleared() and direct child Fragments that are retained not receiving onDestroy() as well as grandchild Fragments and beyond (which may also be retained Fragments).

I've been able to replicate the ViewModel case in

https://android-review.googlesource.com/941796, still need to create a test for the retained Fragment case and dispatching the destruction to the child Fragments' FragmentManager instances. As well as actually fix the issue.

ap...@google.com <ap...@google.com> #4Apr 18, 2019 05:58PM

Marked as fixed.

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

commit c53eb8ad0d6763d3893f046cc58b91c838e845ff
Author: jbwoods <jbwoods@google.com>
Date: Tue Apr 16 15:35:39 2019

Save fragment mRemoving on save and restore

The mRemoving flag is not saved when a fragment goes through save
state. This means when a configuration change (or any other save and
restore cycle), the flag is lost. If a fragment is on the backstack,
this can cause that fragment to never reach the CREATED state again.

Adding the mRemoving flag to FragmentState ensures that fragments on
the backstack get to the CREATED state. A fragment can now also get to the
DESTROYED state if its FragmentManager is destroyed, meaning the
non-config state (viewModels) is properly cleared.

Test: new ViewModelTest, new SaveStateFragmentTest, rerun old test
Fixes: 129593351
Change-Id: Ia1d10e96f569dee606de2ac19c586901b240b3cd

M fragment/src/androidTest/java/androidx/fragment/app/SaveStateFragmentTest.kt
M fragment/src/androidTest/java/androidx/fragment/app/ViewModelTest.kt
M fragment/src/androidTest/java/androidx/fragment/app/test/ViewModelActivity.kt
M fragment/src/main/java/androidx/fragment/app/FragmentState.java

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

https://goto.google.com/android-sha1/c53eb8ad0d6763d3893f046cc58b91c838e845ff

dh...@gmail.com <dh...@gmail.com> #5Apr 19, 2019 04:37AM

Thanks for operative work guys! I guess this will be rolled out with a new androidx.fragment release?

il...@google.com <il...@google.com> #6Apr 19, 2019 03:31PM

Yes, it'll be part of Fragments 1.1.0-alpha07

Issue 129593351