Do not reuse fragment indices.

Bug 25472591

Use a SparseArray instead of two ArrayLists to track
active fragments. This allows monotonically increasing
indexes instead of reusing indexes in the ArrayList.

Test: ran cts tests

Change-Id: I98f1b7928a0ef2373b719b76582a4c6da3425817

diff --git a/core/java/android/app/FragmentManager.java b/core/java/android/app/FragmentManager.java
index 0d859a1..d710d8b 100644 (file)
--- a/core/java/android/app/FragmentManager.java
+++ b/core/java/android/app/FragmentManager.java
@@ -552,6 +552,7 @@ final class FragmentManagerState implements Parcelable {
     int[] mAdded;
     BackStackState[] mBackStack;
     int mPrimaryNavActiveIndex = -1;
+    int mNextFragmentIndex;
     
     public FragmentManagerState() {
     }
@@ -561,6 +562,7 @@ final class FragmentManagerState implements Parcelable {
         mAdded = in.createIntArray();
         mBackStack = in.createTypedArray(BackStackState.CREATOR);
         mPrimaryNavActiveIndex = in.readInt();
+        mNextFragmentIndex = in.readInt();
     }
     
     public int describeContents() {
@@ -572,6 +574,7 @@ final class FragmentManagerState implements Parcelable {
         dest.writeIntArray(mAdded);
         dest.writeTypedArray(mBackStack, flags);
         dest.writeInt(mPrimaryNavActiveIndex);
+        dest.writeInt(mNextFragmentIndex);
     }
     
     public static final Parcelable.Creator<FragmentManagerState> CREATOR
@@ -638,10 +641,10 @@ final class FragmentManagerImpl extends FragmentManager implements LayoutInflate
 
     ArrayList<OpGenerator> mPendingActions;
     boolean mExecutingActions;
-    
-    ArrayList<Fragment> mActive;
+
+    int mNextFragmentIndex = 0;
+    SparseArray<Fragment> mActive;
     ArrayList<Fragment> mAdded;
-    ArrayList<Integer> mAvailIndices;
     ArrayList<BackStackRecord> mBackStack;
     ArrayList<Fragment> mCreatedMenus;
     
@@ -839,6 +842,7 @@ final class FragmentManagerImpl extends FragmentManager implements LayoutInflate
         }
 
         doPendingDeferredStart();
+        burpActive();
         return executePop;
     }
 
@@ -882,10 +886,6 @@ final class FragmentManagerImpl extends FragmentManager implements LayoutInflate
         if (index == -1) {
             return null;
         }
-        if (index >= mActive.size()) {
-            throwException(new IllegalStateException("Fragment no longer exists for key "
-                    + key + ": index " + index));
-        }
         Fragment f = mActive.get(index);
         if (f == null) {
             throwException(new IllegalStateException("Fragment no longer exists for key "
@@ -939,7 +939,7 @@ final class FragmentManagerImpl extends FragmentManager implements LayoutInflate
                         writer.print(Integer.toHexString(System.identityHashCode(this)));
                         writer.println(":");
                 for (int i=0; i<N; i++) {
-                    Fragment f = mActive.get(i);
+                    Fragment f = mActive.valueAt(i);
                     writer.print(prefix); writer.print("  #"); writer.print(i);
                             writer.print(": "); writer.println(f);
                     if (f != null) {
@@ -1034,10 +1034,6 @@ final class FragmentManagerImpl extends FragmentManager implements LayoutInflate
             writer.print(prefix); writer.print("  mNoTransactionsBecause=");
                     writer.println(mNoTransactionsBecause);
         }
-        if (mAvailIndices != null && mAvailIndices.size() > 0) {
-            writer.print(prefix); writer.print("  mAvailIndices: ");
-                    writer.println(Arrays.toString(mAvailIndices.toArray()));
-        }
     }
 
     Animator loadAnimator(Fragment fragment, int transit, boolean enter,
@@ -1164,7 +1160,7 @@ final class FragmentManagerImpl extends FragmentManager implements LayoutInflate
                         // If we have a target fragment, push it along to at least CREATED
                         // so that this one can rely on it as an initialized dependency.
                         if (f.mTarget != null) {
-                            if (!mActive.contains(f.mTarget)) {
+                            if (mActive.get(f.mTarget.mIndex) != f.mTarget) {
                                 throw new IllegalStateException("Fragment " + f
                                         + " declared target fragment " + f.mTarget
                                         + " that does not belong to this FragmentManager!");
@@ -1557,7 +1553,7 @@ final class FragmentManagerImpl extends FragmentManager implements LayoutInflate
             // and detached.
             final int numActive = mActive.size();
             for (int i = 0; i < numActive; i++) {
-                Fragment f = mActive.get(i);
+                Fragment f = mActive.valueAt(i);
                 if (f != null && (f.mRemoving || f.mDetached) && !f.mIsNewlyAdded) {
                     moveFragmentToExpectedState(f);
                     if (f.mLoaderManager != null) {
@@ -1581,7 +1577,7 @@ final class FragmentManagerImpl extends FragmentManager implements LayoutInflate
         if (mActive == null) return;
 
         for (int i=0; i<mActive.size(); i++) {
-            Fragment f = mActive.get(i);
+            Fragment f = mActive.valueAt(i);
             if (f != null) {
                 performPendingDeferredStart(f);
             }
@@ -1592,18 +1588,12 @@ final class FragmentManagerImpl extends FragmentManager implements LayoutInflate
         if (f.mIndex >= 0) {
             return;
         }
-        
-        if (mAvailIndices == null || mAvailIndices.size() <= 0) {
-            if (mActive == null) {
-                mActive = new ArrayList<Fragment>();
-            }
-            f.setIndex(mActive.size(), mParent);
-            mActive.add(f);
-            
-        } else {
-            f.setIndex(mAvailIndices.remove(mAvailIndices.size()-1), mParent);
-            mActive.set(f.mIndex, f);
+
+        f.setIndex(mNextFragmentIndex++, mParent);
+        if (mActive == null) {
+            mActive = new SparseArray<>();
         }
+        mActive.put(f.mIndex, f);
         if (DEBUG) Log.v(TAG, "Allocated fragment index " + f);
     }
     
@@ -1613,11 +1603,9 @@ final class FragmentManagerImpl extends FragmentManager implements LayoutInflate
         }
         
         if (DEBUG) Log.v(TAG, "Freeing fragment index " + f);
-        mActive.set(f.mIndex, null);
-        if (mAvailIndices == null) {
-            mAvailIndices = new ArrayList<Integer>();
-        }
-        mAvailIndices.add(f.mIndex);
+        // Don't remove yet. That happens in burpActive(). This prevents
+        // concurrent modification while iterating over mActive
+        mActive.put(f.mIndex, null);
         mHost.inactivateFragment(f.mWho);
         f.initState();
     }
@@ -1754,7 +1742,7 @@ final class FragmentManagerImpl extends FragmentManager implements LayoutInflate
         if (mActive != null) {
             // Now for any known fragment.
             for (int i=mActive.size()-1; i>=0; i--) {
-                Fragment f = mActive.get(i);
+                Fragment f = mActive.valueAt(i);
                 if (f != null && f.mFragmentId == id) {
                     return f;
                 }
@@ -1776,7 +1764,7 @@ final class FragmentManagerImpl extends FragmentManager implements LayoutInflate
         if (mActive != null && tag != null) {
             // Now for any known fragment.
             for (int i=mActive.size()-1; i>=0; i--) {
-                Fragment f = mActive.get(i);
+                Fragment f = mActive.valueAt(i);
                 if (f != null && tag.equals(f.mTag)) {
                     return f;
                 }
@@ -1788,7 +1776,7 @@ final class FragmentManagerImpl extends FragmentManager implements LayoutInflate
     public Fragment findFragmentByWho(String who) {
         if (mActive != null && who != null) {
             for (int i=mActive.size()-1; i>=0; i--) {
-                Fragment f = mActive.get(i);
+                Fragment f = mActive.valueAt(i);
                 if (f != null && (f=f.findFragmentByWho(who)) != null) {
                     return f;
                 }
@@ -1953,6 +1941,7 @@ final class FragmentManagerImpl extends FragmentManager implements LayoutInflate
         }
 
         doPendingDeferredStart();
+        burpActive();
     }
 
     /**
@@ -1983,6 +1972,7 @@ final class FragmentManagerImpl extends FragmentManager implements LayoutInflate
         }
 
         doPendingDeferredStart();
+        burpActive();
 
         return didSomething;
     }
@@ -2248,7 +2238,7 @@ final class FragmentManagerImpl extends FragmentManager implements LayoutInflate
             for (int i = 0; i < numActive; i++) {
                 // Allow added fragments to be removed during the pop since we aren't going
                 // to move them to the final state with moveToState(mCurState).
-                Fragment fragment = mActive.get(i);
+                Fragment fragment = mActive.valueAt(i);
                 if (fragment != null && fragment.mView != null && fragment.mIsNewlyAdded
                         && record.interactsWith(fragment.mContainerId)) {
                     fragment.mIsNewlyAdded = false;
@@ -2360,7 +2350,7 @@ final class FragmentManagerImpl extends FragmentManager implements LayoutInflate
     private void endAnimatingAwayFragments() {
         final int numFragments = mActive == null ? 0 : mActive.size();
         for (int i = 0; i < numFragments; i++) {
-            Fragment fragment = mActive.get(i);
+            Fragment fragment = mActive.valueAt(i);
             if (fragment != null && fragment.getAnimatingAway() != null) {
                 // Give up waiting for the animation and just end it.
                 fragment.getAnimatingAway().end();
@@ -2400,7 +2390,7 @@ final class FragmentManagerImpl extends FragmentManager implements LayoutInflate
         if (mHavePendingDeferredStart) {
             boolean loadersRunning = false;
             for (int i=0; i<mActive.size(); i++) {
-                Fragment f = mActive.get(i);
+                Fragment f = mActive.valueAt(i);
                 if (f != null && f.mLoaderManager != null) {
                     loadersRunning |= f.mLoaderManager.hasRunningLoaders();
                 }
@@ -2489,7 +2479,7 @@ final class FragmentManagerImpl extends FragmentManager implements LayoutInflate
         ArrayList<FragmentManagerNonConfig> childFragments = null;
         if (mActive != null) {
             for (int i=0; i<mActive.size(); i++) {
-                Fragment f = mActive.get(i);
+                Fragment f = mActive.valueAt(i);
                 if (f != null) {
                     if (f.mRetainInstance) {
                         if (fragments == null) {
@@ -2594,7 +2584,7 @@ final class FragmentManagerImpl extends FragmentManager implements LayoutInflate
         FragmentState[] active = new FragmentState[N];
         boolean haveFragments = false;
         for (int i=0; i<N; i++) {
-            Fragment f = mActive.get(i);
+            Fragment f = mActive.valueAt(i);
             if (f != null) {
                 if (f.mIndex < 0) {
                     throwException(new IllegalStateException(
@@ -2680,6 +2670,7 @@ final class FragmentManagerImpl extends FragmentManager implements LayoutInflate
         fms.mActive = active;
         fms.mAdded = added;
         fms.mBackStack = backStack;
+        fms.mNextFragmentIndex = mNextFragmentIndex;
         if (mPrimaryNav != null) {
             fms.mPrimaryNavActiveIndex = mPrimaryNav.mIndex;
         }
@@ -2722,10 +2713,7 @@ final class FragmentManagerImpl extends FragmentManager implements LayoutInflate
         
         // Build the full list of active fragments, instantiating them from
         // their saved state.
-        mActive = new ArrayList<>(fms.mActive.length);
-        if (mAvailIndices != null) {
-            mAvailIndices.clear();
-        }
+        mActive = new SparseArray<>(fms.mActive.length);
         for (int i=0; i<fms.mActive.length; i++) {
             FragmentState fs = fms.mActive[i];
             if (fs != null) {
@@ -2735,18 +2723,11 @@ final class FragmentManagerImpl extends FragmentManager implements LayoutInflate
                 }
                 Fragment f = fs.instantiate(mHost, mContainer, mParent, childNonConfig);
                 if (DEBUG) Log.v(TAG, "restoreAllState: active #" + i + ": " + f);
-                mActive.add(f);
+                mActive.put(f.mIndex, f);
                 // Now that the fragment is instantiated (or came from being
                 // retained above), clear mInstance in case we end up re-restoring
                 // from this FragmentState again.
                 fs.mInstance = null;
-            } else {
-                mActive.add(null);
-                if (mAvailIndices == null) {
-                    mAvailIndices = new ArrayList<>();
-                }
-                if (DEBUG) Log.v(TAG, "restoreAllState: avail #" + i);
-                mAvailIndices.add(i);
             }
         }
         
@@ -2757,9 +2738,8 @@ final class FragmentManagerImpl extends FragmentManager implements LayoutInflate
             for (int i = 0; i < count; i++) {
                 Fragment f = nonConfigFragments.get(i);
                 if (f.mTargetIndex >= 0) {
-                    if (f.mTargetIndex < mActive.size()) {
-                        f.mTarget = mActive.get(f.mTargetIndex);
-                    } else {
+                    f.mTarget = mActive.get(f.mTargetIndex);
+                    if (f.mTarget == null) {
                         Log.w(TAG, "Re-attaching retained fragment " + f
                                 + " target no longer exists: " + f.mTargetIndex);
                         f.mTarget = null;
@@ -2813,8 +2793,25 @@ final class FragmentManagerImpl extends FragmentManager implements LayoutInflate
         if (fms.mPrimaryNavActiveIndex >= 0) {
             mPrimaryNav = mActive.get(fms.mPrimaryNavActiveIndex);
         }
+
+        mNextFragmentIndex = fms.mNextFragmentIndex;
     }
-    
+
+    /**
+     * To prevent list modification errors, mActive sets values to null instead of
+     * removing them when the Fragment becomes inactive. This cleans up the list at the
+     * end of executing the transactions.
+     */
+    private void burpActive() {
+        if (mActive != null) {
+            for (int i = mActive.size() - 1; i >= 0; i--) {
+                if (mActive.valueAt(i) == null) {
+                    mActive.delete(mActive.keyAt(i));
+                }
+            }
+        }
+    }
+
     public void attachController(FragmentHostCallback<?> host, FragmentContainer container,
             Fragment parent) {
         if (mHost != null) throw new IllegalStateException("Already attached");
@@ -3051,8 +3048,8 @@ final class FragmentManagerImpl extends FragmentManager implements LayoutInflate
     }
 
     public void setPrimaryNavigationFragment(Fragment f) {
-        if (f != null && (f.getFragmentManager() != this || f.mIndex >= mActive.size()
-                || mActive.get(f.mIndex) != f)) {
+        if (f != null && (mActive.get(f.mIndex) != f
+                || (f.mHost != null && f.getFragmentManager() != this))) {
             throw new IllegalArgumentException("Fragment " + f
                     + " is not an active fragment of FragmentManager " + this);
         }