Merge "Have paused keepalives keep their hardware slot"

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

@@ -150,8 +150,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;
@@ -171,25 +172,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
@@ -470,8 +476,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 */);
         }
     }
 
@@ -510,7 +516,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);
     }
@@ -520,6 +528,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().
@@ -558,17 +568,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();
@@ -614,7 +629,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
@@ -632,6 +648,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