Eliminate acquiring WM lock when bind/unbind service

There were lock contention between AM and WM when an activity
binds/unbinds service from:
- Associate the connection to the activity
- Add connection
- Remove connection
when WM is running layout/animation or updating config.

This change applies:
- Use a separated lock to manage the connection.
- Cache the visible state for BIND_ADJUST_WITH_ACTIVITY.
to eliminate the lock contention.

Bug: 263755904
Bug: 204870457
Test: ActivityRecordTests#testActivityServiceConnectionsHolder
Change-Id: Id5b7862866180064cb8ec863cd9cd6ccebdaea1e

5 files changed, 83 insertions, 41 deletions

diff --git a/services/core/java/com/android/server/wm/ActivityRecord.java b/services/core/java/com/android/server/wm/ActivityRecord.java
index df33163016eb..5f8e01e583c4 100644
--- a/services/core/java/com/android/server/wm/ActivityRecord.java
+++ b/services/core/java/com/android/server/wm/ActivityRecord.java
@@ -341,6 +341,7 @@ import android.window.TransitionInfo.AnimationOptions;
 import android.window.WindowContainerToken;
 
 import com.android.internal.R;
+import com.android.internal.annotations.GuardedBy;
 import com.android.internal.annotations.VisibleForTesting;
 import com.android.internal.app.ResolverActivity;
 import com.android.internal.content.ReferrerIntent;
@@ -503,7 +504,10 @@ final class ActivityRecord extends WindowToken implements WindowManagerService.A
     private RemoteTransition mPendingRemoteTransition;
     ActivityOptions returningOptions; // options that are coming back via convertToTranslucent
     AppTimeTracker appTimeTracker; // set if we are tracking the time in this app/task/activity
+    @GuardedBy("this")
     ActivityServiceConnectionsHolder mServiceConnectionsHolder; // Service connections.
+    /** @see android.content.Context#BIND_ADJUST_WITH_ACTIVITY */
+    volatile boolean mVisibleForServiceConnection;
     UriPermissionOwner uriPermissions; // current special URI access perms.
     WindowProcessController app;      // if non-null, hosting application
     private State mState;    // current state we are in
@@ -553,7 +557,8 @@ final class ActivityRecord extends WindowToken implements WindowManagerService.A
     long lastLaunchTime;    // time of last launch of this activity
     ComponentName requestedVrComponent; // the requested component for handling VR mode.
 
-    boolean inHistory;  // are we in the history task?
+    /** Whether this activity is reachable from hierarchy. */
+    volatile boolean inHistory;
     final ActivityTaskSupervisor mTaskSupervisor;
     final RootWindowContainer mRootWindowContainer;
 
@@ -1904,7 +1909,9 @@ final class ActivityRecord extends WindowToken implements WindowManagerService.A
         }
     }
 
-    static @Nullable ActivityRecord forTokenLocked(IBinder token) {
+    /** Gets the corresponding record by the token. Note that it may not exist in the hierarchy. */
+    @Nullable
+    static ActivityRecord forToken(IBinder token) {
         if (token == null) return null;
         final Token activityToken;
         try {
@@ -1913,7 +1920,11 @@ final class ActivityRecord extends WindowToken implements WindowManagerService.A
             Slog.w(TAG, "Bad activity token: " + token, e);
             return null;
         }
-        final ActivityRecord r = activityToken.mActivityRef.get();
+        return activityToken.mActivityRef.get();
+    }
+
+    static @Nullable ActivityRecord forTokenLocked(IBinder token) {
+        final ActivityRecord r = forToken(token);
         return r == null || r.getRootTask() == null ? null : r;
     }
 
@@ -4036,17 +4047,32 @@ final class ActivityRecord extends WindowToken implements WindowManagerService.A
         mDisplayContent.getDisplayPolicy().removeRelaunchingApp(this);
     }
 
+    ActivityServiceConnectionsHolder getOrCreateServiceConnectionsHolder() {
+        synchronized (this) {
+            if (mServiceConnectionsHolder == null) {
+                mServiceConnectionsHolder = new ActivityServiceConnectionsHolder(this);
+            }
+            return mServiceConnectionsHolder;
+        }
+    }
+
     /**
      * Perform clean-up of service connections in an activity record.
      */
     private void cleanUpActivityServices() {
-        if (mServiceConnectionsHolder == null) {
-            return;
+        synchronized (this) {
+            if (mServiceConnectionsHolder == null) {
+                return;
+            }
+            // 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;
         }
-        // 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;
+    }
+
+    private void updateVisibleForServiceConnection() {
+        mVisibleForServiceConnection = mVisibleRequested || mState == RESUMED || mState == PAUSING;
     }
 
     /**
@@ -5139,6 +5165,7 @@ final class ActivityRecord extends WindowToken implements WindowManagerService.A
             taskFragment.onActivityVisibleRequestedChanged();
         }
         setInsetsFrozen(!visible);
+        updateVisibleForServiceConnection();
         if (app != null) {
             mTaskSupervisor.onProcessActivityStateChanged(app, false /* forceBatch */);
         }
@@ -5617,6 +5644,7 @@ final class ActivityRecord extends WindowToken implements WindowManagerService.A
                 return;
             }
         }
+        updateVisibleForServiceConnection();
         if (app != null) {
             mTaskSupervisor.onProcessActivityStateChanged(app, false /* forceBatch */);
         }
diff --git a/services/core/java/com/android/server/wm/ActivityServiceConnectionsHolder.java b/services/core/java/com/android/server/wm/ActivityServiceConnectionsHolder.java
index 0859d40c0fd1..5f56af7fd4e0 100644
--- a/services/core/java/com/android/server/wm/ActivityServiceConnectionsHolder.java
+++ b/services/core/java/com/android/server/wm/ActivityServiceConnectionsHolder.java
@@ -16,14 +16,14 @@
 
 package com.android.server.wm;
 
-import static com.android.server.wm.ActivityRecord.State.PAUSING;
-import static com.android.server.wm.ActivityRecord.State.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 com.android.internal.annotations.GuardedBy;
+
 import java.io.PrintWriter;
 import java.util.function.Consumer;
 
@@ -38,8 +38,6 @@ import java.util.function.Consumer;
  */
 public class ActivityServiceConnectionsHolder<T> {
 
-    private final ActivityTaskManagerService mService;
-
     /** The activity the owns this service connection object. */
     private final ActivityRecord mActivity;
 
@@ -49,19 +47,19 @@ 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.
      */
+    @GuardedBy("mActivity")
     private ArraySet<T> mConnections;
 
     /** Whether all connections of {@link #mActivity} are being removed. */
     private volatile boolean mIsDisconnecting;
 
-    ActivityServiceConnectionsHolder(ActivityTaskManagerService service, ActivityRecord activity) {
-        mService = service;
+    ActivityServiceConnectionsHolder(ActivityRecord activity) {
         mActivity = activity;
     }
 
     /** Adds a connection record that the activity has bound to a specific service. */
     public void addConnection(T c) {
-        synchronized (mService.mGlobalLock) {
+        synchronized (mActivity) {
             if (mIsDisconnecting) {
                 // This is unlikely to happen because the caller should create a new holder.
                 if (DEBUG_CLEANUP) {
@@ -79,7 +77,7 @@ public class ActivityServiceConnectionsHolder<T> {
 
     /** Removed a connection record between the activity and a specific service. */
     public void removeConnection(T c) {
-        synchronized (mService.mGlobalLock) {
+        synchronized (mActivity) {
             if (mConnections == null) {
                 return;
             }
@@ -90,20 +88,18 @@ public class ActivityServiceConnectionsHolder<T> {
         }
     }
 
+    /** @see android.content.Context#BIND_ADJUST_WITH_ACTIVITY */
     public boolean isActivityVisible() {
-        synchronized (mService.mGlobalLock) {
-            return mActivity.isVisibleRequested() || mActivity.isState(RESUMED, PAUSING);
-        }
+        return mActivity.mVisibleForServiceConnection;
     }
 
     public int getActivityPid() {
-        synchronized (mService.mGlobalLock) {
-            return mActivity.hasProcess() ? mActivity.app.getPid() : -1;
-        }
+        final WindowProcessController wpc = mActivity.app;
+        return wpc != null ? wpc.getPid() : -1;
     }
 
     public void forEachConnection(Consumer<T> consumer) {
-        synchronized (mService.mGlobalLock) {
+        synchronized (mActivity) {
             if (mConnections == null || mConnections.isEmpty()) {
                 return;
             }
@@ -118,6 +114,7 @@ public class ActivityServiceConnectionsHolder<T> {
      * general, this method is used to clean up if the activity didn't unbind services before it
      * is destroyed.
      */
+    @GuardedBy("mActivity")
     void disconnectActivityFromServices() {
         if (mConnections == null || mConnections.isEmpty() || mIsDisconnecting) {
             return;
@@ -130,16 +127,14 @@ public class ActivityServiceConnectionsHolder<T> {
         // 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);
+        mActivity.mAtmService.mH.post(() -> {
+            mActivity.mAtmService.mAmInternal.disconnectActivityFromServices(this);
             mIsDisconnecting = false;
         });
     }
 
     public void dump(PrintWriter pw, String prefix) {
-        synchronized (mService.mGlobalLock) {
-            pw.println(prefix + "activity=" + mActivity);
-        }
+        pw.println(prefix + "activity=" + mActivity);
     }
 
     /** Used by {@link ActivityRecord#dump}. */
diff --git a/services/core/java/com/android/server/wm/ActivityTaskManagerService.java b/services/core/java/com/android/server/wm/ActivityTaskManagerService.java
index 861961599ada..c527310abb14 100644
--- a/services/core/java/com/android/server/wm/ActivityTaskManagerService.java
+++ b/services/core/java/com/android/server/wm/ActivityTaskManagerService.java
@@ -6000,18 +6000,11 @@ public class ActivityTaskManagerService extends IActivityTaskManager.Stub {
 
         @Override
         public ActivityServiceConnectionsHolder getServiceConnectionsHolder(IBinder token) {
-            synchronized (mGlobalLock) {
-                final ActivityRecord r = ActivityRecord.isInRootTaskLocked(token);
-                if (r == null) {
-                    return null;
-                }
-                if (r.mServiceConnectionsHolder == null) {
-                    r.mServiceConnectionsHolder = new ActivityServiceConnectionsHolder(
-                            ActivityTaskManagerService.this, r);
-                }
-
-                return r.mServiceConnectionsHolder;
+            final ActivityRecord r = ActivityRecord.forToken(token);
+            if (r == null || !r.inHistory) {
+                return null;
             }
+            return r.getOrCreateServiceConnectionsHolder();
         }
 
         @Override
diff --git a/services/core/java/com/android/server/wm/TaskFragment.java b/services/core/java/com/android/server/wm/TaskFragment.java
index 22da56ad4d3b..19e45ff02463 100644
--- a/services/core/java/com/android/server/wm/TaskFragment.java
+++ b/services/core/java/com/android/server/wm/TaskFragment.java
@@ -1953,7 +1953,7 @@ class TaskFragment extends WindowContainer<WindowContainer> {
                         1f);
                 mBackScreenshots.put(r.mActivityComponent.flattenToString(), backBuffer);
             }
-            child.asActivityRecord().inHistory = true;
+            addingActivity.inHistory = true;
             task.onDescendantActivityAdded(taskHadActivity, activityType, addingActivity);
         }
     }
diff --git a/services/tests/wmtests/src/com/android/server/wm/ActivityRecordTests.java b/services/tests/wmtests/src/com/android/server/wm/ActivityRecordTests.java
index a017bd6a9436..ea50179746d8 100644
--- a/services/tests/wmtests/src/com/android/server/wm/ActivityRecordTests.java
+++ b/services/tests/wmtests/src/com/android/server/wm/ActivityRecordTests.java
@@ -2324,6 +2324,32 @@ public class ActivityRecordTests extends WindowTestsBase {
     }
 
     @Test
+    public void testActivityServiceConnectionsHolder() {
+        final ActivityRecord activity = new ActivityBuilder(mAtm).setCreateTask(true).build();
+        final ActivityServiceConnectionsHolder<Object> holder =
+                mAtm.mInternal.getServiceConnectionsHolder(activity.token);
+        assertNotNull(holder);
+        final Object connection = new Object();
+        holder.addConnection(connection);
+        assertTrue(holder.isActivityVisible());
+        final int[] count = new int[1];
+        final Consumer<Object> c = conn -> count[0]++;
+        holder.forEachConnection(c);
+        assertEquals(1, count[0]);
+
+        holder.removeConnection(connection);
+        holder.forEachConnection(c);
+        assertEquals(1, count[0]);
+
+        activity.setVisibleRequested(false);
+        activity.setState(STOPPED, "test");
+        assertFalse(holder.isActivityVisible());
+
+        activity.removeImmediately();
+        assertNull(mAtm.mInternal.getServiceConnectionsHolder(activity.token));
+    }
+
+    @Test
     public void testTransferLaunchCookieWhenFinishing() {
         final ActivityRecord activity1 = createActivityWithTask();
         final Binder launchCookie = new Binder();