Skip to content

Commit 1b2c670

Browse files
0nko0nko
authored
Fix: IllegalStateException crash (#6074)
Task/Issue URL: https://app.asana.com/1/137249556945/project/1207418217763355/task/1210250194815685?focus=true ### Description This PR fixes a crash caused by `FragmentStateAdapter` attempting to restore fragment state when `FragmentManager` loses its fragments after activity recreation. ### Steps to test this PR - [x] Enable Don't keep activities - [x] Set DDG as a default browser - [x] Enable tab swiping - [x] Open DDG → Open a few tabs - [x] Go to tab manager - [x] Switch to an app that can open links (like Twitter) - [x] Tap on a link - [x] Notice DDG opens the link and doesn't crash
1 parent 00e161d commit 1b2c670

File tree

4 files changed

+30
-76
lines changed

4 files changed

+30
-76
lines changed

‎app/src/main/java/com/duckduckgo/app/browser/BrowserActivity.kt

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -206,9 +206,7 @@ open class BrowserActivity : DuckDuckGoActivity() {
206206

207207
private val tabPagerAdapter by lazy {
208208
TabPagerAdapter(
209-
fragmentManager = supportFragmentManager,
210-
lifecycleOwner = this,
211-
activityIntent = intent,
209+
activity = this,
212210
swipingTabsFeature = swipingTabsFeature,
213211
)
214212
}
@@ -876,7 +874,7 @@ open class BrowserActivity : DuckDuckGoActivity() {
876874
tabPager.setPageTransformer(MarginPageTransformer(resources.getDimension(com.duckduckgo.mobile.android.R.dimen.keyline_1).toPx().toInt()))
877875

878876
savedInstanceState?.getBundle(KEY_TAB_PAGER_STATE)?.let {
879-
tabPagerAdapter.restoreState(it)
877+
tabPagerAdapter.restore(it)
880878
}
881879
}
882880

‎app/src/main/java/com/duckduckgo/app/browser/tabs/DefaultTabManager.kt

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@ import timber.log.Timber
3131
interface TabManager {
3232
companion object {
3333
const val MAX_ACTIVE_TABS = 20
34-
const val NEW_TAB_CREATION_TIMEOUT_LIMIT = 2 // seconds
3534
}
3635

3736
fun registerCallbacks(onTabsUpdated: (List<TabModel>) -> Unit)

‎app/src/main/java/com/duckduckgo/app/browser/tabs/adapter/FragmentStateAdapter.java

Lines changed: 17 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import android.view.ViewGroup;
3030
import android.view.ViewParent;
3131
import android.widget.FrameLayout;
32+
3233
import androidx.annotation.CallSuper;
3334
import androidx.annotation.NonNull;
3435
import androidx.annotation.Nullable;
@@ -47,8 +48,8 @@
4748
import androidx.viewpager2.adapter.StatefulAdapter;
4849
import androidx.viewpager2.widget.ViewPager2;
4950

50-
import com.duckduckgo.common.ui.tabs.SwipingTabsFeatureProvider;
5151
import com.duckduckgo.app.browser.tabs.TabManager;
52+
import com.duckduckgo.common.ui.tabs.SwipingTabsFeatureProvider;
5253

5354
import java.util.ArrayDeque;
5455
import java.util.ArrayList;
@@ -57,6 +58,7 @@
5758
import java.util.Set;
5859
import java.util.concurrent.ConcurrentHashMap;
5960
import java.util.concurrent.CopyOnWriteArrayList;
61+
6062
import timber.log.Timber;
6163

6264
/**
@@ -89,7 +91,6 @@ public abstract class FragmentStateAdapter extends RecyclerView.Adapter<Fragment
8991
implements StatefulAdapter {
9092
// State saving config
9193
private static final String KEY_PREFIX_FRAGMENT = "f#";
92-
private static final String KEY_PREFIX_STATE = "s#";
9394

9495
// Fragment GC config
9596
private static final long GRACE_WINDOW_TIME_MS = 10_000; // 10 seconds
@@ -107,7 +108,6 @@ public abstract class FragmentStateAdapter extends RecyclerView.Adapter<Fragment
107108
@SuppressWarnings("WeakerAccess") // to avoid creation of a synthetic accessor
108109
final LongSparseArray<Fragment> mFragments = new LongSparseArray<>();
109110

110-
private final LongSparseArray<Fragment.SavedState> mSavedStates = new LongSparseArray<>();
111111
private final LongSparseArray<Integer> mItemIdToViewHolder = new LongSparseArray<>();
112112

113113
private FragmentMaxLifecycleEnforcer mFragmentMaxLifecycleEnforcer;
@@ -130,42 +130,20 @@ public abstract class FragmentStateAdapter extends RecyclerView.Adapter<Fragment
130130
/**
131131
* @param fragmentActivity if the {@link ViewPager2} lives directly in a {@link
132132
* FragmentActivity} subclass.
133-
* @see FragmentStateAdapter#FragmentStateAdapter(Fragment)
134-
* @see FragmentStateAdapter#FragmentStateAdapter(FragmentManager, Lifecycle)
135-
*/
136-
public FragmentStateAdapter(@NonNull FragmentActivity fragmentActivity) {
137-
this(fragmentActivity.getSupportFragmentManager(), fragmentActivity.getLifecycle());
138-
}
139-
140-
/**
141-
* @param fragment if the {@link ViewPager2} lives directly in a {@link Fragment} subclass.
142-
* @see FragmentStateAdapter#FragmentStateAdapter(FragmentActivity)
143-
* @see FragmentStateAdapter#FragmentStateAdapter(FragmentManager, Lifecycle)
144-
*/
145-
public FragmentStateAdapter(@NonNull Fragment fragment) {
146-
this(fragment.getChildFragmentManager(), fragment.getLifecycle());
147-
}
148-
149-
/**
150-
* @param fragmentManager of {@link ViewPager2}'s host
151-
* @param lifecycle of {@link ViewPager2}'s host
152-
* @see FragmentStateAdapter#FragmentStateAdapter(FragmentActivity)
153-
* @see FragmentStateAdapter#FragmentStateAdapter(Fragment)
133+
* @param swipingTabsFeature Feature flag to enable swiping tabs fixes
154134
*/
155135
public FragmentStateAdapter(
156-
@NonNull FragmentManager fragmentManager, @NonNull Lifecycle lifecycle) {
157-
mFragmentManager = fragmentManager;
158-
mLifecycle = lifecycle;
159-
mSwipingTabsFeature = null;
160-
super.setHasStableIds(true);
136+
@NonNull FragmentActivity fragmentActivity,
137+
SwipingTabsFeatureProvider swipingTabsFeature) {
138+
this(fragmentActivity.getSupportFragmentManager(),
139+
fragmentActivity.getLifecycle(),
140+
swipingTabsFeature);
161141
}
162142

163143
/**
164144
* @param fragmentManager of {@link ViewPager2}'s host
165145
* @param lifecycle of {@link ViewPager2}'s host
166146
* @param swipingTabsFeature Feature flag to enable swiping tabs fixes
167-
* @see FragmentStateAdapter#FragmentStateAdapter(FragmentActivity)
168-
* @see FragmentStateAdapter#FragmentStateAdapter(Fragment)
169147
*/
170148
public FragmentStateAdapter(
171149
@NonNull FragmentManager fragmentManager,
@@ -307,7 +285,6 @@ private void ensureFragment(int position) {
307285
if (!mFragments.containsKey(itemId)) {
308286
// TODO(133419201): check if a Fragment provided here is a new Fragment
309287
Fragment newFragment = createFragment(position);
310-
newFragment.setInitialSavedState(mSavedStates.get(itemId));
311288
mFragments.put(itemId, newFragment);
312289
}
313290
}
@@ -519,10 +496,6 @@ private void removeFragment(long itemId) {
519496
}
520497
}
521498

522-
if (!containsItem(itemId)) {
523-
mSavedStates.remove(itemId);
524-
}
525-
526499
if (!fragment.isAdded()) {
527500
mFragments.remove(itemId);
528501
return;
@@ -538,8 +511,6 @@ private void removeFragment(long itemId) {
538511
mFragmentEventDispatcher.dispatchPreSavedInstanceState(fragment);
539512
Fragment.SavedState savedState = mFragmentManager.saveFragmentInstanceState(fragment);
540513
mFragmentEventDispatcher.dispatchPostEvents(onPost);
541-
542-
mSavedStates.put(itemId, savedState);
543514
}
544515
List<FragmentTransactionCallback.OnPostEventListener> onPost =
545516
mFragmentEventDispatcher.dispatchPreRemoved(fragment);
@@ -635,7 +606,7 @@ public final void setHasStableIds(boolean hasStableIds) {
635606
@Override
636607
public final @NonNull Parcelable saveState() {
637608
/* TODO(b/122670461): use custom {@link Parcelable} instead of Bundle to save space */
638-
Bundle savedState = new Bundle(mFragments.size() + mSavedStates.size());
609+
Bundle savedState = new Bundle(mFragments.size());
639610

640611
/* save references to active fragments */
641612
for (int ix = 0; ix < mFragments.size(); ix++) {
@@ -646,23 +617,13 @@ public final void setHasStableIds(boolean hasStableIds) {
646617
mFragmentManager.putFragment(savedState, key, fragment);
647618
}
648619
}
649-
650-
/* Write {@link mSavedStates) into a {@link Parcelable} */
651-
for (int ix = 0; ix < mSavedStates.size(); ix++) {
652-
long itemId = mSavedStates.keyAt(ix);
653-
if (containsItem(itemId)) {
654-
String key = createKey(KEY_PREFIX_STATE, itemId);
655-
savedState.putParcelable(key, mSavedStates.get(itemId));
656-
}
657-
}
658-
659620
return savedState;
660621
}
661622

662623
@Override
663624
@SuppressWarnings("deprecation")
664625
public final void restoreState(@NonNull Parcelable savedState) {
665-
if (!mSavedStates.isEmpty() || !mFragments.isEmpty()) {
626+
if (!mFragments.isEmpty()) {
666627
throw new IllegalStateException(
667628
"Expected the adapter to be 'fresh' while restoring state.");
668629
}
@@ -676,21 +637,14 @@ public final void restoreState(@NonNull Parcelable savedState) {
676637
for (String key : bundle.keySet()) {
677638
if (isValidKey(key, KEY_PREFIX_FRAGMENT)) {
678639
long itemId = parseIdFromKey(key, KEY_PREFIX_FRAGMENT);
679-
Fragment fragment = mFragmentManager.getFragment(bundle, key);
680-
mFragments.put(itemId, fragment);
681-
continue;
682-
}
683-
684-
if (isValidKey(key, KEY_PREFIX_STATE)) {
685-
long itemId = parseIdFromKey(key, KEY_PREFIX_STATE);
686-
Fragment.SavedState state = bundle.getParcelable(key);
687-
if (containsItem(itemId)) {
688-
mSavedStates.put(itemId, state);
640+
try {
641+
Fragment fragment = mFragmentManager.getFragment(bundle, key);
642+
mFragments.put(itemId, fragment);
643+
} catch (IllegalStateException e) {
644+
Timber.w("FragmentManager is in a bad state, unable to restore fragment %d",
645+
itemId);
689646
}
690-
continue;
691647
}
692-
693-
throw new IllegalArgumentException("Unexpected key in savedState: " + key);
694648
}
695649

696650
if (!mFragments.isEmpty()) {

‎app/src/main/java/com/duckduckgo/app/browser/tabs/adapter/TabPagerAdapter.kt

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -17,11 +17,9 @@
1717
package com.duckduckgo.app.browser.tabs.adapter
1818

1919
import android.annotation.SuppressLint
20-
import android.content.Intent
20+
import android.os.Bundle
2121
import android.os.Message
2222
import androidx.fragment.app.Fragment
23-
import androidx.fragment.app.FragmentManager
24-
import androidx.lifecycle.LifecycleOwner
2523
import androidx.recyclerview.widget.DiffUtil
2624
import androidx.recyclerview.widget.RecyclerView
2725
import com.duckduckgo.app.browser.BrowserActivity
@@ -30,11 +28,9 @@ import com.duckduckgo.app.browser.tabs.TabManager.TabModel
3028
import com.duckduckgo.common.ui.tabs.SwipingTabsFeatureProvider
3129

3230
class TabPagerAdapter(
33-
lifecycleOwner: LifecycleOwner,
34-
fragmentManager: FragmentManager,
35-
private val activityIntent: Intent?,
31+
private val activity: BrowserActivity,
3632
swipingTabsFeature: SwipingTabsFeatureProvider,
37-
) : FragmentStateAdapter(fragmentManager, lifecycleOwner.lifecycle, swipingTabsFeature) {
33+
) : FragmentStateAdapter(activity, swipingTabsFeature) {
3834
private val tabs = mutableListOf<TabModel>()
3935
private var messageForNewFragment: Message? = null
4036

@@ -52,7 +48,7 @@ class TabPagerAdapter(
5248

5349
override fun createFragment(position: Int): Fragment {
5450
val tab = tabs[position]
55-
val isExternal = activityIntent?.getBooleanExtra(BrowserActivity.LAUNCH_FROM_EXTERNAL_EXTRA, false) == true
51+
val isExternal = activity.intent?.getBooleanExtra(BrowserActivity.LAUNCH_FROM_EXTERNAL_EXTRA, false) == true
5652

5753
return if (messageForNewFragment != null) {
5854
val message = messageForNewFragment
@@ -65,6 +61,13 @@ class TabPagerAdapter(
6561
}
6662
}
6763

64+
fun restore(state: Bundle) {
65+
// state is only useful when there are fragments to restore (also avoids a crash)
66+
if (activity.supportFragmentManager.fragments.isNotEmpty()) {
67+
restoreState(state)
68+
}
69+
}
70+
6871
fun setMessageForNewFragment(message: Message) {
6972
messageForNewFragment = message
7073
}

0 commit comments

Comments
 (0)