From 89b15785c792f386e043224bd0b7cad5889d6726 Mon Sep 17 00:00:00 2001 From: Jason Chiu Date: Tue, 23 Oct 2018 15:31:54 +0800 Subject: [PATCH] Fix crash on Wi-Fi Slice when it's continuously pinned and unpinned It's because WifiTracker's initialization and onStop is on different thread. Fine tune the thread logic in SliceBackgroundWorker. Fixes: 118165942 Test: manual Change-Id: Icc86b5df7ec3c6fd0e4a79a62ea0c84465e9528d --- .../slices/SettingsSliceProvider.java | 29 ++++++++++++++++-- .../slices/SliceBackgroundWorker.java | 6 +++- src/com/android/settings/wifi/WifiSlice.java | 30 +++++++++---------- .../slices/SettingsSliceProviderTest.java | 20 +++++++++++-- 4 files changed, 64 insertions(+), 21 deletions(-) diff --git a/src/com/android/settings/slices/SettingsSliceProvider.java b/src/com/android/settings/slices/SettingsSliceProvider.java index 08ed0d8bf15..5e3e92fc88d 100644 --- a/src/com/android/settings/slices/SettingsSliceProvider.java +++ b/src/com/android/settings/slices/SettingsSliceProvider.java @@ -52,6 +52,7 @@ import com.android.settings.wifi.calling.WifiCallingSliceHelper; import com.android.settingslib.SliceBroadcastRelay; import com.android.settingslib.utils.ThreadUtils; +import java.io.IOException; import java.net.URISyntaxException; import java.util.ArrayList; import java.util.Arrays; @@ -134,6 +135,7 @@ public class SettingsSliceProvider extends SliceProvider { final Set mRegisteredUris = new ArraySet<>(); final Map mWorkerMap = new ArrayMap<>(); + final Set mLiveWorkers = new ArraySet<>(); public SettingsSliceProvider() { super(READ_SEARCH_INDEXABLES); @@ -169,7 +171,9 @@ public class SettingsSliceProvider extends SliceProvider { if (filter != null) { registerIntentToUri(filter, sliceUri); } - startBackgroundWorker(sliceable); + ThreadUtils.postOnMainThread(() -> { + startBackgroundWorker(sliceable); + }); return; } @@ -198,7 +202,9 @@ public class SettingsSliceProvider extends SliceProvider { SliceBroadcastRelay.unregisterReceivers(getContext(), sliceUri); mRegisteredUris.remove(sliceUri); } - stopBackgroundWorker(sliceUri); + ThreadUtils.postOnMainThread(() -> { + stopBackgroundWorker(sliceUri); + }); mSliceDataCache.remove(sliceUri); } @@ -365,12 +371,15 @@ public class SettingsSliceProvider extends SliceProvider { } final Uri uri = sliceable.getUri(); - Log.d(TAG, "Starting background worker for: " + uri); if (mWorkerMap.containsKey(uri)) { return; } + Log.d(TAG, "Starting background worker for: " + uri); mWorkerMap.put(uri, worker); + if (!mLiveWorkers.contains(worker)) { + mLiveWorkers.add(worker); + } worker.onSlicePinned(); } @@ -383,6 +392,20 @@ public class SettingsSliceProvider extends SliceProvider { } } + @Override + public void shutdown() { + for (SliceBackgroundWorker worker : mLiveWorkers) { + ThreadUtils.postOnMainThread(() -> { + try { + worker.close(); + } catch (IOException e) { + Log.w(TAG, "Exception when shutting down worker", e); + } + }); + } + mLiveWorkers.clear(); + } + private List buildUrisFromKeys(List keys, String authority) { final List descendants = new ArrayList<>(); diff --git a/src/com/android/settings/slices/SliceBackgroundWorker.java b/src/com/android/settings/slices/SliceBackgroundWorker.java index 7178eadbdc5..422fcc7b921 100644 --- a/src/com/android/settings/slices/SliceBackgroundWorker.java +++ b/src/com/android/settings/slices/SliceBackgroundWorker.java @@ -16,9 +16,11 @@ package com.android.settings.slices; +import android.annotation.MainThread; import android.content.ContentResolver; import android.net.Uri; +import java.io.Closeable; import java.util.ArrayList; import java.util.List; @@ -32,7 +34,7 @@ import java.util.List; * {@link SliceBackgroundWorker} caches the results, uses the cache to compare if there is any data * changed, and then notifies the Slice {@link Uri} to update. */ -public abstract class SliceBackgroundWorker { +public abstract class SliceBackgroundWorker implements Closeable { private final ContentResolver mContentResolver; private final Uri mUri; @@ -48,12 +50,14 @@ public abstract class SliceBackgroundWorker { * Called when the Slice is pinned. This is the place to register callbacks or initialize scan * tasks. */ + @MainThread protected abstract void onSlicePinned(); /** * Called when the Slice is unpinned. This is the place to unregister callbacks or perform any * final cleanup. */ + @MainThread protected abstract void onSliceUnpinned(); /** diff --git a/src/com/android/settings/wifi/WifiSlice.java b/src/com/android/settings/wifi/WifiSlice.java index 4a78ded2a44..e483d16c4b0 100644 --- a/src/com/android/settings/wifi/WifiSlice.java +++ b/src/com/android/settings/wifi/WifiSlice.java @@ -30,8 +30,6 @@ import android.net.wifi.WifiInfo; import android.net.wifi.WifiManager; import android.net.wifi.WifiSsid; import android.os.Bundle; -import android.os.Handler; -import android.os.Looper; import android.provider.SettingsSlicesContract; import android.text.TextUtils; @@ -62,7 +60,6 @@ import java.util.List; */ public class WifiSlice implements CustomSliceable { - /** * Backing Uri for the Wifi Slice. */ @@ -124,16 +121,16 @@ public class WifiSlice implements CustomSliceable { return listBuilder.build(); } - List result = getBackgroundWorker().getResults(); - if (result == null) { - result = new ArrayList<>(); + List results = getBackgroundWorker().getResults(); + if (results == null) { + results = new ArrayList<>(); } - final int apCount = result.size(); + final int apCount = results.size(); // Add AP rows final CharSequence placeholder = mContext.getText(R.string.summary_placeholder); for (int i = 0; i < DEFAULT_EXPANDED_ROW_COUNT; i++) { if (i < apCount) { - listBuilder.addRow(getAccessPointRow(result.get(i))); + listBuilder.addRow(getAccessPointRow(results.get(i))); } else { listBuilder.addRow(new RowBuilder() .setTitle(placeholder) @@ -277,12 +274,12 @@ public class WifiSlice implements CustomSliceable { private static class WifiScanWorker extends SliceBackgroundWorker implements WifiTracker.WifiListener { + // TODO: enforce all the SliceBackgroundWorkers being singletons at syntax level private static WifiScanWorker mWifiScanWorker; private final Context mContext; private WifiTracker mWifiTracker; - private WifiManager mWifiManager; private WifiScanWorker(Context context, Uri uri) { super(context.getContentResolver(), uri); @@ -298,17 +295,20 @@ public class WifiSlice implements CustomSliceable { @Override protected void onSlicePinned() { - new Handler(Looper.getMainLooper()).post(() -> { + if (mWifiTracker == null) { mWifiTracker = new WifiTracker(mContext, this, true, true); - mWifiManager = mWifiTracker.getManager(); - mWifiTracker.onStart(); - onAccessPointsChanged(); - }); + } + mWifiTracker.onStart(); + onAccessPointsChanged(); } @Override protected void onSliceUnpinned() { mWifiTracker.onStop(); + } + + @Override + public void close() { mWifiTracker.onDestroy(); mWifiScanWorker = null; } @@ -324,7 +324,7 @@ public class WifiSlice implements CustomSliceable { @Override public void onAccessPointsChanged() { // in case state has changed - if (!mWifiManager.isWifiEnabled()) { + if (!mWifiTracker.getManager().isWifiEnabled()) { updateResults(null); return; } diff --git a/tests/robotests/src/com/android/settings/slices/SettingsSliceProviderTest.java b/tests/robotests/src/com/android/settings/slices/SettingsSliceProviderTest.java index 19f3d329b1d..26683db80c6 100644 --- a/tests/robotests/src/com/android/settings/slices/SettingsSliceProviderTest.java +++ b/tests/robotests/src/com/android/settings/slices/SettingsSliceProviderTest.java @@ -67,6 +67,7 @@ import org.robolectric.annotation.Implementation; import org.robolectric.annotation.Implements; import org.robolectric.annotation.Resetter; +import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -485,11 +486,15 @@ public class SettingsSliceProviderTest { final SliceBackgroundWorker worker = spy(new SliceBackgroundWorker( mContext.getContentResolver(), uri) { @Override - public void onSlicePinned() { + protected void onSlicePinned() { } @Override - public void onSliceUnpinned() { + protected void onSliceUnpinned() { + } + + @Override + public void close() { } }); final WifiSlice wifiSlice = spy(new WifiSlice(mContext)); @@ -519,6 +524,17 @@ public class SettingsSliceProviderTest { verify(worker).onSliceUnpinned(); } + @Test + public void shutdown_backgroundWorker_closed() throws IOException { + final Uri uri = WifiSlice.WIFI_URI; + final SliceBackgroundWorker worker = initBackgroundWorker(uri); + + mProvider.onSlicePinned(uri); + mProvider.shutdown(); + + verify(worker).close(); + } + @Test public void grantWhitelistedPackagePermissions_noWhitelist_shouldNotGrant() { final List uris = new ArrayList<>();