Merge "[SetupWizard] Fix wifi multithreading issues" (1ff5badb) · Commits · e / os / android_frameworks_base

packages/SettingsLib/src/com/android/settingslib/wifi/AccessPoint.java

+36 −2

Original line number	Diff line number	Diff line
		@@ -49,8 +49,8 @@ import com.android.settingslib.R;

		import java.util.ArrayList;
		import java.util.Iterator;
		import java.util.Map;
		import java.util.concurrent.ConcurrentHashMap;
		import java.util.concurrent.atomic.AtomicInteger;


		public class AccessPoint implements Comparable<AccessPoint> {
		@@ -95,6 +95,7 @@ public class AccessPoint implements Comparable<AccessPoint> {
		private static final String KEY_PSKTYPE = "key_psktype";
		private static final String KEY_SCANRESULTCACHE = "key_scanresultcache";
		private static final String KEY_CONFIG = "key_config";
		private static final AtomicInteger sLastId = new AtomicInteger(0);

		/**
		* These values are matched in string arrays -- changes must be kept in sync
		@@ -127,10 +128,13 @@ public class AccessPoint implements Comparable<AccessPoint> {

		private WifiInfo mInfo;
		private NetworkInfo mNetworkInfo;
		private AccessPointListener mAccessPointListener;
		AccessPointListener mAccessPointListener;

		private Object mTag;

		// used to co-relate internal vs returned accesspoint.
		int mId;

		public AccessPoint(Context context, Bundle savedState) {
		mContext = context;
		mConfig = savedState.getParcelable(KEY_CONFIG);
		@@ -161,16 +165,46 @@ public class AccessPoint implements Comparable<AccessPoint> {
		update(mConfig, mInfo, mNetworkInfo);
		mRssi = getRssi();
		mSeen = getSeen();
		mId = sLastId.incrementAndGet();
		}

		AccessPoint(Context context, ScanResult result) {
		mContext = context;
		initWithScanResult(result);
		mId = sLastId.incrementAndGet();
		}

		AccessPoint(Context context, WifiConfiguration config) {
		mContext = context;
		loadConfig(config);
		mId = sLastId.incrementAndGet();
		}

		AccessPoint(Context context, AccessPoint other) {
		mContext = context;
		copyFrom(other);
		}

		/**
		* Copy accesspoint information. NOTE: We do not copy tag information because that is never
		* set on the internal copy.
		* @param that
		*/
		void copyFrom(AccessPoint that) {
		that.evictOldScanResults();
		this.ssid = that.ssid;
		this.bssid = that.bssid;
		this.security = that.security;
		this.networkId = that.networkId;
		this.pskType = that.pskType;
		this.mConfig = that.mConfig; //TODO: Watch out, this object is mutated.
		this.mRssi = that.mRssi;
		this.mSeen = that.mSeen;
		this.mInfo = that.mInfo;
		this.mNetworkInfo = that.mNetworkInfo;
		this.mScanResultCache.clear();
		this.mScanResultCache.putAll(that.mScanResultCache);
		this.mId = that.mId;
		}

		@Override

packages/SettingsLib/src/com/android/settingslib/wifi/WifiTracker.java

+178 −28

Original line number	Diff line number	Diff line
		@@ -29,10 +29,13 @@ import android.net.wifi.ScanResult;
		import android.net.wifi.WifiConfiguration;
		import android.net.wifi.WifiInfo;
		import android.net.wifi.WifiManager;
		import android.os.ConditionVariable;
		import android.os.Handler;
		import android.os.Looper;
		import android.os.Message;
		import android.util.Log;
		import android.util.SparseArray;
		import android.util.SparseIntArray;
		import android.widget.Toast;

		import com.android.internal.annotations.VisibleForTesting;
		@@ -62,30 +65,59 @@ public class WifiTracker {
		// TODO: Allow control of this?
		// Combo scans can take 5-6s to complete - set to 10s.
		private static final int WIFI_RESCAN_INTERVAL_MS = 10 * 1000;
		private static final int NUM_SCANS_TO_CONFIRM_AP_LOSS = 3;

		private final Context mContext;
		private final WifiManager mWifiManager;
		private final IntentFilter mFilter;
		private final ConnectivityManager mConnectivityManager;
		private final NetworkRequest mNetworkRequest;
		private WifiTrackerNetworkCallback mNetworkCallback;

		private final AtomicBoolean mConnected = new AtomicBoolean(false);
		private final WifiListener mListener;
		private final boolean mIncludeSaved;
		private final boolean mIncludeScans;
		private final boolean mIncludePasspoints;

		private final MainHandler mMainHandler;
		private final WorkHandler mWorkHandler;

		private WifiTrackerNetworkCallback mNetworkCallback;

		private boolean mSavedNetworksExist;
		private boolean mRegistered;
		private ArrayList<AccessPoint> mAccessPoints = new ArrayList<>();
		private HashMap<String, Integer> mSeenBssids = new HashMap<>();
		private HashMap<String, ScanResult> mScanResultCache = new HashMap<>();

		/** Updated using main handler. Clone of this collection is returned
		* from {@link #getAccessPoints()}
		*/
		private final List<AccessPoint> mAccessPoints = new ArrayList<>();

		/**
		* Protects APs contained in {@link #mInternalAccessPoints} from being modified concurrently
		* while its being read. Usage contract:
		*
		* 1. MainHandler opens the condition after copying the states thereby
		* allowing WorkerHandler to mutate the contents.
		* 2. WorkerHandler after mutating the contents, sends a message to MainHandler to copy the
		* states and closes the condition.
		*
		* This is better than synchronizing on a variable because it prevents MainHandler from
		* unnecessarily blocking/waiting to acquire lock for copying states. When MainHandler is about
		* to access {@link #mInternalAccessPoints}, it is assumed that it has exclusively lock on the
		* contents.
		*/
		private final ConditionVariable mInternalAccessPointsWriteLock = new ConditionVariable(true);

		/** Guarded by mInternalAccessPointsWriteLock, updated using worker handler.
		* Never exposed outside this class.
		*/
		private final List<AccessPoint> mInternalAccessPoints = new ArrayList<>();

		//visible to both worker and main thread. Guarded by #mInternalAccessPoints
		private final AccessPointListenerAdapter mAccessPointListenerAdapter
		= new AccessPointListenerAdapter();

		private final HashMap<String, Integer> mSeenBssids = new HashMap<>();
		private final HashMap<String, ScanResult> mScanResultCache = new HashMap<>();
		private Integer mScanId = 0;
		private static final int NUM_SCANS_TO_CONFIRM_AP_LOSS = 3;

		private NetworkInfo mLastNetworkInfo;
		private WifiInfo mLastInfo;
		@@ -231,13 +263,12 @@ public class WifiTracker {
		}

		/**
		* Gets the current list of access points.
		* Gets the current list of access points. Should be called from main thread, otherwise
		* expect inconsistencies
		*/
		public List<AccessPoint> getAccessPoints() {
		synchronized (mAccessPoints) {
		return new ArrayList<>(mAccessPoints);
		}
		}

		public WifiManager getManager() {
		return mWifiManager;
		@@ -316,8 +347,17 @@ public class WifiTracker {
		}

		private void updateAccessPoints() {
		// Wait until main worker is done copying the states. This is done to prevent propagation
		// of accesspoint states while the update is in progress.
		long before = System.currentTimeMillis();
		mInternalAccessPointsWriteLock.block();
		if (DBG) {
		Log.d(TAG, "Acquired AP lock on WorkerHandler. Time to wait = "
		+ (System.currentTimeMillis() - before) + " ms.");
		}

		// Swap the current access points into a cached list.
		List<AccessPoint> cachedAccessPoints = getAccessPoints();
		List<AccessPoint> cachedAccessPoints = new ArrayList<>(mInternalAccessPoints);
		ArrayList<AccessPoint> accessPoints = new ArrayList<>();

		// Clear out the configs so we don't think something is saved when it isn't.
		@@ -325,7 +365,7 @@ public class WifiTracker {
		accessPoint.clearConfig();
		}

		/** Lookup table to more quickly update AccessPoints by only considering objects with the
		/* Lookup table to more quickly update AccessPoints by only considering objects with the
		* correct SSID. Maps SSID -> List of AccessPoints with the given SSID. */
		Multimap<String, AccessPoint> apMap = new Multimap<String, AccessPoint>();
		WifiConfiguration connectionConfig = null;
		@@ -422,12 +462,14 @@ public class WifiTracker {

		// Log accesspoints that were deleted
		if (DBG) Log.d(TAG, "------ Dumping SSIDs that were not seen on this scan ------");
		for (AccessPoint prevAccessPoint : mAccessPoints) {
		if (prevAccessPoint.getSsid() == null) continue;
		for (AccessPoint prevAccessPoint : mInternalAccessPoints) {
		if (prevAccessPoint.getSsid() == null)
		continue;
		String prevSsid = prevAccessPoint.getSsidStr();
		boolean found = false;
		for (AccessPoint newAccessPoint : accessPoints) {
		if (newAccessPoint.getSsid() != null && newAccessPoint.getSsid().equals(prevSsid)) {
		if (newAccessPoint.getSsid() != null && newAccessPoint.getSsid()
		.equals(prevSsid)) {
		found = true;
		break;
		}
		@@ -437,8 +479,9 @@ public class WifiTracker {
		}
		if (DBG) Log.d(TAG, "---- Done dumping SSIDs that were not seen on this scan ----");

		mAccessPoints = accessPoints;
		mMainHandler.sendEmptyMessage(MainHandler.MSG_ACCESS_POINT_CHANGED);
		mInternalAccessPoints.clear();
		mInternalAccessPoints.addAll(accessPoints);
		mMainHandler.scheduleAPCopyingAndCloseWriteLock();
		}

		private AccessPoint getCachedOrCreate(ScanResult result, List<AccessPoint> cache) {
		@@ -462,7 +505,9 @@ public class WifiTracker {
		return ret;
		}
		}
		return new AccessPoint(mContext, config);
		AccessPoint accessPoint = new AccessPoint(mContext, config);
		accessPoint.setListener(mAccessPointListenerAdapter);
		return accessPoint;
		}

		private void updateNetworkInfo(NetworkInfo networkInfo) {
		@@ -489,17 +534,24 @@ public class WifiTracker {
		connectionConfig = getWifiConfigurationForNetworkId(mLastInfo.getNetworkId());
		}

		// Lock required to prevent accidental copying of AccessPoint states while the modification
		// is in progress. see #copyAndNotifyListeners
		long before = System.currentTimeMillis();
		mInternalAccessPointsWriteLock.block();
		if (DBG) {
		Log.d(TAG, "Acquired AP lock on WorkerHandler for updating NetworkInfo. Wait time = " +
		(System.currentTimeMillis() - before) + "ms.");
		}

		boolean reorder = false;
		for (int i = mAccessPoints.size() - 1; i >= 0; --i) {
		if (mAccessPoints.get(i).update(connectionConfig, mLastInfo, mLastNetworkInfo)) {
		for (int i = mInternalAccessPoints.size() - 1; i >= 0; --i) {
		if (mInternalAccessPoints.get(i).update(connectionConfig, mLastInfo, mLastNetworkInfo)) {
		reorder = true;
		}
		}
		if (reorder) {
		synchronized (mAccessPoints) {
		Collections.sort(mAccessPoints);
		}
		mMainHandler.sendEmptyMessage(MainHandler.MSG_ACCESS_POINT_CHANGED);
		Collections.sort(mInternalAccessPoints);
		mMainHandler.scheduleAPCopyingAndCloseWriteLock();
		}
		}

		@@ -512,6 +564,7 @@ public class WifiTracker {
		WifiTracker tracker = new WifiTracker(context,
		null, null, includeSaved, includeScans, includePasspoints);
		tracker.forceUpdate();
		tracker.copyAndNotifyListeners(false /notifyListeners/);
		return tracker.getAccessPoints();
		}

		@@ -576,6 +629,7 @@ public class WifiTracker {
		mListener.onWifiStateChanged(msg.arg1);
		break;
		case MSG_ACCESS_POINT_CHANGED:
		copyAndNotifyListeners(true /notifyListeners/);
		mListener.onAccessPointsChanged();
		break;
		case MSG_RESUME_SCANNING:
		@@ -590,6 +644,12 @@ public class WifiTracker {
		break;
		}
		}

		void scheduleAPCopyingAndCloseWriteLock() {
		//prevent worker thread from modifying mInternalAccessPoints
		mInternalAccessPointsWriteLock.close();
		sendEmptyMessage(MSG_ACCESS_POINT_CHANGED);
		}
		}

		private final class WorkHandler extends Handler {
		@@ -725,4 +785,94 @@ public class WifiTracker {
		*/
		void onAccessPointsChanged();
		}

		/**
		* Helps capture notifications that were generated during AccessPoint modification. Used later
		* on by {@link #copyAndNotifyListeners(boolean)} to send notifications.
		*/
		private static class AccessPointListenerAdapter implements AccessPoint.AccessPointListener {
		static final int AP_CHANGED = 1;
		static final int LEVEL_CHANGED = 2;

		final SparseIntArray mPendingNotifications = new SparseIntArray();

		@Override
		public void onAccessPointChanged(AccessPoint accessPoint) {
		int type = mPendingNotifications.get(accessPoint.mId);
		mPendingNotifications.put(accessPoint.mId, type \| AP_CHANGED);
		}

		@Override
		public void onLevelChanged(AccessPoint accessPoint) {
		int type = mPendingNotifications.get(accessPoint.mId);
		mPendingNotifications.put(accessPoint.mId, type \| LEVEL_CHANGED);
		}
		}

		/**
		* Responsible for copying access points from {@link #mInternalAccessPoints} and notifying
		* accesspoint listeners. Mutation of the accesspoints returned is done on the main thread.
		*
		* @param notifyListeners if true, accesspoint listeners are notified, otherwise notifications
		* dropped.
		*/
		private void copyAndNotifyListeners(boolean notifyListeners) {
		// Need to watch out for memory allocations on main thread.
		SparseArray<AccessPoint> oldAccessPoints = new SparseArray<>();
		SparseIntArray notificationMap = null;
		List<AccessPoint> updatedAccessPoints = new ArrayList<>();

		for (AccessPoint accessPoint : mAccessPoints) {
		oldAccessPoints.put(accessPoint.mId, accessPoint);
		}

		//synchronize to prevent modification of "mInternalAccessPoints" by worker thread.
		long before = System.currentTimeMillis();
		try {
		if (DBG) {
		Log.d(TAG, "Starting to copy AP items on the MainHandler");
		}
		if (notifyListeners) {
		notificationMap = mAccessPointListenerAdapter.mPendingNotifications.clone();
		}

		mAccessPointListenerAdapter.mPendingNotifications.clear();
		for (AccessPoint internalAccessPoint : mInternalAccessPoints) {
		AccessPoint accessPoint = oldAccessPoints.get(internalAccessPoint.mId);
		if (accessPoint == null) {
		accessPoint = new AccessPoint(mContext, internalAccessPoint);
		} else {
		accessPoint.copyFrom(internalAccessPoint);
		}
		updatedAccessPoints.add(accessPoint);
		}
		} finally {
		mInternalAccessPointsWriteLock.open();
		if (DBG) {
		Log.d(TAG, "Opened AP Write lock on the MainHandler. Time to copy = " +
		(System.currentTimeMillis() - before) + " ms.");
		}
		}

		mAccessPoints.clear();
		mAccessPoints.addAll(updatedAccessPoints);

		if (notificationMap != null && notificationMap.size() > 0) {
		for (AccessPoint accessPoint : updatedAccessPoints) {
		int notificationType = notificationMap.get(accessPoint.mId);
		AccessPoint.AccessPointListener listener = accessPoint.mAccessPointListener;
		if (notificationType == 0 \|\| listener == null) {
		continue;
		}

		if ((notificationType & AccessPointListenerAdapter.AP_CHANGED) != 0) {
		listener.onAccessPointChanged(accessPoint);
		}

		if ((notificationType & AccessPointListenerAdapter.LEVEL_CHANGED) != 0) {
		listener.onLevelChanged(accessPoint);
		}
		}
		}
		}
		}

packages/SettingsLib/tests/src/com/android/settingslib/wifi/AccessPointTest.java

+69 −2

Original line number	Diff line number	Diff line
		@@ -15,24 +15,42 @@
		*/
		package com.android.settingslib.wifi;

		import static org.junit.Assert.assertEquals;
		import static org.junit.Assert.assertSame;
		import static org.junit.Assert.assertTrue;

		import android.content.Context;
		import android.net.ConnectivityManager;
		import android.net.NetworkInfo;
		import android.net.wifi.ScanResult;
		import android.net.wifi.WifiConfiguration;
		import android.net.wifi.WifiInfo;
		import android.net.wifi.WifiSsid;
		import android.os.Bundle;
		import android.os.SystemClock;
		import android.support.test.InstrumentationRegistry;
		import android.support.test.filters.SmallTest;
		import android.support.test.runner.AndroidJUnit4;
		import android.text.SpannableString;
		import android.text.style.TtsSpan;

		import org.junit.Before;
		import org.junit.Test;
		import org.junit.runner.RunWith;

		import static org.junit.Assert.assertEquals;
		import static org.junit.Assert.assertTrue;
		import java.util.ArrayList;

		@SmallTest
		@RunWith(AndroidJUnit4.class)
		public class AccessPointTest {

		private static final String TEST_SSID = "test_ssid";
		private Context mContext;

		@Before
		public void setUp() {
		mContext = InstrumentationRegistry.getTargetContext();
		}

		@Test
		public void testSsidIsTelephoneSpan() {
		@@ -48,4 +66,53 @@ public class AccessPointTest {
		assertEquals(1, spans.length);
		assertEquals(TtsSpan.TYPE_TELEPHONE, spans[0].getType());
		}

		@Test
		public void testCopyAccessPoint_dataShouldMatch() {
		WifiConfiguration configuration = createWifiConfiguration();

		NetworkInfo networkInfo =
		new NetworkInfo(ConnectivityManager.TYPE_WIFI, 2, "WIFI", "WIFI_SUBTYPE");
		AccessPoint originalAccessPoint = new AccessPoint(mContext, configuration);
		WifiInfo wifiInfo = new WifiInfo();
		wifiInfo.setSSID(WifiSsid.createFromAsciiEncoded(configuration.SSID));
		wifiInfo.setBSSID(configuration.BSSID);
		originalAccessPoint.update(configuration, wifiInfo, networkInfo);
		AccessPoint copy = new AccessPoint(mContext, originalAccessPoint);

		assertEquals(originalAccessPoint.getSsid().toString(), copy.getSsid().toString());
		assertEquals(originalAccessPoint.getBssid(), copy.getBssid());
		assertSame(originalAccessPoint.getConfig(), copy.getConfig());
		assertEquals(originalAccessPoint.getSecurity(), copy.getSecurity());
		assertTrue(originalAccessPoint.compareTo(copy) == 0);
		}

		@Test
		public void testThatCopyAccessPoint_scanCacheShouldMatch() {
		Bundle bundle = new Bundle();
		ArrayList<ScanResult> scanResults = new ArrayList<>();
		for (int i = 0; i < 5; i++) {
		ScanResult scanResult = new ScanResult();
		scanResult.level = i;
		scanResult.BSSID = "bssid-" + i;
		scanResult.timestamp = SystemClock.elapsedRealtime() * 1000;
		scanResults.add(scanResult);
		}

		bundle.putParcelableArrayList("key_scanresultcache", scanResults);
		AccessPoint original = new AccessPoint(mContext, bundle);
		assertEquals(4, original.getRssi());
		AccessPoint copy = new AccessPoint(mContext, createWifiConfiguration());
		assertEquals(Integer.MIN_VALUE, copy.getRssi());
		copy.copyFrom(original);
		assertEquals(original.getRssi(), copy.getRssi());
		}

		private WifiConfiguration createWifiConfiguration() {
		WifiConfiguration configuration = new WifiConfiguration();
		configuration.BSSID = "bssid";
		configuration.SSID = "ssid";
		configuration.networkId = 123;
		return configuration;
		}
		}