Merge "Make the disconnecting list from activity up-to-date"

4 files changed, 53 insertions, 23 deletions

diff --git a/core/java/android/app/ActivityManagerInternal.java b/core/java/android/app/ActivityManagerInternal.java
index 4e47594b6196..c60f7bd29ce8 100644
--- a/core/java/android/app/ActivityManagerInternal.java
+++ b/core/java/android/app/ActivityManagerInternal.java
@@ -278,7 +278,7 @@ public abstract class ActivityManagerInternal {
             String resolvedType, boolean fgRequired, String callingPackage, @UserIdInt int userId,
             boolean allowBackgroundActivityStarts) throws TransactionTooLargeException;
 
-    public abstract void disconnectActivityFromServices(Object connectionHolder, Object conns);
+    public abstract void disconnectActivityFromServices(Object connectionHolder);
     public abstract void cleanUpServices(@UserIdInt int userId, ComponentName component,
             Intent baseIntent);
     public abstract ActivityInfo getActivityInfoForUser(ActivityInfo aInfo, @UserIdInt int userId);
diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java
index 6560777edd53..d80ff35ce70d 100644
--- a/services/core/java/com/android/server/am/ActivityManagerService.java
+++ b/services/core/java/com/android/server/am/ActivityManagerService.java
@@ -18924,19 +18924,16 @@ public class ActivityManagerService extends IActivityManager.Stub
         }
 
         // The arguments here are untyped because the base ActivityManagerInternal class
-        // doesn't have compile-time visiblity into ActivityServiceConnectionHolder or
+        // doesn't have compile-time visibility into ActivityServiceConnectionHolder or
         // ConnectionRecord.
         @Override
-        public void disconnectActivityFromServices(Object connectionHolder, Object conns) {
+        public void disconnectActivityFromServices(Object connectionHolder) {
             // 'connectionHolder' is an untyped ActivityServiceConnectionsHolder
-            // 'conns' is an untyped HashSet<ConnectionRecord>
             final ActivityServiceConnectionsHolder holder =
                     (ActivityServiceConnectionsHolder) connectionHolder;
-            final HashSet<ConnectionRecord> toDisconnect = (HashSet<ConnectionRecord>) conns;
-            synchronized(ActivityManagerService.this) {
-                for (ConnectionRecord cr : toDisconnect) {
-                    mServices.removeConnectionLocked(cr, null, holder);
-                }
+            synchronized (ActivityManagerService.this) {
+                holder.forEachConnection(cr -> mServices.removeConnectionLocked(
+                        (ConnectionRecord) cr, null /* skipApp */, holder /* skipAct */));
             }
         }
 
diff --git a/services/core/java/com/android/server/wm/ActivityRecord.java b/services/core/java/com/android/server/wm/ActivityRecord.java
index df6dfc4d8ffe..ed38e9a73050 100644
--- a/services/core/java/com/android/server/wm/ActivityRecord.java
+++ b/services/core/java/com/android/server/wm/ActivityRecord.java
@@ -3037,6 +3037,8 @@ final class ActivityRecord extends WindowToken implements WindowManagerService.A
         }
         // Throw away any services that have been bound by this activity.
         mServiceConnectionsHolder.disconnectActivityFromServices();
+        // This activity record is removing, make sure not to disconnect twice.
+        mServiceConnectionsHolder = null;
     }
 
     @Override
diff --git a/services/core/java/com/android/server/wm/ActivityServiceConnectionsHolder.java b/services/core/java/com/android/server/wm/ActivityServiceConnectionsHolder.java
index 6e75f9c9167f..5dfc261480f2 100644
--- a/services/core/java/com/android/server/wm/ActivityServiceConnectionsHolder.java
+++ b/services/core/java/com/android/server/wm/ActivityServiceConnectionsHolder.java
@@ -18,10 +18,13 @@ package com.android.server.wm;
 
 import static com.android.server.wm.ActivityStack.ActivityState.PAUSING;
 import static com.android.server.wm.ActivityStack.ActivityState.RESUMED;
+import static com.android.server.wm.ActivityTaskManagerDebugConfig.DEBUG_CLEANUP;
+import static com.android.server.wm.ActivityTaskManagerDebugConfig.TAG_ATM;
+
+import android.util.ArraySet;
+import android.util.Slog;
 
 import java.io.PrintWriter;
-import java.util.HashSet;
-import java.util.Iterator;
 import java.util.function.Consumer;
 
 /**
@@ -30,6 +33,8 @@ import java.util.function.Consumer;
  * instance of this per activity for tracking all services connected to that activity. AM will
  * sometimes query this to bump the OOM score for the processes with services connected to visible
  * activities.
+ * <p>
+ * Public methods are called in AM lock, otherwise in WM lock.
  */
 public class ActivityServiceConnectionsHolder<T> {
 
@@ -44,7 +49,10 @@ public class ActivityServiceConnectionsHolder<T> {
      * on the WM side since we don't perform operations on the object. Mainly here for communication
      * and booking with the AM side.
      */
-    private HashSet<T> mConnections;
+    private ArraySet<T> mConnections;
+
+    /** Whether all connections of {@link #mActivity} are being removed. */
+    private volatile boolean mIsDisconnecting;
 
     ActivityServiceConnectionsHolder(ActivityTaskManagerService service, ActivityRecord activity) {
         mService = service;
@@ -54,8 +62,16 @@ public class ActivityServiceConnectionsHolder<T> {
     /** Adds a connection record that the activity has bound to a specific service. */
     public void addConnection(T c) {
         synchronized (mService.mGlobalLock) {
+            if (mIsDisconnecting) {
+                // This is unlikely to happen because the caller should create a new holder.
+                if (DEBUG_CLEANUP) {
+                    Slog.e(TAG_ATM, "Skip adding connection " + c + " to a disconnecting holder of "
+                            + mActivity);
+                }
+                return;
+            }
             if (mConnections == null) {
-                mConnections = new HashSet<>();
+                mConnections = new ArraySet<>();
             }
             mConnections.add(c);
         }
@@ -67,6 +83,9 @@ public class ActivityServiceConnectionsHolder<T> {
             if (mConnections == null) {
                 return;
             }
+            if (DEBUG_CLEANUP && mIsDisconnecting) {
+                Slog.v(TAG_ATM, "Remove pending disconnecting " + c + " of " + mActivity);
+            }
             mConnections.remove(c);
         }
     }
@@ -88,26 +107,33 @@ public class ActivityServiceConnectionsHolder<T> {
             if (mConnections == null || mConnections.isEmpty()) {
                 return;
             }
-            final Iterator<T> it = mConnections.iterator();
-            while (it.hasNext()) {
-                T c = it.next();
-                consumer.accept(c);
+            for (int i = mConnections.size() - 1; i >= 0; i--) {
+                consumer.accept(mConnections.valueAt(i));
             }
         }
     }
 
-    /** Removes the connection between the activity and all services that were connected to it. */
+    /**
+     * Removes the connection between the activity and all services that were connected to it. In
+     * general, this method is used to clean up if the activity didn't unbind services before it
+     * is destroyed.
+     */
     void disconnectActivityFromServices() {
-        if (mConnections == null || mConnections.isEmpty()) {
+        if (mConnections == null || mConnections.isEmpty() || mIsDisconnecting) {
             return;
         }
-        // Capture and null out mConnections, to guarantee that we process
+        // Mark as disconnecting, to guarantee that we process
         // disconnect of these specific connections exactly once even if
         // we're racing with rapid activity lifecycle churn and this
         // method is invoked more than once on this object.
-        final Object disc = mConnections;
-        mConnections = null;
-        mService.mH.post(() -> mService.mAmInternal.disconnectActivityFromServices(this, disc));
+        // It is possible that {@link #removeConnection} is called while the disconnect-runnable is
+        // still in the message queue, so keep the reference of {@link #mConnections} to make sure
+        // the connection list is up-to-date.
+        mIsDisconnecting = true;
+        mService.mH.post(() -> {
+            mService.mAmInternal.disconnectActivityFromServices(this);
+            mIsDisconnecting = false;
+        });
     }
 
     public void dump(PrintWriter pw, String prefix) {
@@ -116,4 +142,9 @@ public class ActivityServiceConnectionsHolder<T> {
         }
     }
 
+    /** Used by {@link ActivityRecord#dump}. */
+    @Override
+    public String toString() {
+        return String.valueOf(mConnections);
+    }
 }