Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[BottomSheetBehavior] SettleRunnable leaks view hierarchy through ViewRootImpl$RunQueue #1417

Closed
pyricau opened this issue Jun 18, 2020 · 3 comments · May be fixed by #1670
Closed

[BottomSheetBehavior] SettleRunnable leaks view hierarchy through ViewRootImpl$RunQueue #1417

pyricau opened this issue Jun 18, 2020 · 3 comments · May be fixed by #1670
Assignees
@drchen
Labels
bug P1 Widget: BottomSheet

Comments

@pyricau
Copy link

pyricau commented Jun 18, 2020 •
edited
Loading

Description:

TL;DR: BottomSheetBehavior.startSettlingAnimation() leaks the view hierarchy if that view hierarchy is not attached to a window at the time of the call (e.g. activity is destroyed)

Details

BottomSheetBehavior.startSettlingAnimation() calls ViewCompat.postOnAnimation(child, settleRunnable);. If the view is attached to a window, settleRunnable is posted and later executed, all is fine. If there is no attached window, the SettleRunnable instance ends up stored in android.view.ViewRootImpl$RunQueue until a new window is available, which could take a while (e.g. process alive with no activity running).

This is a known general issue on Android: posting to a view can create stuck runnables if the view hierarchy is detached.

On 6.0.1 ViewRootImpl has a static field: static final ThreadLocal<RunQueue> sRunQueues = new ThreadLocal<RunQueue>(); :

https://cs.android.com/android/platform/superproject/+/android-6.0.1_r81:frameworks/base/core/java/android/view/ViewRootImpl.java;l=6872-6877

    /**
     * The run queue is used to enqueue pending work from Views when no Handler is
     * attached.  The work is executed during the next call to performTraversals on
     * the thread.
     * @hide
     */
    static final class RunQueue {

In recent versions of Android ViewRootImpl.RunQueue became HandlerActionQueue

Expected behavior: BottomSheetBehavior should probably check that child is attached before posting.

Leak report:

┬───
│ GC Root: System class
│
├─ flow.Preconditions class
│    Leaking: NO (Thread↓ is not leaking and a class is never leaking)
│    ↓ static Preconditions.MAIN_THREAD
├─ java.lang.Thread instance
│    Leaking: NO (the main thread always runs)
│    Thread name: 'main'
│    ↓ Thread.localValues
│             ~~~~~~~~~~~
├─ java.lang.ThreadLocal$Values instance
│    Leaking: UNKNOWN
│    ↓ ThreadLocal$Values.table
│                         ~~~~~
├─ java.lang.Object[] array
│    Leaking: UNKNOWN
│    ↓ Object[].[80]
│               ~~~~
├─ android.view.ViewRootImpl$RunQueue instance
│    Leaking: UNKNOWN
│    ↓ ViewRootImpl$RunQueue.mActions
│                            ~~~~~~~~
├─ java.util.ArrayList instance
│    Leaking: UNKNOWN
│    ↓ ArrayList.array
│                ~~~~~
├─ java.lang.Object[] array
│    Leaking: UNKNOWN
│    ↓ Object[].[1]
│               ~~~
├─ android.view.ViewRootImpl$RunQueue$HandlerAction instance
│    Leaking: UNKNOWN
│    ↓ ViewRootImpl$RunQueue$HandlerAction.action
│                                          ~~~~~~
├─ com.google.android.material.bottomsheet.BottomSheetBehavior$SettleRunnable instance
│    Leaking: UNKNOWN
│    ↓ BottomSheetBehavior$SettleRunnable.view
│                                         ~~~~
├─ android.widget.FrameLayout instance
│    Leaking: YES (View.mContext references a destroyed activity)
│    mContext instance of android.view.ContextThemeWrapper, wrapping activity com.squareup.ui.main.MainActivity with mDestroyed = true
│    View#mParent is set
│    View#mAttachInfo is null (view detached)
│    View.mWindowAttachCount = 1
│    ↓ FrameLayout.mContext
├─ android.view.ContextThemeWrapper instance
│    Leaking: YES (FrameLayout↑ is leaking and ContextThemeWrapper wraps an Activity with Activity.mDestroyed true)
│    ↓ ContextThemeWrapper.mInflater
├─ com.android.internal.policy.PhoneLayoutInflater instance
│    Leaking: YES (ContextThemeWrapper↑ is leaking)
│    ↓ PhoneLayoutInflater.mPrivateFactory
╰→ com.squareup.ui.main.MainActivity instance
​     Leaking: YES (ObjectWatcher was watching this because com.squareup.ui.main.MainActivity received Activity#onDestroy() callback and Activity#mDestroyed is true)

Material Library version: 1.1.0 - this bug still exists on master currently (latest release 1.3.0-alpha01)

Device: Can be reproduced at least on Android 6.0.1 but should be reproducible on all versions.
@pyricau pyricau added the bug label Jun 18, 2020
@pyricau
Copy link
Author

pyricau commented Jun 30, 2020 •
edited
Loading

More on this: BottomSheetBehavior.settleToStatePendingLayout() has this:

    // Start the animation; wait until a pending layout if there is one.
    ViewParent parent = child.getParent();
    if (parent != null && parent.isLayoutRequested() && ViewCompat.isAttachedToWindow(child)) {
      final int finalState = state;
      child.post(
          new Runnable() {
            @Override
            public void run() {
              settleToState(child, finalState);
            }
          });
    } else {
      settleToState(child, state);
    }

As you can see, if the view is detached we're moving forward with creating the SettleRunnable. But if the view is attached we're posting the settleToState() call, which can then execute at a later time where we're detached.
@pyricau
Copy link
Author

pyricau commented Jun 30, 2020

cc @leticiarossi @ymarian I'm not sure if this is the right place to file this issue. Happy to offer a patch if you're interested.
@ymarian
Copy link
Contributor

ymarian commented Jul 1, 2020

@pyricau that's always welcome!
pyricau added a commit to pyricau/material-components-android that referenced this issue Aug 31, 2020
@pyricau
Fix SettleRunnable leak
ccb550d 
Ensures that we don't post the `SettleRunnable` if the view isn't attached, because otherwise if there is no alive activity that `SettleRunnable` can stay in the ViewRootImpl `RunQueue` until a new activity starts.

Fixes material-components#1417.
@pyricau pyricau mentioned this issue Aug 31, 2020
Open
@drchen drchen self-assigned this Feb 18, 2022
@rtsisyk rtsisyk mentioned this issue Feb 25, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug P1 Widget: BottomSheet
3 participants
@pyricau @drchen @ymarian