diff --git a/service/src/com/android/server/connectivity/KeepaliveTracker.java b/service/src/com/android/server/connectivity/KeepaliveTracker.java index b4f74d5e5c..cbf091b0c8 100644 --- a/service/src/com/android/server/connectivity/KeepaliveTracker.java +++ b/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 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 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); } /** diff --git a/tests/unit/java/com/android/server/connectivity/AutomaticOnOffKeepaliveTrackerTest.java b/tests/unit/java/com/android/server/connectivity/AutomaticOnOffKeepaliveTrackerTest.java index 82326585fc..0b20227d78 100644 --- a/tests/unit/java/com/android/server/connectivity/AutomaticOnOffKeepaliveTrackerTest.java +++ b/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