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.

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

Fixes #1555 #2773

Merged
merged 2 commits into from
Sep 18, 2021
Merged

Fixes #1555 #2773

merged 2 commits into from
Sep 18, 2021

Conversation

VishnuSanal
Copy link
Member

Description

Issue tracker

Fixes #1555

Manual tests

  • Done

Build tasks success

Successfully running following tasks on local:

  • ./gradlew assembledebug
  • ./gradlew spotlessCheck

@VishnuSanal VishnuSanal added the Issue-Bug Related unexpected behavior or something worth investigating. label Sep 6, 2021
Copy link
Member

@EmmanuelMess EmmanuelMess left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be fixed, it would get a missing tab:

   try {
        if (fragmentManager == null) {
          fragmentManager = getActivity().getSupportFragmentManager();
        }
        fragments.add(0, fragmentManager.getFragment(savedInstanceState, "tab" + 0));
        fragments.add(1, fragmentManager.getFragment(savedInstanceState, "tab" + 1));
   } catch (Exception e) {
        e.printStackTrace();
   }
 

And remove the generic Exception.

@VishnuSanal
Copy link
Member Author

VishnuSanal commented Sep 8, 2021

it would get a missing tab

How?

I tried doing this:

if (!fragment.isAdded())
     fragmentManager.beginTransaction().add(fragment, "tab" + i).commit();

     fragmentManager.putFragment(outState, "tab" + i, fragment);

But it gave me the following memory leak:

┬───
│ 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 (DecorView↓ is not leaking and InputMethodManager is a
│    singleton)
│    ↓ InputMethodManager.mCurRootView
├─ com.android.internal.policy.DecorView instance
│    Leaking: NO (LinearLayout↓ is not leaking and View attached)
│    View is part of a window view hierarchy
│    View.mAttachInfo is not null (view attached)
│    View.mWindowAttachCount = 1
│    mContext instance of com.android.internal.policy.DecorContext, wrapping
│    activity com.amaze.filemanager.ui.activities.MainActivity with mDestroyed
│    = false
│    ↓ DecorView.mContentRoot
├─ android.widget.LinearLayout instance
│    Leaking: NO (MainActivity↓ is not leaking and View attached)
│    View is part of a window view hierarchy
│    View.mAttachInfo is not null (view attached)
│    View.mWindowAttachCount = 1
│    mContext instance of com.amaze.filemanager.ui.activities.MainActivity with
│    mDestroyed = false
│    ↓ View.mContext
├─ com.amaze.filemanager.ui.activities.MainActivity instance
│    Leaking: NO (LinearLayout↓ is not leaking and Activity#mDestroyed is false)
│    mainActivity instance of com.amaze.filemanager.ui.activities.MainActivity
│    with mDestroyed = false
│    mApplication instance of com.amaze.filemanager.application.AppConfig
│    mBase instance of androidx.appcompat.view.ContextThemeWrapper
│    ↓ MainActivity.indicator_layout
├─ android.widget.LinearLayout instance
│    Leaking: NO (Indicator↓ 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.indicator_layout
│    View.mWindowAttachCount = 1
│    mContext instance of com.amaze.filemanager.ui.activities.MainActivity with
│    mDestroyed = false
│    ↓ ViewGroup.mChildren
├─ android.view.View[] array
│    Leaking: NO (Indicator↓ is not leaking)
│    ↓ View[].[0]
├─ com.amaze.filemanager.ui.views.Indicator instance
│    Leaking: NO (View attached)
│    View is part of a window view hierarchy
│    View.mAttachInfo is not null (view attached)
│    View.mID = R.id.indicator
│    View.mWindowAttachCount = 1
│    mContext instance of com.amaze.filemanager.ui.activities.MainActivity with
│    mDestroyed = false
│    ↓ Indicator.viewPager
│                ~~~~~~~~~
├─ com.amaze.filemanager.ui.views.DisablableViewPager instance
│    Leaking: UNKNOWN
│    Retaining 87.2 kB in 1390 objects
│    View not part of a window view hierarchy
│    View.mAttachInfo is null (view detached)
│    View.mID = R.id.pager
│    View.mWindowAttachCount = 1
│    mContext instance of com.amaze.filemanager.ui.activities.MainActivity with
│    mDestroyed = false
│    ↓ View.mParent
│           ~~~~~~~
╰→ androidx.constraintlayout.widget.ConstraintLayout instance
      Leaking: YES (ObjectWatcher was watching this because com.amaze.
      filemanager.ui.fragments.TabFragment received Fragment#onDestroyView()
      callback (references to its views should be cleared to prevent leaks))
      Retaining 2.5 kB in 59 objects
      key = e3f65561-803e-4f17-87c5-2e79d6be6037
      watchDurationMillis = 14584
      retainedDurationMillis = 9581
      View not part of a window view hierarchy
      View.mAttachInfo is null (view detached)
      View.mWindowAttachCount = 1
      mContext instance of com.amaze.filemanager.ui.activities.MainActivity
      with mDestroyed = false

METADATA

Build.VERSION.SDK_INT: 29
Build.MANUFACTURER: realme
LeakCanary version: 2.6
App process name: com.amaze.filemanager.debug
Stats: LruCache[maxSize=3000,hits=7565,misses=94493,hitRate=7%]
RandomAccess[bytes=5536529,reads=94493,travel=31931450462,range=23362279,size=28
557628]
Heap dump reason: user request
Analysis duration: 8640 ms

What do you think 🤔

Also, from Telegram group, ICYMI

Ref: Your comment on 2772

And if the fragment is not added? Doesn't this just skip putting it?

Yes, we're saving that. Right? So isn't that okay to skip if it is not present in the FragmentManager? (Reference (https://stackoverflow.com/a/26285801/9652621))

@EmmanuelMess
Copy link
Member

it would get a missing tab

How?

It would get a missing tab if you call fragmentManager.getFragment(savedInstanceState, "tab" + 0) on a missing key

@EmmanuelMess
Copy link
Member

if (!fragment.isAdded())
     fragmentManager.beginTransaction().add(fragment, "tab" + i).commit();

     fragmentManager.putFragment(outState, "tab" + i, fragment);

I don't understand why you try to add the fragment twice here. Also, the leak seems not related.

@VishnuSanal
Copy link
Member Author

if (!fragment.isAdded())
     fragmentManager.beginTransaction().add(fragment, "tab" + i).commit();

     fragmentManager.putFragment(outState, "tab" + i, fragment);

I don't understand why you try to add the fragment twice here. Also, the leak seems not related.

Twice? I was adding the fragment if that was not added since you asked it here

And if the fragment is not added? Doesn't this just skip putting it?

& here

it would get a missing tab

How?

It would get a missing tab if you call fragmentManager.getFragment(savedInstanceState, "tab" + 0) on a missing key

@EmmanuelMess
Copy link
Member

if (!fragment.isAdded())
     fragmentManager.beginTransaction().add(fragment, "tab" + i).commit();

     fragmentManager.putFragment(outState, "tab" + i, fragment);

I don't understand why you try to add the fragment twice here. Also, the leak seems not related.

Twice? I was adding the fragment if that was not added since you asked it here

Sorry my mistake, why are you adding the fragment here though? I meant for you to add it if its not in the bundle when the fragment is recreated.

@EmmanuelMess
Copy link
Member

EmmanuelMess commented Sep 9, 2021

it would get a missing tab

How?

It would get a missing tab if you call fragmentManager.getFragment(savedInstanceState, "tab" + 0) on a missing key

this still happens AFAIK in this solution:

if (!fragment.isAdded())
     fragmentManager.beginTransaction().add(fragment, "tab" + i).commit();

     fragmentManager.putFragment(outState, "tab" + i, fragment);

@VishalNehra
Copy link
Member

VishalNehra commented Sep 10, 2021

This will conflict with #2780 (edit by EmmanuelMess) #2789

@EmmanuelMess
Copy link
Member

How does this fix the issue?

@VishalNehra
Copy link
Member

This will force execute any pending transactions for the fragment manager. So by the time we reach to put fragments in instance state we have both of them.

Copy link
Member

@EmmanuelMess EmmanuelMess left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should work

@EmmanuelMess EmmanuelMess merged commit a95b3f8 into hotfix/3.6.2 Sep 18, 2021
@EmmanuelMess EmmanuelMess deleted the vishnu-fragment-fix branch September 18, 2021 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Bug Related unexpected behavior or something worth investigating.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants