Fix object pool for lifecycler

The original implementation of object pool for lifecycle
transactions tried to always recycle objects after a
transaction was scheduled. In case when a client was running
in the same process this lead to objects being emptied before
it could actually perform the transaction.
Also when checking if object was already in the pool we should
use "==" instead of equality check.

Bug: 70554032
Bug: 71346774
Test: com.android.server.am.ClientLifecycleManagerTests
Test: android.app.servertransaction.ObjectPoolTests
Change-Id: I85fb3dae4589c2390e00a37144da0d285d16d151

diff --git a/core/java/android/app/ActivityThread.java b/core/java/android/app/ActivityThread.java
index de346f3..e610ac4 100644 (file)
--- a/core/java/android/app/ActivityThread.java
+++ b/core/java/android/app/ActivityThread.java
@@ -1768,7 +1768,14 @@ public final class ActivityThread extends ClientTransactionHandler {
                             (String[]) ((SomeArgs) msg.obj).arg2);
                     break;
                 case EXECUTE_TRANSACTION:
-                    mTransactionExecutor.execute(((ClientTransaction) msg.obj));
+                    final ClientTransaction transaction = (ClientTransaction) msg.obj;
+                    mTransactionExecutor.execute(transaction);
+                    if (isSystem()) {
+                        // Client transactions inside system process are recycled on the client side
+                        // instead of ClientLifecycleManager to avoid being cleared before this
+                        // message is handled.
+                        transaction.recycle();
+                    }
                     break;
             }
             Object obj = msg.obj;

diff --git a/core/java/android/app/servertransaction/ClientTransaction.java b/core/java/android/app/servertransaction/ClientTransaction.java
index 7703c6b..08ad2f0 100644 (file)
--- a/core/java/android/app/servertransaction/ClientTransaction.java
+++ b/core/java/android/app/servertransaction/ClientTransaction.java
@@ -56,6 +56,11 @@ public class ClientTransaction implements Parcelable, ObjectPoolItem {
     /** Target client activity. Might be null if the entire transaction is targeting an app. */
     private IBinder mActivityToken;
 
+    /** Get the target client of the transaction. */
+    public IApplicationThread getClient() {
+        return mClient;
+    }
+
     /**
      * Add a message to the end of the sequence of callbacks.
      * @param activityCallback A single message that can contain a lifecycle request/callback.

diff --git a/core/java/android/app/servertransaction/ObjectPool.java b/core/java/android/app/servertransaction/ObjectPool.java
index 9812125..2fec30a 100644 (file)
--- a/core/java/android/app/servertransaction/ObjectPool.java
+++ b/core/java/android/app/servertransaction/ObjectPool.java
@@ -16,8 +16,8 @@
 
 package android.app.servertransaction;
 
+import java.util.ArrayList;
 import java.util.HashMap;
-import java.util.LinkedList;
 import java.util.Map;
 
 /**
@@ -27,7 +27,7 @@ import java.util.Map;
 class ObjectPool {
 
     private static final Object sPoolSync = new Object();
-    private static final Map<Class, LinkedList<? extends ObjectPoolItem>> sPoolMap =
+    private static final Map<Class, ArrayList<? extends ObjectPoolItem>> sPoolMap =
             new HashMap<>();
 
     private static final int MAX_POOL_SIZE = 50;
@@ -40,9 +40,9 @@ class ObjectPool {
     public static <T extends ObjectPoolItem> T obtain(Class<T> itemClass) {
         synchronized (sPoolSync) {
             @SuppressWarnings("unchecked")
-            LinkedList<T> itemPool = (LinkedList<T>) sPoolMap.get(itemClass);
+            final ArrayList<T> itemPool = (ArrayList<T>) sPoolMap.get(itemClass);
             if (itemPool != null && !itemPool.isEmpty()) {
-                return itemPool.poll();
+                return itemPool.remove(itemPool.size() - 1);
             }
             return null;
         }
@@ -56,16 +56,20 @@ class ObjectPool {
     public static <T extends ObjectPoolItem> void recycle(T item) {
         synchronized (sPoolSync) {
             @SuppressWarnings("unchecked")
-            LinkedList<T> itemPool = (LinkedList<T>) sPoolMap.get(item.getClass());
+            ArrayList<T> itemPool = (ArrayList<T>) sPoolMap.get(item.getClass());
             if (itemPool == null) {
-                itemPool = new LinkedList<>();
+                itemPool = new ArrayList<>();
                 sPoolMap.put(item.getClass(), itemPool);
             }
-            if (itemPool.contains(item)) {
-                throw new IllegalStateException("Trying to recycle already recycled item");
+            // Check if the item is already in the pool
+            final int size = itemPool.size();
+            for (int i = 0; i < size; i++) {
+                if (itemPool.get(i) == item) {
+                    throw new IllegalStateException("Trying to recycle already recycled item");
+                }
             }
 
-            if (itemPool.size() < MAX_POOL_SIZE) {
+            if (size < MAX_POOL_SIZE) {
                 itemPool.add(item);
             }
         }

diff --git a/core/tests/coretests/src/android/app/servertransaction/ObjectPoolTests.java b/core/tests/coretests/src/android/app/servertransaction/ObjectPoolTests.java
index c19a343..aefc47e 100644 (file)
--- a/core/tests/coretests/src/android/app/servertransaction/ObjectPoolTests.java
+++ b/core/tests/coretests/src/android/app/servertransaction/ObjectPoolTests.java
@@ -42,8 +42,7 @@ import org.junit.runner.RunWith;
 
 @RunWith(AndroidJUnit4.class)
 @SmallTest
-// TODO: b/70616950
-//@Presubmit
+@Presubmit
 public class ObjectPoolTests {
 
     // 1. Check if two obtained objects from pool are not the same.

diff --git a/services/core/java/com/android/server/am/ClientLifecycleManager.java b/services/core/java/com/android/server/am/ClientLifecycleManager.java
index 014f708..ae8d9fc 100644 (file)
--- a/services/core/java/com/android/server/am/ClientLifecycleManager.java
+++ b/services/core/java/com/android/server/am/ClientLifecycleManager.java
@@ -21,6 +21,7 @@ import android.app.IApplicationThread;
 import android.app.servertransaction.ClientTransaction;
 import android.app.servertransaction.ClientTransactionItem;
 import android.app.servertransaction.ActivityLifecycleItem;
+import android.os.Binder;
 import android.os.IBinder;
 import android.os.RemoteException;
 
@@ -42,9 +43,14 @@ class ClientLifecycleManager {
      * @see ClientTransaction
      */
     void scheduleTransaction(ClientTransaction transaction) throws RemoteException {
+        final IApplicationThread client = transaction.getClient();
         transaction.schedule();
-        // TODO: b/70616950
-        //transaction.recycle();
+        if (!(client instanceof Binder)) {
+            // If client is not an instance of Binder - it's a remote call and at this point it is
+            // safe to recycle the object. All objects used for local calls will be recycled after
+            // the transaction is executed on client in ActivityThread.
+            transaction.recycle();
+        }
     }
 
     /**

diff --git a/services/tests/servicestests/src/com/android/server/am/ActivityRecordTests.java b/services/tests/servicestests/src/com/android/server/am/ActivityRecordTests.java
index 1dba39f..b38a413 100644 (file)
--- a/services/tests/servicestests/src/com/android/server/am/ActivityRecordTests.java
+++ b/services/tests/servicestests/src/com/android/server/am/ActivityRecordTests.java
@@ -35,24 +35,23 @@ import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.Mockito.any;
 import static org.mockito.Mockito.anyInt;
-import static org.mockito.Mockito.atLeast;
+import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.eq;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
-
 import android.app.servertransaction.ClientTransaction;
 import android.app.servertransaction.PauseActivityItem;
 import android.graphics.Rect;
 import android.platform.test.annotations.Presubmit;
 import android.support.test.filters.MediumTest;
 import android.support.test.runner.AndroidJUnit4;
+import android.util.MutableBoolean;
 
 import org.junit.runner.RunWith;
 import org.junit.Before;
 import org.junit.Test;
-
-import org.mockito.ArgumentCaptor;
+import org.mockito.invocation.InvocationOnMock;
 
 /**
  * Tests for the {@link ActivityRecord} class.
@@ -110,23 +109,20 @@ public class ActivityRecordTests extends ActivityTestsBase {
 
     @Test
     public void testPausingWhenVisibleFromStopped() throws Exception {
+        final MutableBoolean pauseFound = new MutableBoolean(false);
+        doAnswer((InvocationOnMock invocationOnMock) -> {
+            final ClientTransaction transaction = invocationOnMock.getArgument(0);
+            if (transaction.getLifecycleStateRequest() instanceof PauseActivityItem) {
+                pauseFound.value = true;
+            }
+            return null;
+        }).when(mActivity.app.thread).scheduleTransaction(any());
         mActivity.state = STOPPED;
-        mActivity.makeVisibleIfNeeded(null /* starting */);
-        assertEquals(mActivity.state, PAUSING);
-
-        final ArgumentCaptor<ClientTransaction> transaction =
-                ArgumentCaptor.forClass(ClientTransaction.class);
-        verify(mActivity.app.thread, atLeast(1)).scheduleTransaction(transaction.capture());
 
-        boolean pauseFound = false;
-
-        for (ClientTransaction targetTransaction : transaction.getAllValues()) {
-            if (targetTransaction.getLifecycleStateRequest() instanceof PauseActivityItem) {
-                pauseFound = true;
-            }
-        }
+        mActivity.makeVisibleIfNeeded(null /* starting */);
 
-        assertTrue(pauseFound);
+        assertEquals(mActivity.state, PAUSING);
+        assertTrue(pauseFound.value);
     }
 
     @Test

diff --git a/services/tests/servicestests/src/com/android/server/am/ClientLifecycleManagerTests.java b/services/tests/servicestests/src/com/android/server/am/ClientLifecycleManagerTests.java
new file mode 100644 (file)
index 0000000..ef6d5e8
--- /dev/null
+++ b/services/tests/servicestests/src/com/android/server/am/ClientLifecycleManagerTests.java
@@ -0,0 +1,44 @@
+package com.android.server.am;
+
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+
+import android.app.IApplicationThread;
+import android.app.servertransaction.ClientTransaction;
+import android.os.Binder;
+import android.platform.test.annotations.Presubmit;
+import android.support.test.filters.SmallTest;
+import android.support.test.runner.AndroidJUnit4;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+@RunWith(AndroidJUnit4.class)
+@SmallTest
+@Presubmit
+public class ClientLifecycleManagerTests {
+
+    @Test
+    public void testScheduleAndRecycleBinderClientTransaction() throws Exception {
+        ClientTransaction item = spy(ClientTransaction.obtain(mock(IApplicationThread.class),
+                new Binder()));
+
+        ClientLifecycleManager clientLifecycleManager = new ClientLifecycleManager();
+        clientLifecycleManager.scheduleTransaction(item);
+
+        verify(item, times(1)).recycle();
+    }
+
+    @Test
+    public void testScheduleNoRecycleNonBinderClientTransaction() throws Exception {
+        ClientTransaction item = spy(ClientTransaction.obtain(mock(IApplicationThread.Stub.class),
+                new Binder()));
+
+        ClientLifecycleManager clientLifecycleManager = new ClientLifecycleManager();
+        clientLifecycleManager.scheduleTransaction(item);
+
+        verify(item, times(0)).recycle();
+    }
+}