Fenix: FNX-14657 ⁃ [Bug][LeakCanary] SavedLoginsAuthFragment leaked

[cadeyrn]

/cc @mcomella

Steps to reproduce

Settings > Logins and passwords > Saved logins > unlock

Expected behavior

No leak.

Actual behavior

┬───
│ 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

METADATA

Build.VERSION.SDK_INT: 29
Build.MANUFACTURER: OnePlus
LeakCanary version: 2.4
App process name: org.mozilla.fenix.debug
Analysis duration: 6083 ms

Device information

• Android device: OnePlus 7T Pro McLaren Edition / Oxygen OS 10.0.10 (Android 10)
• Fenix version: master branch revision 841d19e8be8b0d85e207524073c1ce2a10b3b670

[pyricau]

Side note: I'd suggest using triple quotes when sharing traces (recommend admins edit the bug report).

HomeActivity is alive (not destroyed), it references its ViewModelStore which contains a BiometricViewModel. From that we can deduce that the lifecycle of BiometricViewModel is tied to the lifecycle of the activity. However BiometricViewModel has an mClientCallback field that references an anonymous class defined in SavedLoginsAuthFragment, and SavedLoginsAuthFragment is destroyed.

This likely means that SavedLoginsAuthFragment somehow registered a callback to BiometricViewModel but didn't clear it when destroyed.

The leaking object, biometricPromptCallback, an anonymous implemention of BiometricPrompt.AuthenticationCallback, is defined here

It's registered here:

biometricPrompt = BiometricPrompt(this, executor, biometricPromptCallback)

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 the view model is always created from an activity, so the callback always has the lifecycle of the activity. This looks like a design issue with how Android X BiometricPrompt work. Until Google fixes it, the AuthenticationCallback implementation needs to stop being an anonymous class and it needs to have a nullable reference to the fragment.

[pyricau]

Filed https://issuetracker.google.com/issues/167014923