BiometricPrompt.AuthenticationCallback tied to activity lifecycle and leak fragments when following example code. [167014923]

Fixed

Bug

Status Update

No update yet.

Description

py...@gmail.com

created issue #1

Aug 30, 2020 02:05PM

Biometric library version: 1.1.0-alpha01

This bug was reported in Mozilla Fenix, but it's actually a leak in BiometricViewModel.

Mozilla Fenix bug report: https://github.com/mozilla-mobile/fenix/issues/13477

To reproduce, follow the official Android X sample code and add the sample code to a fragment. Display the login prompt, get the fragment destroyed, the fragment is now leaking

As soon as the fragment is destroyed, the leak triggers as the fragment cannot be garbage collected because the AuthenticationCallback implementation is retained.

┬───
│ GC Root: Global variable in native code
│
├─ dalvik.system.PathClassLoader instance
│ Leaking: NO (InternalLeakCanary↓ is not leaking and A ClassLoader is never leaking)
│ ↓ PathClassLoader.runtimeInternalObjects
├─ java.lang.Object[] array
│ Leaking: NO (InternalLeakCanary↓ is not leaking)
│ ↓ Object[].[891]
├─ leakcanary.internal.InternalLeakCanary class
│ Leaking: NO (HomeActivity↓ is not leaking and a class is never leaking)
│ ↓ static InternalLeakCanary.resumedActivity
├─ org.mozilla.fenix.HomeActivity instance
│ Leaking: NO (Activity#mDestroyed is false)
│ ↓ HomeActivity.mViewModelStore
│                ~~~~~~~~~~~~~~~
├─ androidx.lifecycle.ViewModelStore instance
│ Leaking: UNKNOWN
│ ↓ ViewModelStore.mMap
│                  ~~~~
├─ java.util.HashMap instance
│ Leaking: UNKNOWN
│ ↓ HashMap.table
│           ~~~~~
├─ java.util.HashMap$Node[] array
│ Leaking: UNKNOWN
│ ↓ HashMap$Node[].[2]
│                  ~~~
├─ java.util.HashMap$Node instance
│ Leaking: UNKNOWN
│ ↓ HashMap$Node.value
│                ~~~~~
├─ androidx.biometric.BiometricViewModel instance
│ Leaking: UNKNOWN
│ ↓ BiometricViewModel.mClientCallback
│                      ~~~~~~~~~~~~~~~
├─ org.mozilla.fenix.settings.logins.fragment.SavedLoginsAuthFragment$biometricPromptCallback$1 instance
│ Leaking: UNKNOWN
│ Anonymous subclass of androidx.biometric.BiometricPrompt$AuthenticationCallback
│ ↓ SavedLoginsAuthFragment$biometricPromptCallback$1.this$0
│                                                     ~~~~~~
╰→ org.mozilla.fenix.settings.logins.fragment.SavedLoginsAuthFragment instance
 Leaking: YES (ObjectWatcher was watching this because org.mozilla.fenix.settings.logins.fragment.SavedLoginsAuthFragment received Fragment#onDestroy() callback and Fragment#mFragmentManager is null)
 key = 77a534a9-73fb-4b7e-9bb1-1c26e0064ca8
 watchDurationMillis = 9964
 retainedDurationMillis = 4964

Looking at the Android X BiometricPrompt sources, the callback is set in BiometricPrompt.init() which is called from the BiometricPrompt constructor and never unset.

    private void init(
            @Nullable FragmentActivity activity,
            @Nullable FragmentManager fragmentManager,
            @Nullable Executor executor,
            @NonNull AuthenticationCallback callback) {

        mClientFragmentManager = fragmentManager;

        if (activity != null) {
            final BiometricViewModel viewModel =
                    new ViewModelProvider(activity).get(BiometricViewModel.class);
            if (executor != null) {
                viewModel.setClientExecutor(executor);
            }
            viewModel.setClientCallback(callback);
        }
    }

So, as it stands, there is no way to clear the callback, and BiometricViewModel is always created with an activity lifecycle, so the callback always has the lifecycle of the activity. This means that currently the library will leak any fragment it's used from.

Comments

py...@gmail.com <py...@gmail.com> #2Aug 31, 2020 09:14AM

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

rb...@unwire.com <rb...@unwire.com> #3Oct 14, 2020 11:24AM

Any news/possible for this issue?

ca...@gmail.com <ca...@gmail.com> #4Nov 4, 2020 09:48PM

Can confirm that it still happens in 1.1.0-beta01

cu...@google.com <cu...@google.com> Jan 15, 2021 09:49PM

Accepted by cu...@google.com.

ap...@google.com <ap...@google.com> #5Jan 21, 2021 01:18AM

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

commit fcbbd9368d75b736de4b92d067be84421376b294
Author: Curtis Belmonte <curtislb@google.com>
Date: Wed Jan 20 15:54:34 2021

Reset BiometricViewModel callback in Fragment#onDestroy()

Currently, the client callback reference held by BiometricViewModel is
retained for the lifecycle of the hosting activity. In the (likely) case
that a client is using a fragment within that activity to host
BiometricPrompt and passes an AuthenticationCallback with a reference to
that fragment, this will cause the fragment to leak.

This commit applies a minimal fix for the issue by resetting the
callback reference held by the view model when the host fragment is
destroyed (via a LifecycleObserver). This shouldn't affect the prompt's
behavior across configuration changes such as device rotation, since the
callback should be reinitialized by the client in onCreate() or similar.

Test: Biometric integration test app on API 27-30.
Test: ./gradlew biometric:biometric:test
Test: ./gradlew biometric:biometric:connectedAndroidTest
Test: ./gradlew biometric:integration-tests:testapp:connectedAndroidTest

Bug: 167014923
Change-Id: I7086460fac3921a490f4e2abf0671adec5c146bd

M biometric/biometric/src/main/java/androidx/biometric/BiometricPrompt.java
M biometric/biometric/src/main/java/androidx/biometric/BiometricViewModel.java

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

cu...@google.com <cu...@google.com> Jan 21, 2021 02:21AM

Marked as fixed.

di...@gmail.com <di...@gmail.com> #6Feb 9, 2021 06:08PM

I don't use Fragments and use a View-based solutions instead. I create prompt like this:

BiometricPrompt(requireActivity, myAuthCallback)

So it is tied to the activity and it still leaks my view-based screens (1.2.0-alpha02). Ideally there would be a way to reset this callback. Latest commit fixes this leak only for Fragments (sadly).

Is there any workaround maybe? Dirty (but working) ones would do too ;)

ja...@sharesies.co.nz <ja...@sharesies.co.nz> #7Oct 6, 2022 01:04AM

Ditto with the above comment. We're a compose-only app, and use `BiometricPrompt` with the `Activity` context as opposed to passing in a Fragment. This results in leaks, as the callback held by `BiometricViewModel` is never cleared.

An API allowing developers to pass in their own `LifecycleOwner` to set up the `ResetCallbackObserver` observer would be a relatively quick and easy fix for this. I'm more than happy to draft a change for this and submit it via GitHub!

te...@gmail.com <te...@gmail.com> #8May 31, 2023 03:47PM

Any updates on this?