Have paused keepalives keep their hardware slot

When a keepalive is paused, it should sit on its hardware
slot for this network to make sure that it is possible to
resume later, even if it means other keepalives can't be
started at the same time.

Test: update AutomaticOnOffKeepaliveTrackerTest for this
Fixes: 268149573
Fixes: 283886067
Change-Id: Ida325bdea198d751483a83ee5d9ec26e39812137

service/src/com/android/server/connectivity/KeepaliveTracker.java

@@ -148,8 +148,9 @@ public class KeepaliveTracker {
         public static final int TYPE_TCP = 2;
 
         // Keepalive slot. A small integer that identifies this keepalive among the ones handled
-        // by this network.
-        private int mSlot = NO_KEEPALIVE;
+        // by this network. This is initialized to NO_KEEPALIVE for new keepalives, but to the
+        // old slot for resumed keepalives.
+        private int mSlot;
 
         // Packet data.
         private final KeepalivePacketData mPacket;
@@ -169,25 +170,30 @@ public class KeepaliveTracker {
                 int interval,
                 int type,
                 @Nullable FileDescriptor fd) throws InvalidSocketException {
-            this(callback, nai, packet, interval, type, fd, false /* resumed */);
+            this(callback, nai, packet, Binder.getCallingPid(), Binder.getCallingUid(), interval,
+                    type, fd, NO_KEEPALIVE /* slot */, false /* resumed */);
         }
 
         KeepaliveInfo(@NonNull ISocketKeepaliveCallback callback,
                 @NonNull NetworkAgentInfo nai,
                 @NonNull KeepalivePacketData packet,
+                int pid,
+                int uid,
                 int interval,
                 int type,
                 @Nullable FileDescriptor fd,
+                int slot,
                 boolean resumed) throws InvalidSocketException {
             mCallback = callback;
-            mPid = Binder.getCallingPid();
-            mUid = Binder.getCallingUid();
+            mPid = pid;
+            mUid = uid;
             mPrivileged = (PERMISSION_GRANTED == mContext.checkPermission(PERMISSION, mPid, mUid));
 
             mNai = nai;
             mPacket = packet;
             mInterval = interval;
             mType = type;
+            mSlot = slot;
             mResumed = resumed;
 
             // For SocketKeepalive, a dup of fd is kept in mFd so the source port from which the
@@ -468,8 +474,8 @@ public class KeepaliveTracker {
          * Construct a new KeepaliveInfo from existing KeepaliveInfo with a new fd.
          */
         public KeepaliveInfo withFd(@NonNull FileDescriptor fd) throws InvalidSocketException {
-            return new KeepaliveInfo(mCallback, mNai, mPacket, mInterval, mType, fd,
-                    true /* resumed */);
+            return new KeepaliveInfo(mCallback, mNai, mPacket, mPid, mUid, mInterval, mType,
+                    fd, mSlot, true /* resumed */);
         }
     }
 
@@ -508,7 +514,9 @@ public class KeepaliveTracker {
      */
     public int handleStartKeepalive(KeepaliveInfo ki) {
         NetworkAgentInfo nai = ki.getNai();
-        int slot = findFirstFreeSlot(nai);
+        // If this was a paused keepalive, then reuse the same slot that was kept for it. Otherwise,
+        // use the first free slot for this network agent.
+        final int slot = NO_KEEPALIVE != ki.mSlot ? ki.mSlot : findFirstFreeSlot(nai);
         mKeepalives.get(nai).put(slot, ki);
         return ki.start(slot);
     }
@@ -518,6 +526,8 @@ public class KeepaliveTracker {
         if (networkKeepalives != null) {
             final ArrayList<KeepaliveInfo> kalist = new ArrayList(networkKeepalives.values());
             for (KeepaliveInfo ki : kalist) {
+                // Check if keepalive is already stopped
+                if (ki.mStopReason == SUCCESS_PAUSED) continue;
                 ki.stop(reason);
                 // Clean up keepalives since the network agent is disconnected and unable to pass
                 // back asynchronous result of stop().
@@ -556,17 +566,22 @@ public class KeepaliveTracker {
             return;
         }
 
-        // Remove the keepalive from hash table so the slot can be considered available when reusing
-        // it.
-        networkKeepalives.remove(slot);
-        Log.d(TAG, "Remove keepalive " + slot + " on " + networkName + ", "
-                + networkKeepalives.size() + " remains.");
+        // If the keepalive was stopped for good, remove it from the hash table so the slot can
+        // be considered available when reusing it. If it was only a pause, let it sit in the map
+        // so it sits on the slot.
+        final int reason = ki.mStopReason;
+        if (reason != SUCCESS_PAUSED) {
+            networkKeepalives.remove(slot);
+            Log.d(TAG, "Remove keepalive " + slot + " on " + networkName + ", "
+                    + networkKeepalives.size() + " remains.");
+        } else {
+            Log.d(TAG, "Pause keepalive " + slot + " on " + networkName + ", keep slot reserved");
+        }
         if (networkKeepalives.isEmpty()) {
             mKeepalives.remove(nai);
         }
 
         // Notify app that the keepalive is stopped.
-        final int reason = ki.mStopReason;
         if (reason == SUCCESS) {
             try {
                 ki.mCallback.onStopped();
@@ -612,7 +627,8 @@ public class KeepaliveTracker {
     /**
      * Finalize a paused keepalive.
      *
-     * This will send the appropriate callback after checking that this keepalive is indeed paused.
+     * This will send the appropriate callback after checking that this keepalive is indeed paused,
+     * and free the slot.
      *
      * @param ki the keepalive to finalize
      * @param reason the reason the keepalive is stopped
@@ -630,6 +646,13 @@ public class KeepaliveTracker {
         } else {
             notifyErrorCallback(ki.mCallback, reason);
         }
+
+        final HashMap<Integer, KeepaliveInfo> networkKeepalives = mKeepalives.get(ki.mNai);
+        if (networkKeepalives == null) {
+            Log.e(TAG, "Attempt to finalize keepalive on nonexistent network " + ki.mNai);
+            return;
+        }
+        networkKeepalives.remove(ki.mSlot);
     }
 
     /**

tests/unit/java/com/android/server/connectivity/AutomaticOnOffKeepaliveTrackerTest.java

@@ -692,6 +692,10 @@ public class AutomaticOnOffKeepaliveTrackerTest {
         assertNull(getAutoKiForBinder(testInfo.binder));
 
         verifyNoMoreInteractions(ignoreStubs(testInfo.socketKeepaliveCallback));
+
+        // Make sure the slot is free
+        final TestKeepaliveInfo testInfo2 = doStartNattKeepalive();
+        checkAndProcessKeepaliveStart(testInfo2.kpd);
     }
 
     @Test
@@ -820,36 +824,34 @@ public class AutomaticOnOffKeepaliveTrackerTest {
 
         clearInvocations(mNai);
         // Start the second keepalive while the first is paused.
-        // TODO: Uncomment the following test after fixing b/283886067. Currently this attempts to
-        // start the keepalive on TEST_SLOT and this throws in the handler thread.
-        // final TestKeepaliveInfo testInfo2 = doStartNattKeepalive();
-        // // The slot used is TEST_SLOT + 1 since TEST_SLOT is being taken by the paused keepalive.
-        // checkAndProcessKeepaliveStart(TEST_SLOT + 1, testInfo2.kpd);
-        // verify(testInfo2.socketKeepaliveCallback).onStarted();
-        // assertNotNull(getAutoKiForBinder(testInfo2.binder));
+        final TestKeepaliveInfo testInfo2 = doStartNattKeepalive();
+        // The slot used is TEST_SLOT + 1 since TEST_SLOT is being taken by the paused keepalive.
+        checkAndProcessKeepaliveStart(TEST_SLOT + 1, testInfo2.kpd);
+        verify(testInfo2.socketKeepaliveCallback).onStarted();
+        assertNotNull(getAutoKiForBinder(testInfo2.binder));
 
-        // clearInvocations(mNai);
-        // doResumeKeepalive(autoKi1);
-        // // Resume on TEST_SLOT.
-        // checkAndProcessKeepaliveStart(TEST_SLOT, testInfo1.kpd);
-        // verify(testInfo1.socketKeepaliveCallback).onResumed();
+        clearInvocations(mNai);
+        doResumeKeepalive(autoKi1);
+        // Resume on TEST_SLOT.
+        checkAndProcessKeepaliveStart(TEST_SLOT, testInfo1.kpd);
+        verify(testInfo1.socketKeepaliveCallback).onResumed();
 
-        // clearInvocations(mNai);
-        // doStopKeepalive(autoKi1);
-        // checkAndProcessKeepaliveStop(TEST_SLOT);
-        // verify(testInfo1.socketKeepaliveCallback).onStopped();
-        // verify(testInfo2.socketKeepaliveCallback, never()).onStopped();
-        // assertNull(getAutoKiForBinder(testInfo1.binder));
+        clearInvocations(mNai);
+        doStopKeepalive(autoKi1);
+        checkAndProcessKeepaliveStop(TEST_SLOT);
+        verify(testInfo1.socketKeepaliveCallback).onStopped();
+        verify(testInfo2.socketKeepaliveCallback, never()).onStopped();
+        assertNull(getAutoKiForBinder(testInfo1.binder));
 
-        // clearInvocations(mNai);
-        // assertNotNull(getAutoKiForBinder(testInfo2.binder));
-        // doStopKeepalive(getAutoKiForBinder(testInfo2.binder));
-        // checkAndProcessKeepaliveStop(TEST_SLOT + 1);
-        // verify(testInfo2.socketKeepaliveCallback).onStopped();
-        // assertNull(getAutoKiForBinder(testInfo2.binder));
+        clearInvocations(mNai);
+        assertNotNull(getAutoKiForBinder(testInfo2.binder));
+        doStopKeepalive(getAutoKiForBinder(testInfo2.binder));
+        checkAndProcessKeepaliveStop(TEST_SLOT + 1);
+        verify(testInfo2.socketKeepaliveCallback).onStopped();
+        assertNull(getAutoKiForBinder(testInfo2.binder));
 
-        // verifyNoMoreInteractions(ignoreStubs(testInfo1.socketKeepaliveCallback));
-        // verifyNoMoreInteractions(ignoreStubs(testInfo2.socketKeepaliveCallback));
+        verifyNoMoreInteractions(ignoreStubs(testInfo1.socketKeepaliveCallback));
+        verifyNoMoreInteractions(ignoreStubs(testInfo2.socketKeepaliveCallback));
     }
 
     @Test