From a68f81540e842e432df7586c41ec3893c80e802f Mon Sep 17 00:00:00 2001 From: Jason Chiu Date: Mon, 29 Oct 2018 17:58:55 +0800 Subject: [PATCH] Enforce all the SliceBackgroundWorkers being singletons at syntax level - Create workers via reflection in SliceBackgroundWorker - Store the workers in a static container and release then at shutdown() Fixes: 118228009 Test: robolectric Change-Id: I564277d3a12b2d7d3b50cef091bdfedb3397c145 --- proguard.flags | 7 +- .../settings/slices/CustomSliceable.java | 7 +- .../slices/SettingsSliceProvider.java | 36 ++++----- .../slices/SliceBackgroundWorker.java | 65 +++++++++++++++-- src/com/android/settings/wifi/WifiSlice.java | 26 ++----- .../slices/SettingsSliceProviderTest.java | 73 +++++++++---------- 6 files changed, 124 insertions(+), 90 deletions(-) diff --git a/proguard.flags b/proguard.flags index 82e8e58d92b..b66a7862df2 100644 --- a/proguard.flags +++ b/proguard.flags @@ -60,4 +60,9 @@ # Keep classes that implements CustomSliceable, which are used by reflection. -keepclasseswithmembers class * implements com.android.settings.slices.CustomSliceable { public (android.content.Context); -} \ No newline at end of file +} + +# Keep classes that extends SliceBackgroundWorker, which are used by reflection. +-keepclasseswithmembers class * extends com.android.settings.slices.SliceBackgroundWorker { + public (android.content.Context, android.net.Uri); +} diff --git a/src/com/android/settings/slices/CustomSliceable.java b/src/com/android/settings/slices/CustomSliceable.java index e09cc7386f7..b538b898252 100644 --- a/src/com/android/settings/slices/CustomSliceable.java +++ b/src/com/android/settings/slices/CustomSliceable.java @@ -91,11 +91,12 @@ public interface CustomSliceable { /** * Settings Slices which can represent component lists that are updatable by the - * {@link SliceBackgroundWorker} returned here. + * {@link SliceBackgroundWorker} class returned here. * - * @return a {@link SliceBackgroundWorker} for fetching the list of results in the background. + * @return a {@link SliceBackgroundWorker} class for fetching the list of results in the + * background. */ - default SliceBackgroundWorker getBackgroundWorker() { + default Class getBackgroundWorkerClass() { return null; } diff --git a/src/com/android/settings/slices/SettingsSliceProvider.java b/src/com/android/settings/slices/SettingsSliceProvider.java index 39116037949..33576f5ddcd 100644 --- a/src/com/android/settings/slices/SettingsSliceProvider.java +++ b/src/com/android/settings/slices/SettingsSliceProvider.java @@ -52,7 +52,6 @@ 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,8 +133,7 @@ public class SettingsSliceProvider extends SliceProvider { final Set mRegisteredUris = new ArraySet<>(); - final Map mWorkerMap = new ArrayMap<>(); - final Set mLiveWorkers = new ArraySet<>(); + final Map mPinnedWorkers = new ArrayMap<>(); public SettingsSliceProvider() { super(READ_SEARCH_INDEXABLES); @@ -365,45 +363,37 @@ public class SettingsSliceProvider extends SliceProvider { } private void startBackgroundWorker(CustomSliceable sliceable) { - final SliceBackgroundWorker worker = sliceable.getBackgroundWorker(); - if (worker == null) { + final Class workerClass = sliceable.getBackgroundWorkerClass(); + if (workerClass == null) { return; } final Uri uri = sliceable.getUri(); - if (mWorkerMap.containsKey(uri)) { + if (mPinnedWorkers.containsKey(uri)) { return; } Log.d(TAG, "Starting background worker for: " + uri); - mWorkerMap.put(uri, worker); - if (!mLiveWorkers.contains(worker)) { - mLiveWorkers.add(worker); - } + final SliceBackgroundWorker worker = SliceBackgroundWorker.getInstance( + getContext(), sliceable); + mPinnedWorkers.put(uri, worker); worker.onSlicePinned(); } private void stopBackgroundWorker(Uri uri) { - final SliceBackgroundWorker worker = mWorkerMap.get(uri); + final SliceBackgroundWorker worker = mPinnedWorkers.get(uri); if (worker != null) { Log.d(TAG, "Stopping background worker for: " + uri); worker.onSliceUnpinned(); - mWorkerMap.remove(uri); + mPinnedWorkers.remove(uri); } } @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(); + ThreadUtils.postOnMainThread(() -> { + SliceBackgroundWorker.shutdown(); + }); } private List buildUrisFromKeys(List keys, String authority) { @@ -532,4 +522,4 @@ public class SettingsSliceProvider extends SliceProvider { } return new String[0]; } -} \ No newline at end of file +} diff --git a/src/com/android/settings/slices/SliceBackgroundWorker.java b/src/com/android/settings/slices/SliceBackgroundWorker.java index 422fcc7b921..a663eceb1df 100644 --- a/src/com/android/settings/slices/SliceBackgroundWorker.java +++ b/src/com/android/settings/slices/SliceBackgroundWorker.java @@ -17,35 +17,86 @@ package com.android.settings.slices; import android.annotation.MainThread; -import android.content.ContentResolver; +import android.content.Context; import android.net.Uri; +import android.util.ArrayMap; +import android.util.Log; import java.io.Closeable; +import java.io.IOException; +import java.lang.reflect.InvocationTargetException; import java.util.ArrayList; import java.util.List; +import java.util.Map; /** * The Slice background worker is used to make Settings Slices be able to work with data that is * changing continuously, e.g. available Wi-Fi networks. * - * The background worker will be started at {@link SettingsSliceProvider#onSlicePinned(Uri)}, and be - * stopped at {@link SettingsSliceProvider#onSliceUnpinned(Uri)}. + * The background worker will be started at {@link SettingsSliceProvider#onSlicePinned(Uri)}, be + * stopped at {@link SettingsSliceProvider#onSliceUnpinned(Uri)}, and be closed at {@link + * SettingsSliceProvider#shutdown()}. * * {@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. + * + * It also stores all instances of all workers to ensure each worker is a Singleton. */ public abstract class SliceBackgroundWorker implements Closeable { - private final ContentResolver mContentResolver; + private static final String TAG = "SliceBackgroundWorker"; + + private static final Map LIVE_WORKERS = new ArrayMap<>(); + + private final Context mContext; private final Uri mUri; private List mCachedResults; - protected SliceBackgroundWorker(ContentResolver cr, Uri uri) { - mContentResolver = cr; + protected SliceBackgroundWorker(Context context, Uri uri) { + mContext = context; mUri = uri; } + /** + * Returns the singleton instance of the {@link SliceBackgroundWorker} for specified {@link + * CustomSliceable} + */ + public static SliceBackgroundWorker getInstance(Context context, CustomSliceable sliceable) { + final Uri uri = sliceable.getUri(); + final Class workerClass = + sliceable.getBackgroundWorkerClass(); + SliceBackgroundWorker worker = LIVE_WORKERS.get(uri); + if (worker == null) { + worker = createInstance(context, uri, workerClass); + LIVE_WORKERS.put(uri, worker); + } + return worker; + } + + private static SliceBackgroundWorker createInstance(Context context, Uri uri, + Class clazz) { + Log.d(TAG, "create instance: " + clazz); + try { + return clazz.getConstructor(Context.class, Uri.class).newInstance(context, uri); + } catch (NoSuchMethodException | IllegalAccessException | InstantiationException | + InvocationTargetException e) { + throw new IllegalStateException( + "Invalid slice background worker: " + clazz, e); + } + } + + static void shutdown() { + for (SliceBackgroundWorker worker : LIVE_WORKERS.values()) { + try { + worker.close(); + } catch (IOException e) { + Log.w(TAG, "Shutting down worker failed", e); + } + } + LIVE_WORKERS.clear(); + } + /** * Called when the Slice is pinned. This is the place to register callbacks or initialize scan * tasks. @@ -83,7 +134,7 @@ public abstract class SliceBackgroundWorker implements Closeable { if (needNotify) { mCachedResults = results; - mContentResolver.notifyChange(mUri, null); + mContext.getContentResolver().notifyChange(mUri, null); } } } diff --git a/src/com/android/settings/wifi/WifiSlice.java b/src/com/android/settings/wifi/WifiSlice.java index 43f5372981e..d06d830af39 100644 --- a/src/com/android/settings/wifi/WifiSlice.java +++ b/src/com/android/settings/wifi/WifiSlice.java @@ -121,7 +121,7 @@ public class WifiSlice implements CustomSliceable { return listBuilder.build(); } - List results = getBackgroundWorker().getResults(); + List results = SliceBackgroundWorker.getInstance(mContext, this).getResults(); if (results == null) { results = new ArrayList<>(); } @@ -264,36 +264,27 @@ public class WifiSlice implements CustomSliceable { } @Override - public SliceBackgroundWorker getBackgroundWorker() { - return WifiScanWorker.getInstance(mContext, WIFI_URI); + public Class getBackgroundWorkerClass() { + return WifiScanWorker.class; } - private static class WifiScanWorker extends SliceBackgroundWorker + public 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 WifiScanWorker(Context context, Uri uri) { - super(context.getContentResolver(), uri); + public WifiScanWorker(Context context, Uri uri) { + super(context, uri); mContext = context; } - public static WifiScanWorker getInstance(Context context, Uri uri) { - if (mWifiScanWorker == null) { - mWifiScanWorker = new WifiScanWorker(context, uri); - } - return mWifiScanWorker; - } - @Override protected void onSlicePinned() { if (mWifiTracker == null) { - mWifiTracker = new WifiTracker(mContext, this, true, true); + mWifiTracker = new WifiTracker(mContext, this /* wifiListener */, + true /* includeSaved */, true /* includeScans */); } mWifiTracker.onStart(); onAccessPointsChanged(); @@ -307,7 +298,6 @@ public class WifiSlice implements CustomSliceable { @Override public void close() { mWifiTracker.onDestroy(); - mWifiScanWorker = null; } @Override diff --git a/tests/robotests/src/com/android/settings/slices/SettingsSliceProviderTest.java b/tests/robotests/src/com/android/settings/slices/SettingsSliceProviderTest.java index a0fd21be3d0..3c2cbdbb036 100644 --- a/tests/robotests/src/com/android/settings/slices/SettingsSliceProviderTest.java +++ b/tests/robotests/src/com/android/settings/slices/SettingsSliceProviderTest.java @@ -25,6 +25,7 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.verify; @@ -60,6 +61,7 @@ import com.android.settings.testutils.shadow.ShadowThreadUtils; import com.android.settings.testutils.shadow.ShadowUserManager; import com.android.settings.testutils.shadow.ShadowUtils; import com.android.settings.wifi.WifiSlice; +import com.android.settingslib.wifi.WifiTracker; import org.junit.After; import org.junit.Before; @@ -75,7 +77,6 @@ import org.robolectric.annotation.Resetter; import org.robolectric.shadow.api.Shadow; import org.robolectric.shadows.ShadowAccessibilityManager; -import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -91,7 +92,8 @@ import java.util.Set; @RunWith(SettingsRobolectricTestRunner.class) @Config(shadows = {ShadowUserManager.class, ShadowThreadUtils.class, ShadowUtils.class, SlicesDatabaseAccessorTest.ShadowApplicationPackageManager.class, - ShadowBluetoothAdapter.class, ShadowLockPatternUtils.class}) + ShadowBluetoothAdapter.class, ShadowLockPatternUtils.class, + SettingsSliceProviderTest.ShadowWifiScanWorker.class}) public class SettingsSliceProviderTest { private static final String KEY = "KEY"; @@ -135,7 +137,7 @@ public class SettingsSliceProviderTest { mProvider.mSliceWeakDataCache = new HashMap<>(); mProvider.mSliceDataCache = new HashMap<>(); mProvider.mSlicesDatabaseAccessor = new SlicesDatabaseAccessor(mContext); - mProvider.mCustomSliceManager = spy(new CustomSliceManager(mContext)); + mProvider.mCustomSliceManager = new CustomSliceManager(mContext); when(mProvider.getContext()).thenReturn(mContext); SlicesDatabaseHelper.getInstance(mContext).setIndexedState(); @@ -497,57 +499,52 @@ public class SettingsSliceProviderTest { mProvider.onSlicePinned(uri); } - private SliceBackgroundWorker initBackgroundWorker(Uri uri) { - final SliceBackgroundWorker worker = spy(new SliceBackgroundWorker( - mContext.getContentResolver(), uri) { - @Override - protected void onSlicePinned() { - } + @Implements(WifiSlice.WifiScanWorker.class) + public static class ShadowWifiScanWorker { + private static WifiTracker mWifiTracker; - @Override - protected void onSliceUnpinned() { - } + @Implementation + protected void onSlicePinned() { + mWifiTracker = mock(WifiTracker.class); + mWifiTracker.onStart(); + } - @Override - public void close() { - } - }); - final WifiSlice wifiSlice = spy(new WifiSlice(mContext)); - when(wifiSlice.getBackgroundWorker()).thenReturn(worker); - when(mProvider.mCustomSliceManager.getSliceableFromUri(uri)).thenReturn(wifiSlice); - return worker; + @Implementation + protected void onSliceUnpinned() { + mWifiTracker.onStop(); + } + + @Implementation + public void close() { + mWifiTracker.onDestroy(); + } + + static WifiTracker getWifiTracker() { + return mWifiTracker; + } } @Test public void onSlicePinned_backgroundWorker_started() { - final Uri uri = WifiSlice.WIFI_URI; - final SliceBackgroundWorker worker = initBackgroundWorker(uri); + mProvider.onSlicePinned(WifiSlice.WIFI_URI); - mProvider.onSlicePinned(uri); - - verify(worker).onSlicePinned(); + verify(ShadowWifiScanWorker.getWifiTracker()).onStart(); } @Test public void onSlicePinned_backgroundWorker_stopped() { - final Uri uri = WifiSlice.WIFI_URI; - final SliceBackgroundWorker worker = initBackgroundWorker(uri); + mProvider.onSlicePinned(WifiSlice.WIFI_URI); + mProvider.onSliceUnpinned(WifiSlice.WIFI_URI); - mProvider.onSlicePinned(uri); - mProvider.onSliceUnpinned(uri); - - verify(worker).onSliceUnpinned(); + verify(ShadowWifiScanWorker.getWifiTracker()).onStop(); } @Test - public void shutdown_backgroundWorker_closed() throws IOException { - final Uri uri = WifiSlice.WIFI_URI; - final SliceBackgroundWorker worker = initBackgroundWorker(uri); - - mProvider.onSlicePinned(uri); + public void shutdown_backgroundWorker_closed() { + mProvider.onSlicePinned(WifiSlice.WIFI_URI); mProvider.shutdown(); - verify(worker).close(); + verify(ShadowWifiScanWorker.getWifiTracker()).onDestroy(); } @Test @@ -630,4 +627,4 @@ public class SettingsSliceProviderTest { return sSetThreadPolicyCount != 0; } } -} \ No newline at end of file +}