Merge "Resolve race condition by guaranteeing that onDestroyed is first on the right queue" into sc-mainline-prod
diff --git a/service/java/com/android/server/wifi/HalDeviceManager.java b/service/java/com/android/server/wifi/HalDeviceManager.java
index b664c1f..e87c38d 100644
--- a/service/java/com/android/server/wifi/HalDeviceManager.java
+++ b/service/java/com/android/server/wifi/HalDeviceManager.java
@@ -57,6 +57,7 @@
import java.util.Arrays;
import java.util.HashMap;
import java.util.HashSet;
+import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Set;
@@ -2367,6 +2368,11 @@
return false;
}
+ // dispatch listeners on other threads to prevent race conditions in case the HAL is
+ // blocking and they get notification about destruction from HAL before cleaning up
+ // status.
+ dispatchDestroyedListeners(name, type, true);
+
WifiStatus status = null;
try {
switch (type) {
@@ -2391,7 +2397,7 @@
}
// dispatch listeners no matter what status
- dispatchDestroyedListeners(name, type);
+ dispatchDestroyedListeners(name, type, false);
if (validateRttController) {
// Try to update the RttController
updateRttControllerWhenInterfaceChanges();
@@ -2407,10 +2413,13 @@
}
// dispatch all destroyed listeners registered for the specified interface AND remove the
- // cache entry
- private void dispatchDestroyedListeners(String name, int type) {
+ // cache entries for the called listeners
+ // onlyOnOtherThreads = true: only call listeners on other threads
+ // onlyOnOtherThreads = false: call all listeners
+ private void dispatchDestroyedListeners(String name, int type, boolean onlyOnOtherThreads) {
if (VDBG) Log.d(TAG, "dispatchDestroyedListeners: iface(name)=" + name);
+ List<InterfaceDestroyedListenerProxy> triggerList = new ArrayList<>();
synchronized (mLock) {
InterfaceCacheEntry entry = mInterfaceInfoCache.get(Pair.create(name, type));
if (entry == null) {
@@ -2418,11 +2427,22 @@
return;
}
- for (InterfaceDestroyedListenerProxy listener : entry.destroyedListeners) {
- listener.trigger();
+ Iterator<InterfaceDestroyedListenerProxy> iterator =
+ entry.destroyedListeners.iterator();
+ while (iterator.hasNext()) {
+ InterfaceDestroyedListenerProxy listener = iterator.next();
+ if (!onlyOnOtherThreads || !listener.requestedToRunInCurrentThread()) {
+ triggerList.add(listener);
+ iterator.remove();
+ }
}
- entry.destroyedListeners.clear(); // for insurance (though cache entry is removed)
- mInterfaceInfoCache.remove(Pair.create(name, type));
+ if (!onlyOnOtherThreads) { // leave entry until final call to *all* callbacks
+ mInterfaceInfoCache.remove(Pair.create(name, type));
+ }
+ }
+
+ for (InterfaceDestroyedListenerProxy listener : triggerList) {
+ listener.trigger();
}
}
@@ -2462,44 +2482,34 @@
return mListener.hashCode();
}
- void trigger() {
- if (mHandler != null) {
- // TODO(b/199792691): The thread check is needed to preserve the existing
- // assumptions of synchronous execution of the "onDestroyed" callback as much as
- // possible. This is needed to prevent regressions caused by posting to the handler
- // thread changing the code execution order.
- // When all wifi services (ie. WifiAware, WifiP2p) get moved to the wifi handler
- // thread, remove this thread check and the Handler#post() and simply always
- // invoke the callback directly.
- long currentTid = mWifiInjector.getCurrentThreadId();
- long handlerTid = mHandler.getLooper().getThread().getId();
- if (currentTid == handlerTid) {
- // Already running on the same handler thread. Trigger listener synchronously.
- action();
- } else {
- // Current thread is not the thread the listener should be invoked on.
- // Post action to the intended thread.
- mHandler.post(() -> {
- action();
- });
- }
- } else {
- action();
- }
+ public boolean requestedToRunInCurrentThread() {
+ if (mHandler == null) return true;
+ long currentTid = mWifiInjector.getCurrentThreadId();
+ long handlerTid = mHandler.getLooper().getThread().getId();
+ return currentTid == handlerTid;
}
- void triggerWithArg(boolean arg) {
- if (mHandler != null) {
- mHandler.post(() -> {
- actionWithArg(arg);
- });
+ void trigger() {
+ // TODO(b/199792691): The thread check is needed to preserve the existing
+ // assumptions of synchronous execution of the "onDestroyed" callback as much as
+ // possible. This is needed to prevent regressions caused by posting to the handler
+ // thread changing the code execution order.
+ // When all wifi services (ie. WifiAware, WifiP2p) get moved to the wifi handler
+ // thread, remove this thread check and the Handler#post() and simply always
+ // invoke the callback directly.
+ if (requestedToRunInCurrentThread()) {
+ // Already running on the same handler thread. Trigger listener synchronously.
+ action();
} else {
- actionWithArg(arg);
+ // Current thread is not the thread the listener should be invoked on.
+ // Post action to the intended thread.
+ mHandler.postAtFrontOfQueue(() -> {
+ action();
+ });
}
}
protected void action() {}
- protected void actionWithArg(boolean arg) {}
ListenerProxy(LISTENER listener, Handler handler, String tag) {
mListener = listener;
diff --git a/service/java/com/android/server/wifi/WifiNative.java b/service/java/com/android/server/wifi/WifiNative.java
index b07101c..33746bc 100644
--- a/service/java/com/android/server/wifi/WifiNative.java
+++ b/service/java/com/android/server/wifi/WifiNative.java
@@ -133,6 +133,7 @@
mSupplicantStaIfaceHal.enableVerboseLogging(mVerboseLoggingEnabled);
mHostapdHal.enableVerboseLogging(mVerboseLoggingEnabled);
mWifiVendorHal.enableVerboseLogging(mVerboseLoggingEnabled);
+ mIfaceMgr.enableVerboseLogging(mVerboseLoggingEnabled);
}
/**
@@ -300,9 +301,17 @@
private int mNextId;
/** Map of the id to the iface structure */
private HashMap<Integer, Iface> mIfaces = new HashMap<>();
+ private boolean mVerboseLoggingEnabled = false;
+
+ public void enableVerboseLogging(boolean enable) {
+ mVerboseLoggingEnabled = enable;
+ }
/** Allocate a new iface for the given type */
- private Iface allocateIface(@Iface.IfaceType int type) {
+ private Iface allocateIface(@Iface.IfaceType int type) {
+ if (mVerboseLoggingEnabled) {
+ Log.d(TAG, "IfaceManager#allocateIface: type=" + type + ", pre-map=" + mIfaces);
+ }
Iface iface = new Iface(mNextId, type);
mIfaces.put(mNextId, iface);
mNextId++;
@@ -311,6 +320,9 @@
/** Remove the iface using the provided id */
private Iface removeIface(int id) {
+ if (mVerboseLoggingEnabled) {
+ Log.d(TAG, "IfaceManager#removeIface: id=" + id + ", pre-map=" + mIfaces);
+ }
return mIfaces.remove(id);
}
@@ -387,6 +399,10 @@
/** Removes the existing iface that does not match the provided id. */
public Iface removeExistingIface(int newIfaceId) {
+ if (mVerboseLoggingEnabled) {
+ Log.d(TAG, "IfaceManager#removeExistingIface: newIfaceId=" + newIfaceId
+ + ", pre-map=" + mIfaces);
+ }
Iface removedIface = null;
// The number of ifaces in the database could be 1 existing & 1 new at the max.
if (mIfaces.size() > 2) {
@@ -402,6 +418,11 @@
}
return removedIface;
}
+
+ @Override
+ public String toString() {
+ return mIfaces.toString();
+ }
}
private class NormalScanEventCallback implements WifiNl80211Manager.ScanEventCallback {
@@ -829,6 +850,11 @@
// This is invoked from the main system_server thread. Post to our handler.
mHandler.post(() -> {
synchronized (mLock) {
+ if (mVerboseLoggingEnabled) {
+ Log.d(TAG, "interfaceLinkStateChanged: ifaceName=" + ifaceName
+ + ", mInterfaceId = " + mInterfaceId
+ + ", mIfaceMgr=" + mIfaceMgr.toString());
+ }
final Iface ifaceWithId = mIfaceMgr.getIface(mInterfaceId);
if (ifaceWithId == null) {
if (mVerboseLoggingEnabled) {
@@ -1073,7 +1099,9 @@
* @param listener StatusListener listener object.
*/
public void registerStatusListener(@NonNull StatusListener listener) {
- mStatusListeners.add(listener);
+ synchronized (mLock) {
+ mStatusListeners.add(listener);
+ }
}
/**
diff --git a/service/tests/wifitests/src/com/android/server/wifi/HalDeviceManagerTest.java b/service/tests/wifitests/src/com/android/server/wifi/HalDeviceManagerTest.java
index c7127ce..f62fc2b 100644
--- a/service/tests/wifitests/src/com/android/server/wifi/HalDeviceManagerTest.java
+++ b/service/tests/wifitests/src/com/android/server/wifi/HalDeviceManagerTest.java
@@ -985,7 +985,7 @@
// Verify a runnable is posted because current thread is different than the intended thread
// for running "onDestroyed"
- verify(staIfaceOnDestroyedHandler).post(lambdaCaptor.capture());
+ verify(staIfaceOnDestroyedHandler).postAtFrontOfQueue(lambdaCaptor.capture());
// Verify onDestroyed is only run after the posted runnable is dispatched
verify(staIdl, never()).onDestroyed("wlan0");
@@ -1012,7 +1012,7 @@
simulateStartAndStopWifi(staIdl, staIfaceOnDestroyedHandler);
// Verify a runnable is never posted
- verify(staIfaceOnDestroyedHandler, never()).post(any());
+ verify(staIfaceOnDestroyedHandler, never()).postAtFrontOfQueue(any());
// Verify onDestroyed is triggered directly
verify(staIdl).onDestroyed("wlan0");
}
