Simplify the return condition in stop()
Previously, the return condition in stop() will check if the state
is STOPPING and the reason is ERROR_INVALID_NETWORK. The condition
is too restricted so that if another event is happened after binder
died, the exception will be thrown and crash the system.
Since calling stop() twice doesn't make sense, so relax the condition
of return when the state is STOPPING.
Bug: 182586681
Test: atest FrameworksNetTests
Change-Id: I2454c1c080d8954dd3785d4ac6e96fc4131fdb47
diff --git a/service/src/com/android/server/connectivity/KeepaliveTracker.java b/service/src/com/android/server/connectivity/KeepaliveTracker.java
index acf39f0..ee1538a 100644
--- a/service/src/com/android/server/connectivity/KeepaliveTracker.java
+++ b/service/src/com/android/server/connectivity/KeepaliveTracker.java
@@ -373,12 +373,10 @@
Log.e(TAG, "Cannot stop unowned keepalive " + mSlot + " on " + mNai.network);
}
}
- // Ignore the case when the network disconnects immediately after stop() has been
- // called and the keepalive code is waiting for the response from the modem. This
- // might happen when the caller listens for a lower-layer network disconnect
- // callback and stop the keepalive at that time. But the stop() races with the
- // stop() generated in ConnectivityService network disconnection code path.
- if (mStartedState == STOPPING && reason == ERROR_INVALID_NETWORK) return;
+ // To prevent races from re-entrance of stop(), return if the state is already stopping.
+ // This might happen if multiple event sources stop keepalive in a short time. Such as
+ // network disconnect after user calls stop(), or tear down socket after binder died.
+ if (mStartedState == STOPPING) return;
// Store the reason of stopping, and report it after the keepalive is fully stopped.
if (mStopReason != ERROR_STOP_REASON_UNINITIALIZED) {
diff --git a/tests/unit/java/com/android/server/ConnectivityServiceTest.java b/tests/unit/java/com/android/server/ConnectivityServiceTest.java
index 134078d..d13421f 100644
--- a/tests/unit/java/com/android/server/ConnectivityServiceTest.java
+++ b/tests/unit/java/com/android/server/ConnectivityServiceTest.java
@@ -5450,6 +5450,8 @@
// the follow-up network disconnection will be processed first.
mWiFiNetworkAgent.setKeepaliveResponseDelay(3 * TIMEOUT_MS);
ka.stop();
+ // Call stop() twice shouldn't result in crash, b/182586681.
+ ka.stop();
// Make sure the stop has been processed. Wait for executor idle is needed to prevent
// flaky since the actual stop call to the service is delegated to executor thread.
