WifiConfigManager: Cleanup the pending store read logic
I had assumed that user unlock would always come after boot completed,
but it turns out that it's not always the case (especially for non
encrypted devices).
Couple of flags added to handle this race:
1. mPendingStoreRead: Used to indicate if we have invoked
|loadFromStore| or |migrateFromStore| once. This flag is used to defer
any user unlock or switching handling untill we do load from store.
2. mDeferredUserUnlockRead: Used to indicate if we deferred a user
unlock handling because we had not yet loaded from store.
User switching requires a UI interaction and cannot occur until the
BOOT_COMPLETED broadcast is received. So, there is no need to handle
that. A wtf was added to alert for out of order notifications.
Bug: 34741678
Test: Added Unit tests.
Change-Id: Id17d233f8da2540210b84cbd313b2a3efff90d70
(cherry picked from commit d7e826a86f845e439715eae57d43731d1ca9a404)
diff --git a/service/java/com/android/server/wifi/WifiConfigManager.java b/service/java/com/android/server/wifi/WifiConfigManager.java
index 1424a8c..e4bf1e4 100644
--- a/service/java/com/android/server/wifi/WifiConfigManager.java
+++ b/service/java/com/android/server/wifi/WifiConfigManager.java
@@ -262,6 +262,16 @@
*/
private boolean mPendingUnlockStoreRead = true;
/**
+ * Flag to indicate if we have performed a read from store at all. This is used to gate
+ * any user unlock/switch operations until we read the store (Will happen if wifi is disabled
+ * when user updates from N to O).
+ */
+ private boolean mPendingStoreRead = true;
+ /**
+ * Flag to indicate if the user unlock was deferred until the store load occurs.
+ */
+ private boolean mDeferredUserUnlockRead = false;
+ /**
* This is keeping track of the next network ID to be assigned. Any new networks will be
* assigned |mNextNetworkId| as network ID.
*/
@@ -2293,12 +2303,6 @@
if (mVerboseLoggingEnabled) {
Log.v(TAG, "Loading from store after user switch/unlock for " + userId);
}
- // Don't handle user unlock or switch unless the migration from legacy stores is complete.
- if (mWifiConfigStoreLegacy.areStoresPresent() && !mWifiConfigStore.areStoresPresent()) {
- Log.d(TAG, "Legacy store files found. Ignore user switch/unlock until migration is "
- + "complete!");
- return;
- }
// Switch out the user store file.
if (loadFromUserStoreAfterUnlockOrSwitch(userId)) {
saveToStore(true);
@@ -2328,6 +2332,10 @@
Log.w(TAG, "User already in foreground " + userId);
return new HashSet<>();
}
+ if (mPendingStoreRead) {
+ Log.wtf(TAG, "Unexpected user switch before store is read!");
+ return new HashSet<>();
+ }
if (mUserManager.isUserUnlockingOrUnlocked(mCurrentUserId)) {
saveToStore(true);
}
@@ -2358,6 +2366,11 @@
if (mVerboseLoggingEnabled) {
Log.v(TAG, "Handling user unlock for " + userId);
}
+ if (mPendingStoreRead) {
+ Log.w(TAG, "Ignore user unlock until store is read!");
+ mDeferredUserUnlockRead = true;
+ return;
+ }
if (userId == mCurrentUserId && mPendingUnlockStoreRead) {
handleUserUnlockOrSwitch(mCurrentUserId);
}
@@ -2483,6 +2496,7 @@
Log.w(TAG, "No stored networks found.");
}
sendConfiguredNetworksChangedBroadcast();
+ mPendingStoreRead = false;
}
/**
@@ -2539,6 +2553,13 @@
}
loadInternalData(storeData.getSharedConfigurations(), storeData.getUserConfigurations(),
storeData.getDeletedEphemeralSSIDs());
+ // If the user unlock comes in before we load from store, we defer the handling until
+ // the load from store is triggered.
+ if (mDeferredUserUnlockRead) {
+ Log.i(TAG, "Handling user unlock after loading from store.");
+ handleUserUnlockOrSwitch(mCurrentUserId);
+ mDeferredUserUnlockRead = false;
+ }
return true;
}
diff --git a/tests/wifitests/src/com/android/server/wifi/WifiConfigManagerTest.java b/tests/wifitests/src/com/android/server/wifi/WifiConfigManagerTest.java
index a52e92d..9ea2cad 100644
--- a/tests/wifitests/src/com/android/server/wifi/WifiConfigManagerTest.java
+++ b/tests/wifitests/src/com/android/server/wifi/WifiConfigManagerTest.java
@@ -173,6 +173,9 @@
.thenReturn(true);
when(mWifiConfigStore.areStoresPresent()).thenReturn(true);
+ when(mWifiConfigStore.read())
+ .thenReturn(new WifiConfigStoreData(new ArrayList<WifiConfiguration>(),
+ new ArrayList<WifiConfiguration>(), new HashSet<String>()));
when(mDevicePolicyManagerInternal.isActiveAdminWithPolicy(anyInt(), anyInt()))
.thenReturn(false);
@@ -2057,6 +2060,9 @@
int user2 = TEST_DEFAULT_USER + 1;
setupUserProfiles(user2);
+ // Set up the internal data first.
+ assertTrue(mWifiConfigManager.loadFromStore());
+
when(mWifiConfigStore.switchUserStoreAndRead(any(WifiConfigStore.StoreFile.class)))
.thenReturn(new WifiConfigStoreData(
new ArrayList<WifiConfiguration>(), new ArrayList<WifiConfiguration>(),
@@ -2080,6 +2086,9 @@
int user2 = TEST_DEFAULT_USER + 1;
setupUserProfiles(user2);
+ // Set up the internal data first.
+ assertTrue(mWifiConfigManager.loadFromStore());
+
// user2 is locked and switched to foreground.
when(mUserManager.isUserUnlockingOrUnlocked(user2)).thenReturn(false);
mWifiConfigManager.handleUserSwitch(user2);
@@ -2136,6 +2145,14 @@
public void testHandleUserUnlockAfterBootup() throws Exception {
int user1 = TEST_DEFAULT_USER;
+ // Set up the internal data first.
+ assertTrue(mWifiConfigManager.loadFromStore());
+ mContextConfigStoreMockOrder.verify(mWifiConfigStore).read();
+ mContextConfigStoreMockOrder.verify(mWifiConfigStore, never())
+ .write(anyBoolean(), any(WifiConfigStoreData.class));
+ mContextConfigStoreMockOrder.verify(mWifiConfigStore, never())
+ .switchUserStoreAndRead(any(WifiConfigStore.StoreFile.class));
+
when(mWifiConfigStore.switchUserStoreAndRead(any(WifiConfigStore.StoreFile.class)))
.thenReturn(new WifiConfigStoreData(
new ArrayList<WifiConfiguration>(), new ArrayList<WifiConfiguration>(),
@@ -2143,8 +2160,43 @@
// Unlock the user1 (default user) for the first time and ensure that we read the data.
mWifiConfigManager.handleUserUnlock(user1);
+ mContextConfigStoreMockOrder.verify(mWifiConfigStore, never()).read();
mContextConfigStoreMockOrder.verify(mWifiConfigStore)
.switchUserStoreAndRead(any(WifiConfigStore.StoreFile.class));
+ mContextConfigStoreMockOrder.verify(mWifiConfigStore)
+ .write(anyBoolean(), any(WifiConfigStoreData.class));
+ }
+
+ /**
+ * Verifies that the store read after bootup received after
+ * foreground user unlock via {@link WifiConfigManager#handleUserUnlock(int)}
+ * results in a user store read.
+ */
+ @Test
+ public void testHandleBootupAfterUserUnlock() throws Exception {
+ int user1 = TEST_DEFAULT_USER;
+
+ // Unlock the user1 (default user) for the first time and ensure that we don't read the
+ // data.
+ mWifiConfigManager.handleUserUnlock(user1);
+ mContextConfigStoreMockOrder.verify(mWifiConfigStore, never()).read();
+ mContextConfigStoreMockOrder.verify(mWifiConfigStore, never())
+ .write(anyBoolean(), any(WifiConfigStoreData.class));
+ mContextConfigStoreMockOrder.verify(mWifiConfigStore, never())
+ .switchUserStoreAndRead(any(WifiConfigStore.StoreFile.class));
+
+ when(mWifiConfigStore.switchUserStoreAndRead(any(WifiConfigStore.StoreFile.class)))
+ .thenReturn(new WifiConfigStoreData(
+ new ArrayList<WifiConfiguration>(), new ArrayList<WifiConfiguration>(),
+ new HashSet<String>()));
+
+ // Read from store now.
+ assertTrue(mWifiConfigManager.loadFromStore());
+ mContextConfigStoreMockOrder.verify(mWifiConfigStore).read();
+ mContextConfigStoreMockOrder.verify(mWifiConfigStore)
+ .switchUserStoreAndRead(any(WifiConfigStore.StoreFile.class));
+ mContextConfigStoreMockOrder.verify(mWifiConfigStore)
+ .write(anyBoolean(), any(WifiConfigStoreData.class));
}
/**
@@ -2157,6 +2209,9 @@
int user2 = TEST_DEFAULT_USER + 1;
setupUserProfiles(user2);
+ // Set up the internal data first.
+ assertTrue(mWifiConfigManager.loadFromStore());
+
when(mWifiConfigStore.switchUserStoreAndRead(any(WifiConfigStore.StoreFile.class)))
.thenReturn(new WifiConfigStoreData(
new ArrayList<WifiConfiguration>(), new ArrayList<WifiConfiguration>(),
@@ -2183,16 +2238,13 @@
public void testHandleUserSwitchAfterBootupBeforeLegacyStoreMigration() throws Exception {
int user2 = TEST_DEFAULT_USER + 1;
- when(mWifiConfigStoreLegacy.areStoresPresent()).thenReturn(true);
- when(mWifiConfigStore.areStoresPresent()).thenReturn(false);
-
// Switch to user2 for the first time and ensure that we don't read or
// write the store files.
when(mUserManager.isUserUnlockingOrUnlocked(user2)).thenReturn(false);
mWifiConfigManager.handleUserSwitch(user2);
mContextConfigStoreMockOrder.verify(mWifiConfigStore, never())
.switchUserStoreAndRead(any(WifiConfigStore.StoreFile.class));
- mContextConfigStoreMockOrder.verify(mWifiConfigStore, times(1))
+ mContextConfigStoreMockOrder.verify(mWifiConfigStore, never())
.write(anyBoolean(), any(WifiConfigStoreData.class));
}
@@ -2204,9 +2256,6 @@
public void testHandleUserUnlockAfterBootupBeforeLegacyStoreMigration() throws Exception {
int user1 = TEST_DEFAULT_USER;
- when(mWifiConfigStoreLegacy.areStoresPresent()).thenReturn(true);
- when(mWifiConfigStore.areStoresPresent()).thenReturn(false);
-
// Unlock the user1 (default user) for the first time and ensure that we don't read or
// write the store files.
mWifiConfigManager.handleUserUnlock(user1);
