From bb39991c525f69cc5ed3829ab13abff7c4e721ef Mon Sep 17 00:00:00 2001 From: Sundeep Ghuman Date: Mon, 29 Jan 2018 18:31:15 -0800 Subject: [PATCH] Remove the double lists in WifiTracker. This CL completes the final removal of the double handler, double list, pending notification complexity that was introduced ag/1396615 as a 'fix' to improper API implementation of the AccessPointListener callback. The implementation erroneously refetched the entire WifiTracker list and then performed its own sorting everytime an individual AccessPoint was updated, instead of waiting for WifiTracker's WifiListener.onAccessPointsChanged (plural) method instead. Those changes have now been reverted, and the underlying SetupWizard code has changed since then such that it does not need to be modified to prevent regressions. Bug: 37674366 Test: 1. runtest --path frameworks/base/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/WifiTrackerTest.java 2. runtest --path frameworks/base/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/AccessPointTest.java 3. Manual testing of WifiSettings for visual jank 4. Startup and manual inspection of SetupWizard Wifi Picker. Change-Id: Ia4079859a7a892983ecf55ba8eab13d20120ff99 --- .../android/settingslib/wifi/AccessPoint.java | 91 +++++---- .../android/settingslib/wifi/WifiTracker.java | 186 +++++------------- .../settingslib/wifi/AccessPointTest.java | 32 --- .../settingslib/wifi/WifiTrackerTest.java | 32 --- 4 files changed, 95 insertions(+), 246 deletions(-) diff --git a/packages/SettingsLib/src/com/android/settingslib/wifi/AccessPoint.java b/packages/SettingsLib/src/com/android/settingslib/wifi/AccessPoint.java index e4c55e10d21e5..b380ac53ddad2 100644 --- a/packages/SettingsLib/src/com/android/settingslib/wifi/AccessPoint.java +++ b/packages/SettingsLib/src/com/android/settingslib/wifi/AccessPoint.java @@ -17,6 +17,7 @@ package com.android.settingslib.wifi; import android.annotation.IntDef; +import android.annotation.MainThread; import android.annotation.Nullable; import android.app.AppGlobals; import android.content.Context; @@ -42,6 +43,8 @@ import android.net.wifi.WifiManager; import android.net.wifi.WifiNetworkScoreCache; import android.net.wifi.hotspot2.PasspointConfiguration; import android.os.Bundle; +import android.os.Handler; +import android.os.Looper; import android.os.Parcelable; import android.os.RemoteException; import android.os.ServiceManager; @@ -57,6 +60,7 @@ import android.util.Log; import com.android.internal.annotations.VisibleForTesting; import com.android.settingslib.R; +import com.android.settingslib.utils.ThreadUtils; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; @@ -68,7 +72,14 @@ import java.util.Map; import java.util.Set; import java.util.concurrent.atomic.AtomicInteger; - +/** + * Represents a selectable Wifi Network for use in various wifi selection menus backed by + * {@link WifiTracker}. + * + *

An AccessPoint, which would be more fittingly named "WifiNetwork", is an aggregation of + * {@link ScanResult ScanResults} along with pertinent metadata (e.g. current connection info, + * network scores) required to successfully render the network to the user. + */ public class AccessPoint implements Comparable { static final String TAG = "SettingsLib.AccessPoint"; @@ -288,11 +299,6 @@ public class AccessPoint implements Comparable { mId = sLastId.incrementAndGet(); } - AccessPoint(Context context, AccessPoint other) { - mContext = context; - copyFrom(other); - } - AccessPoint(Context context, Collection results) { mContext = context; @@ -345,33 +351,6 @@ public class AccessPoint implements Comparable { mKey = builder.toString(); } - /** - * Copy accesspoint information. NOTE: We do not copy tag information because that is never - * set on the internal copy. - */ - void copyFrom(AccessPoint that) { - this.ssid = that.ssid; - this.bssid = that.bssid; - this.security = that.security; - this.mKey = that.mKey; - this.networkId = that.networkId; - this.pskType = that.pskType; - this.mConfig = that.mConfig; //TODO: Watch out, this object is mutated. - this.mRssi = that.mRssi; - this.mInfo = that.mInfo; - this.mNetworkInfo = that.mNetworkInfo; - this.mScanResults.clear(); - this.mScanResults.addAll(that.mScanResults); - this.mScoredNetworkCache.clear(); - this.mScoredNetworkCache.putAll(that.mScoredNetworkCache); - this.mId = that.mId; - this.mSpeed = that.mSpeed; - this.mIsScoredNetworkMetered = that.mIsScoredNetworkMetered; - this.mIsCarrierAp = that.mIsCarrierAp; - this.mCarrierApEapType = that.mCarrierApEapType; - this.mCarrierName = that.mCarrierName; - } - /** * Returns a negative integer, zero, or a positive integer if this AccessPoint is less than, * equal to, or greater than the other AccessPoint. @@ -1070,12 +1049,12 @@ public class AccessPoint implements Comparable { // Only update labels on visible rssi changes updateSpeed(); if (mAccessPointListener != null) { - mAccessPointListener.onLevelChanged(this); + ThreadUtils.postOnMainThread(() -> mAccessPointListener.onLevelChanged(this)); } } if (mAccessPointListener != null) { - mAccessPointListener.onAccessPointChanged(this); + ThreadUtils.postOnMainThread(() -> mAccessPointListener.onAccessPointChanged(this)); } if (!scanResults.isEmpty()) { @@ -1123,10 +1102,10 @@ public class AccessPoint implements Comparable { mNetworkInfo = null; } if (updated && mAccessPointListener != null) { - mAccessPointListener.onAccessPointChanged(this); + ThreadUtils.postOnMainThread(() -> mAccessPointListener.onAccessPointChanged(this)); if (oldLevel != getLevel() /* current level */) { - mAccessPointListener.onLevelChanged(this); + ThreadUtils.postOnMainThread(() -> mAccessPointListener.onLevelChanged(this)); } } @@ -1137,7 +1116,7 @@ public class AccessPoint implements Comparable { mConfig = config; networkId = config != null ? config.networkId : WifiConfiguration.INVALID_NETWORK_ID; if (mAccessPointListener != null) { - mAccessPointListener.onAccessPointChanged(this); + ThreadUtils.postOnMainThread(() -> mAccessPointListener.onAccessPointChanged(this)); } } @@ -1333,9 +1312,41 @@ public class AccessPoint implements Comparable { return string; } + /** + * Callbacks relaying changes to the AccessPoint representation. + * + *

All methods are invoked on the Main Thread. + */ public interface AccessPointListener { - void onAccessPointChanged(AccessPoint accessPoint); - void onLevelChanged(AccessPoint accessPoint); + /** + * Indicates a change to the externally visible state of the AccessPoint trigger by an + * update of ScanResults, saved configuration state, connection state, or score + * (labels/metered) state. + * + *

Clients should refresh their view of the AccessPoint to match the updated state when + * this is invoked. Overall this method is extraneous if clients are listening to + * {@link WifiTracker.WifiListener#onAccessPointsChanged()} callbacks. + * + *

Examples of changes include signal strength, connection state, speed label, and + * generally anything that would impact the summary string. + * + * @param accessPoint The accessPoint object the listener was registered on which has + * changed + */ + @MainThread void onAccessPointChanged(AccessPoint accessPoint); + + /** + * Indicates the "wifi pie signal level" has changed, retrieved via calls to + * {@link AccessPoint#getLevel()}. + * + *

This call is a subset of {@link #onAccessPointChanged(AccessPoint)} , hence is also + * extraneous if the client is already reacting to that or the + * {@link WifiTracker.WifiListener#onAccessPointsChanged()} callbacks. + * + * @param accessPoint The accessPoint object the listener was registered on whose level has + * changed + */ + @MainThread void onLevelChanged(AccessPoint accessPoint); } private static boolean isVerboseLoggingEnabled() { diff --git a/packages/SettingsLib/src/com/android/settingslib/wifi/WifiTracker.java b/packages/SettingsLib/src/com/android/settingslib/wifi/WifiTracker.java index cdbe594c2278d..587eeb18bb73f 100644 --- a/packages/SettingsLib/src/com/android/settingslib/wifi/WifiTracker.java +++ b/packages/SettingsLib/src/com/android/settingslib/wifi/WifiTracker.java @@ -15,6 +15,7 @@ */ package com.android.settingslib.wifi; +import android.annotation.AnyThread; import android.annotation.MainThread; import android.content.BroadcastReceiver; import android.content.Context; @@ -48,7 +49,6 @@ import android.text.format.DateUtils; import android.util.ArrayMap; import android.util.ArraySet; import android.util.Log; -import android.util.SparseArray; import android.util.SparseIntArray; import android.widget.Toast; @@ -122,35 +122,17 @@ public class WifiTracker implements LifecycleObserver, OnStart, OnStop, OnDestro @GuardedBy("mLock") private boolean mRegistered; - /** - * The externally visible access point list. - * - * Updated using main handler. Clone of this collection is returned from - * {@link #getAccessPoints()} - */ - private final List mAccessPoints = new ArrayList<>(); - - /** - * The internal list of access points, synchronized on itself. - * - * Never exposed outside this class. - */ + /** The list of AccessPoints, aggregated visible ScanResults with metadata. */ @GuardedBy("mLock") private final List mInternalAccessPoints = new ArrayList<>(); /** * Synchronization lock for managing concurrency between main and worker threads. * - *

This lock should be held for all background work. - * TODO(b/37674366): Remove the worker thread so synchronization is no longer necessary. + *

This lock should be held for all modifications to {@link #mInternalAccessPoints}. */ private final Object mLock = new Object(); - //visible to both worker and main thread. - @GuardedBy("mLock") - private final AccessPointListenerAdapter mAccessPointListenerAdapter - = new AccessPointListenerAdapter(); - private final HashMap mSeenBssids = new HashMap<>(); // TODO(sghuman): Change this to be keyed on AccessPoint.getKey @@ -170,6 +152,12 @@ public class WifiTracker implements LifecycleObserver, OnStart, OnStop, OnDestro @VisibleForTesting Scanner mScanner; + /** + * Tracks whether fresh scan results have been received since scanning start. + * + *

If this variable is false, we will not evict the scan result cache or invoke callbacks + * so that we do not update the UI with stale data / clear out existing UI elements prematurely. + */ @GuardedBy("mLock") private boolean mStaleScanResults = true; @@ -285,12 +273,6 @@ public class WifiTracker implements LifecycleObserver, OnStart, OnStop, OnDestro List configs = mWifiManager.getConfiguredNetworks(); mInternalAccessPoints.clear(); updateAccessPointsLocked(newScanResults, configs); - - // Synchronously copy access points - copyAndNotifyListeners(); - if (isVerboseLoggingEnabled()) { - Log.i(TAG, "force update - external access point list:\n" + mAccessPoints); - } } } @@ -418,12 +400,19 @@ public class WifiTracker implements LifecycleObserver, OnStart, OnStop, OnDestro } /** - * Gets the current list of access points. Should be called from main thread, otherwise - * expect inconsistencies + * Gets the current list of access points. + * + *

This method is can be called on an abitrary thread by clients, but is normally called on + * the UI Thread by the rendering App. */ - @MainThread + @AnyThread public List getAccessPoints() { - return new ArrayList<>(mAccessPoints); + // TODO(sghuman): Investigate how to eliminate or reduce the need for locking now that we + // have transitioned to a single worker thread model. + + synchronized (mLock) { + return new ArrayList<>(mInternalAccessPoints); + } } public WifiManager getManager() { @@ -535,11 +524,15 @@ public class WifiTracker implements LifecycleObserver, OnStart, OnStop, OnDestro * Update the internal list of access points. * *

Do not call directly (except for forceUpdate), use {@link #updateAccessPoints()} which - * respects {@link #mStaleScanResults}. + * acquires the lock first. */ @GuardedBy("mLock") private void updateAccessPointsLocked(final List newScanResults, List configs) { + // TODO(sghuman): Reduce the synchronization time by only holding the lock when + // modifying lists exposed to operations on the MainThread (getAccessPoints, stopTracking, + // startTracking, etc). + WifiConfiguration connectionConfig = null; if (mLastInfo != null) { connectionConfig = getWifiConfigurationForNetworkId( @@ -645,7 +638,7 @@ public class WifiTracker implements LifecycleObserver, OnStart, OnStop, OnDestro mInternalAccessPoints.clear(); mInternalAccessPoints.addAll(accessPoints); - copyAndNotifyListeners(); + conditionallyNotifyListeners(); } @VisibleForTesting @@ -661,7 +654,6 @@ public class WifiTracker implements LifecycleObserver, OnStart, OnStop, OnDestro } } final AccessPoint accessPoint = new AccessPoint(mContext, scanResults); - accessPoint.setListener(mAccessPointListenerAdapter); return accessPoint; } @@ -672,11 +664,11 @@ public class WifiTracker implements LifecycleObserver, OnStart, OnStop, OnDestro if (cache.get(i).matches(config)) { AccessPoint ret = cache.remove(i); ret.loadConfig(config); + return ret; } } final AccessPoint accessPoint = new AccessPoint(mContext, config); - accessPoint.setListener(mAccessPointListenerAdapter); return accessPoint; } @@ -726,16 +718,21 @@ public class WifiTracker implements LifecycleObserver, OnStart, OnStop, OnDestro Collections.sort(mInternalAccessPoints); } if (updated) { - copyAndNotifyListeners(); + conditionallyNotifyListeners(); } } } + /** + * Clears the access point list and conditionally invokes + * {@link WifiListener#onAccessPointsChanged()} if required (i.e. the list was not already + * empty). + */ private void clearAccessPointsAndConditionallyUpdate() { synchronized (mLock) { if (!mInternalAccessPoints.isEmpty()) { mInternalAccessPoints.clear(); - copyAndNotifyListeners(); + mListener.onAccessPointsChanged(); } } } @@ -758,7 +755,7 @@ public class WifiTracker implements LifecycleObserver, OnStart, OnStop, OnDestro } if (updated) { Collections.sort(mInternalAccessPoints); - copyAndNotifyListeners(); + conditionallyNotifyListeners(); } } } @@ -1003,122 +1000,27 @@ public class WifiTracker implements LifecycleObserver, OnStart, OnStop, OnDestro void onWifiStateChanged(int state); /** - * Called when the connection state of wifi has changed and isConnected - * should be called to get the updated state. + * Called when the connection state of wifi has changed and + * {@link WifiTracker#isConnected()} should be called to get the updated state. */ void onConnectedChanged(); /** * Called to indicate the list of AccessPoints has been updated and - * getAccessPoints should be called to get the latest information. + * {@link WifiTracker#getAccessPoints()} should be called to get the updated list. */ void onAccessPointsChanged(); } /** - * Helps capture notifications that were generated during AccessPoint modification. Used later - * on by {@link #copyAndNotifyListeners()} to send notifications. + * Invokes {@link WifiListenerWrapper#onAccessPointsChanged()} if {@link #mStaleScanResults} + * is false. */ - 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); + private void conditionallyNotifyListeners() { + if (mStaleScanResults) { + return; } - @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 and wifi listeners. - * - *

If {@link #mStaleScanResults} is false, listeners are notified, otherwise notifications - * are dropped. - */ - private void copyAndNotifyListeners() { - // Need to watch out for memory allocations on main thread. - SparseArray oldAccessPoints = new SparseArray<>(); - SparseIntArray notificationMap = null; - List updatedAccessPoints = new ArrayList<>(); - - for (AccessPoint accessPoint : mAccessPoints) { - oldAccessPoints.put(accessPoint.mId, accessPoint); - } - - synchronized (mLock) { - if (DBG()) { - Log.d(TAG, "Starting to copy AP items. Internal APs: " - + mInternalAccessPoints); - } - - if (!mStaleScanResults) { - 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); - } - } - - mAccessPoints.clear(); - mAccessPoints.addAll(updatedAccessPoints); - - if (DBG()) { - Log.d(TAG, "External APs after copying: " + mAccessPoints); - } - - 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) { - if (isVerboseLoggingEnabled()) { - Log.i(TAG, String.format( - "Invoking AccessPointListenerAdapter AP_CHANGED for AP key %s and " - + "listener %s on the MainThread", - accessPoint.getKey(), - listener)); - } - ThreadUtils.postOnMainThread(() -> listener.onAccessPointChanged(accessPoint)); - } - - if ((notificationType & AccessPointListenerAdapter.LEVEL_CHANGED) != 0) { - if (isVerboseLoggingEnabled()) { - Log.i(TAG, String.format( - "Invoking AccessPointListenerAdapter LEVEL_CHANGED for AP key %s " - + "and listener %s on the MainThread", - accessPoint.getKey(), - listener)); - } - ThreadUtils.postOnMainThread(() -> listener.onLevelChanged(accessPoint)); - } - } - } - - if (!mStaleScanResults) { - mListener.onAccessPointsChanged(); - } + ThreadUtils.postOnMainThread(() -> mListener.onAccessPointsChanged()); } } diff --git a/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/AccessPointTest.java b/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/AccessPointTest.java index 144031108662e..54c02a22e79fb 100644 --- a/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/AccessPointTest.java +++ b/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/AccessPointTest.java @@ -112,38 +112,6 @@ public class AccessPointTest { assertThat(spans[0].getType()).isEqualTo(TtsSpan.TYPE_TELEPHONE); } - @Test - public void testCopyAccessPoint_dataShouldMatch() { - WifiConfiguration configuration = createWifiConfiguration(); - configuration.meteredHint = true; - - 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); - - assertThat(originalAccessPoint.getSsid().toString()).isEqualTo(copy.getSsid().toString()); - assertThat(originalAccessPoint.getBssid()).isEqualTo(copy.getBssid()); - assertThat(originalAccessPoint.getConfig()).isEqualTo(copy.getConfig()); - assertThat(originalAccessPoint.getSecurity()).isEqualTo(copy.getSecurity()); - assertThat(originalAccessPoint.isMetered()).isEqualTo(copy.isMetered()); - assertThat(originalAccessPoint.compareTo(copy) == 0).isTrue(); - } - - @Test - public void testThatCopyAccessPoint_scanCacheShouldMatch() { - AccessPoint original = createAccessPointWithScanResultCache(); - assertThat(original.getRssi()).isEqualTo(4); - AccessPoint copy = new AccessPoint(mContext, createWifiConfiguration()); - assertThat(copy.getRssi()).isEqualTo(AccessPoint.UNREACHABLE_RSSI); - copy.copyFrom(original); - assertThat(original.getRssi()).isEqualTo(copy.getRssi()); - } - @Test public void testCompareTo_GivesActiveBeforeInactive() { AccessPoint activeAp = new TestAccessPointBuilder(mContext).setActive(true).build(); diff --git a/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/WifiTrackerTest.java b/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/WifiTrackerTest.java index 3322c6b805f14..1ece95b68bc68 100644 --- a/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/WifiTrackerTest.java +++ b/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/WifiTrackerTest.java @@ -382,38 +382,6 @@ public class WifiTrackerTest { waitForHandlersToProcessCurrentlyEnqueuedMessages(tracker); } - @Test - public void testAccessPointListenerSetWhenLookingUpUsingScanResults() { - ScanResult scanResult = new ScanResult(); - scanResult.level = 123; - scanResult.BSSID = "bssid-" + 111; - scanResult.timestamp = SystemClock.elapsedRealtime() * 1000; - scanResult.capabilities = ""; - - WifiTracker tracker = new WifiTracker( - InstrumentationRegistry.getTargetContext(), null, true, true); - - AccessPoint result = tracker.getCachedOrCreate( - Collections.singletonList(scanResult), new ArrayList()); - assertTrue(result.mAccessPointListener != null); - } - - @Test - public void testAccessPointListenerSetWhenLookingUpUsingWifiConfiguration() { - WifiConfiguration configuration = new WifiConfiguration(); - configuration.SSID = "test123"; - configuration.BSSID="bssid"; - configuration.networkId = 123; - configuration.allowedKeyManagement = new BitSet(); - configuration.allowedKeyManagement.set(WifiConfiguration.KeyMgmt.WPA_PSK); - - WifiTracker tracker = new WifiTracker( - InstrumentationRegistry.getTargetContext(), null, true, true); - - AccessPoint result = tracker.getCachedOrCreate(configuration, new ArrayList()); - assertTrue(result.mAccessPointListener != null); - } - @Test public void startAndStopTrackingShouldRegisterAndUnregisterScoreCache() throws InterruptedException {