BackPressHandler leaks the view after its detached

[pyricau]

HandleBackPressWhenAttached adds a OnBackPressedCallback callback to the OnBackPressedDispatcherOwner implemented by the context of the view, i.e. in this case the activity.

The leak trace indicates that the OnBackPressedCallback is still held by the activity even though the view itself is detached.

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

Line 89 in 8c524db

onBackPressedCallback.isEnabled = false

We probably shouldn't keep the callback around (and its reference to the view) after the view has been detached.

Leaktrace

┬───
│ GC Root: System class
│
├─ android.view.inputmethod.InputMethodManager class
│    Leaking: NO (InputMethodManager↓ is not leaking and a class is never leaking)
│    ↓ static InputMethodManager.sInstance
├─ android.view.inputmethod.InputMethodManager instance
│    Leaking: NO (DevDrawerDraggerLayout↓ is not leaking and InputMethodManager is a singleton)
│    ↓ InputMethodManager.mNextServedView
├─ com.squareup.development.drawer.DevDrawerDraggerLayout instance
│    Leaking: NO (SquidSystemUiActivity↓ is not leaking and View attached)
│    View is part of a window view hierarchy
│    View.mAttachInfo is not null (view attached)
│    View.mID = R.id.development_drawer_dragger
│    View.mWindowAttachCount = 1
│    mContext instance of com.squareup.squid.systemuiapp.ui.SquidSystemUiActivity with mDestroyed = false
│    ↓ View.mContext
├─ com.squareup.squid.systemuiapp.ui.SquidSystemUiActivity instance
│    Leaking: NO (LifecycleRegistry↓ is not leaking and Activity#mDestroyed is false)
│    context instance of com.squareup.squid.systemuiapp.ui.SquidSystemUiActivity with mDestroyed = false
│    mApplication instance of com.squareup.squid.systemuiapp.DevSquidSystemUiApplication
│    mBase instance of androidx.appcompat.view.ContextThemeWrapper
│    ↓ ComponentActivity.mLifecycleRegistry
├─ androidx.lifecycle.LifecycleRegistry instance
│    Leaking: NO (mState is not DESTROYED)
│    mState = CREATED
│    ↓ LifecycleRegistry.mObserverMap
│                        ~~~~~~~~~~~~
├─ androidx.arch.core.internal.FastSafeIterableMap instance
│    Leaking: UNKNOWN
│    Retaining 41.5 kB in 765 objects
│    ↓ FastSafeIterableMap[key()]
│                         ~~~~~~~
├─ androidx.activity.OnBackPressedDispatcher$LifecycleOnBackPressedCancellable instance
│    Leaking: UNKNOWN
│    Retaining 40.0 kB in 707 objects
│    ↓ OnBackPressedDispatcher$LifecycleOnBackPressedCancellable.mOnBackPressedCallback
│                                                                ~~~~~~~~~~~~~~~~~~~~~~
├─ com.squareup.workflow1.ui.HandleBackPressWhenAttached$onBackPressedCallback$1 instance
│    Leaking: UNKNOWN
│    Retaining 39.9 kB in 705 objects
│    Anonymous subclass of androidx.activity.OnBackPressedCallback
│    ↓ HandleBackPressWhenAttached$onBackPressedCallback$1.this$0
│                                                          ~~~~~~
├─ com.squareup.workflow1.ui.HandleBackPressWhenAttached instance
│    Leaking: UNKNOWN
│    Retaining 39.9 kB in 701 objects
│    ↓ HandleBackPressWhenAttached.view
│                                  ~~~~
├─ android.widget.LinearLayout instance
│    Leaking: YES (View detached yet still part of window view hierarchy)
│    Retaining 39.9 kB in 700 objects
│    View is part of a window view hierarchy
│    View.mAttachInfo is null (view detached)
│    View.mID = R.id.parent_panel
│    View.mWindowAttachCount = 1
│    mContext instance of com.squareup.squid.systemuiapp.ui.SquidSystemUiActivity with mDestroyed = false
│    ↓ View.mParent
├─ android.widget.FrameLayout instance
│    Leaking: YES (LinearLayout↑ is leaking and View detached yet still part of window view hierarchy)
│    Retaining 21.8 kB in 407 objects
│    View is part of a window view hierarchy
│    View.mAttachInfo is null (view detached)
│    View.mID = R.id.null
│    View.mWindowAttachCount = 1
│    mContext instance of android.view.ContextThemeWrapper, wrapping activity com.squareup.squid.systemuiapp.ui.SquidSystemUiActivity with mDestroyed = false
│    ↓ View.mParent
├─ android.widget.FrameLayout instance
│    Leaking: YES (FrameLayout↑ is leaking and View detached yet still part of window view hierarchy)
│    Retaining 20.6 kB in 387 objects
│    View is part of a window view hierarchy
│    View.mAttachInfo is null (view detached)
│    View.mWindowAttachCount = 1
│    mContext instance of android.view.ContextThemeWrapper, wrapping activity com.squareup.squid.systemuiapp.ui.SquidSystemUiActivity with mDestroyed = false
│    ↓ View.mParent
╰→ com.android.internal.policy.DecorView instance
​     Leaking: YES (ObjectWatcher was watching this because com.android.internal.policy.DecorView received View#onDetachedFromWindow() callback)
​     Retaining 18.8 kB in 364 objects
​     key = a174b29b-52f2-4989-a189-971edc493e4a
​     watchDurationMillis = 242736
​     retainedDurationMillis = 237731
​     View not part of a window view hierarchy
​     View.mAttachInfo is null (view detached)
​     View.mWindowAttachCount = 1
​     mContext instance of android.view.ContextThemeWrapper, wrapping activity com.squareup.squid.systemuiapp.ui.SquidSystemUiActivity with mDestroyed = false

https://app.bugsnag.com/square-inc/register-android-dev-leaks/errors/6262b5b5473e980008b4bce8

[rjrjr]

The proposed fix would break the order of the handler in the dispatcher's queue, non-starter.

I think the real issue is in the start method. We probably should no-op if we find no ViewTreeLifecycleOwner. Maybe same if it's destroyed. (@pyricau, you're confident that LeakCanary doesn't perform its Activity leak checks too eagerly, right, before all calls to LifecycleObserver.onDestroy?)

  fun start() {
    view.context.onBackPressedDispatcherOwnerOrNull()
      ?.let { owner ->
        owner.onBackPressedDispatcher.addCallback(owner, onBackPressedCallback)
        view.addOnAttachStateChangeListener(this)
        if (view.isAttachedToWindow) onViewAttachedToWindow(view)

        // We enable the handler only while its view is attached to a window.
        // This ensures that a temporarily removed view (e.g. for caching)
        // does not participate in back button handling.
        ViewTreeLifecycleOwner.get(view)?.lifecycle?.addObserver(this)
      }
  }

[rjrjr]

Problem was introduced at #632.

I think the fix is to stop relying on ViewTreeLifecycleOwner, which might not be available when we need it -- note all the ?. in that line. Instead the onBackPressedCallback should have a nullable reference to the View, which should be cleared when the callback is disabled.

[rjrjr]

Oh my, here's a very naive thing:

  fun start() {
    view.context.onBackPressedDispatcherOwnerOrNull()
      ?.let { owner ->
        owner.onBackPressedDispatcher.addCallback(owner, onBackPressedCallback)

We're making the OnBackPressedDispatcherOwner the owner of the callback -- why the hell does OnBackPressedDispatcherOwner extend LifecycleOwner? I hate Jetpack Lifecycle, I really do.

The argument should instead be the ViewLifecycleOwner, and our own ViewLifecycleOwner machinery is redundant. Question is whether we can count on the VLO having been set by the time View.backPressedHandler is called? I don't know that we can.

[rjrjr]

Question is whether we can count on the VLO having been set by the time View.backPressedHandler is called? I don't know that we can.

We cannot.