From d8465f805a935660d6c8e8fe2d89a4b8b7097ce8 Mon Sep 17 00:00:00 2001 From: Fabian Kozynski Date: Thu, 21 Jul 2022 11:50:22 -0400 Subject: Fix race condition in QSTileHost QSTileHost had three problems that caused a race condition: * Calls to add/remove tiles could come and be executed in any thread. * Different paths to add/remove tiles used different ground truths on what tiles were currently active. * The list of tiles contained in mTileSpecs was not necessarily the same as the visible tiles. This caused the following problem: * If two tiles (usually CustomTile) were removed roughly at the same time, only one of them would actually be removed from the Setting, as both of them would race to remove from the same list. This caused... * There would be leftover tiles that cannot connect to a TileService and therefore show empty UI. The solution is as follows: * Make sure that all operations that modify the tiles are performed in the main thread. * Make sure that all operations that modify the tiles use the same truth for the current state. * Make sure that mTileSpecs always reflects the setting at the end of onTuningChanged. This means that tiles that are destroyed or removed will never be in this list. Test: atest com.android.systemui.qs Test: manual, start Guest user and notice that there are no blank tiles. Fixes: 235561921 Change-Id: I6ffbb7510e6854f18bc427d1ae1e782918c799dc Merged-In: I6ffbb7510e6854f18bc427d1ae1e782918c799dc (cherry picked from commit 7f2bb15f4e40c0e1d1d0b2308444f4f9f449bd3f) (cherry picked from commit 6098b8f68bc21d84ddde92f12a1550401666aa12) Merged-In: I6ffbb7510e6854f18bc427d1ae1e782918c799dc --- .../src/com/android/systemui/qs/QSTileHost.java | 134 +++++++++----- .../android/systemui/qs/customize/TileAdapter.java | 4 +- .../systemui/qs/external/TileServiceManager.java | 2 +- .../systemui/statusbar/phone/AutoTileManager.java | 2 +- .../CentralSurfacesCommandQueueCallbacks.java | 2 +- .../com/android/systemui/qs/QSFragmentTest.java | 40 ---- .../com/android/systemui/qs/QSTileHostTest.java | 205 +++++++++++++++------ .../systemui/qs/customize/TileAdapterTest.java | 2 +- .../systemui/qs/external/TileServicesTest.java | 7 +- .../statusbar/phone/AutoTileManagerTest.java | 2 +- 10 files changed, 247 insertions(+), 153 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/qs/QSTileHost.java b/packages/SystemUI/src/com/android/systemui/qs/QSTileHost.java index fbdabc74aba4..fcd9e10089bc 100644 --- a/packages/SystemUI/src/com/android/systemui/qs/QSTileHost.java +++ b/packages/SystemUI/src/com/android/systemui/qs/QSTileHost.java @@ -19,8 +19,6 @@ import android.content.Context; import android.content.Intent; import android.content.res.Resources; import android.os.Build; -import android.os.Handler; -import android.os.Looper; import android.os.UserHandle; import android.os.UserManager; import android.provider.Settings.Secure; @@ -28,6 +26,7 @@ import android.text.TextUtils; import android.util.ArraySet; import android.util.Log; +import androidx.annotation.MainThread; import androidx.annotation.Nullable; import com.android.internal.logging.InstanceId; @@ -35,9 +34,7 @@ import com.android.internal.logging.InstanceIdSequence; import com.android.internal.logging.UiEventLogger; import com.android.systemui.Dumpable; import com.android.systemui.R; -import com.android.systemui.broadcast.BroadcastDispatcher; import com.android.systemui.dagger.SysUISingleton; -import com.android.systemui.dagger.qualifiers.Background; import com.android.systemui.dagger.qualifiers.Main; import com.android.systemui.dump.DumpManager; import com.android.systemui.plugins.PluginListener; @@ -68,12 +65,20 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Optional; import java.util.Set; +import java.util.concurrent.Executor; import java.util.function.Predicate; import javax.inject.Inject; import javax.inject.Provider; -/** Platform implementation of the quick settings tile host **/ +/** Platform implementation of the quick settings tile host + * + * This class keeps track of the set of current tiles and is the in memory source of truth + * (ground truth is kept in {@link Secure#QS_TILES}). When the ground truth changes, + * {@link #onTuningChanged} will be called and the tiles will be re-created as needed. + * + * This class also provides the interface for adding/removing/changing tiles. + */ @SysUISingleton public class QSTileHost implements QSHost, Tunable, PluginListener, Dumpable { private static final String TAG = "QSTileHost"; @@ -89,11 +94,11 @@ public class QSTileHost implements QSHost, Tunable, PluginListener, D private final TunerService mTunerService; private final PluginManager mPluginManager; private final DumpManager mDumpManager; - private final BroadcastDispatcher mBroadcastDispatcher; private final QSLogger mQSLogger; private final UiEventLogger mUiEventLogger; private final InstanceIdSequence mInstanceIdSequence; private final CustomTileStatePersister mCustomTileStatePersister; + private final Executor mMainExecutor; private final List mCallbacks = new ArrayList<>(); @Nullable @@ -113,13 +118,11 @@ public class QSTileHost implements QSHost, Tunable, PluginListener, D public QSTileHost(Context context, StatusBarIconController iconController, QSFactory defaultFactory, - @Main Handler mainHandler, - @Background Looper bgLooper, + @Main Executor mainExecutor, PluginManager pluginManager, TunerService tunerService, Provider autoTiles, DumpManager dumpManager, - BroadcastDispatcher broadcastDispatcher, Optional centralSurfacesOptional, QSLogger qsLogger, UiEventLogger uiEventLogger, @@ -137,7 +140,7 @@ public class QSTileHost implements QSHost, Tunable, PluginListener, D mDumpManager = dumpManager; mQSLogger = qsLogger; mUiEventLogger = uiEventLogger; - mBroadcastDispatcher = broadcastDispatcher; + mMainExecutor = mainExecutor; mTileServiceRequestController = tileServiceRequestControllerBuilder.create(this); mTileLifeCycleManagerFactory = tileLifecycleManagerFactory; @@ -151,7 +154,7 @@ public class QSTileHost implements QSHost, Tunable, PluginListener, D mSecureSettings = secureSettings; mCustomTileStatePersister = customTileStatePersister; - mainHandler.post(() -> { + mainExecutor.execute(() -> { // This is technically a hack to avoid circular dependency of // QSTileHost -> XXXTile -> QSTileHost. Posting ensures creation // finishes before creating any tiles. @@ -258,6 +261,33 @@ public class QSTileHost implements QSHost, Tunable, PluginListener, D return mTileSpecs.indexOf(spec); } + /** + * Whenever the Secure Setting keeping track of the current tiles changes (or upon start) this + * will be called with the new value of the setting. + * + * This method will do the following: + *
    + *
  1. Destroy any existing tile that's not one of the current tiles (in the setting)
    2. + *
  2. Create new tiles for those that don't already exist. If this tiles end up being + * not available, they'll also be destroyed.
    3. + *
  3. Save the resolved list of tiles (current tiles that are available) into the setting. + * This means that after this call ends, the tiles in the Setting, {@link #mTileSpecs}, + * and visible tiles ({@link #mTiles}) must match. + *
    4. + *
+ * + * Additionally, if the user has changed, it'll do the following: + *
    + *
  • Change the user for SystemUI tiles: {@link QSTile#userSwitch}.
    • + *
  • Destroy any {@link CustomTile} and recreate it for the new user.
    • + *
+ * + * This happens in main thread as {@link com.android.systemui.tuner.TunerServiceImpl} dispatches + * in main thread. + * + * @see QSTile#isAvailable + */ + @MainThread @Override public void onTuningChanged(String key, String newValue) { if (!TILES_SETTING.equals(key)) { @@ -330,34 +360,44 @@ public class QSTileHost implements QSHost, Tunable, PluginListener, D mCurrentUser = currentUser; List currentSpecs = new ArrayList<>(mTileSpecs); mTileSpecs.clear(); - mTileSpecs.addAll(tileSpecs); + mTileSpecs.addAll(newTiles.keySet()); // Only add the valid (available) tiles. mTiles.clear(); mTiles.putAll(newTiles); if (newTiles.isEmpty() && !tileSpecs.isEmpty()) { // If we didn't manage to create any tiles, set it to empty (default) Log.d(TAG, "No valid tiles on tuning changed. Setting to default."); - changeTiles(currentSpecs, loadTileSpecs(mContext, "")); + changeTilesByUser(currentSpecs, loadTileSpecs(mContext, "")); } else { + String resolvedTiles = TextUtils.join(",", mTileSpecs); + if (!resolvedTiles.equals(newValue)) { + // If the resolved tiles (those we actually ended up with) are different than + // the ones that are in the setting, update the Setting. + saveTilesToSettings(mTileSpecs); + } for (int i = 0; i < mCallbacks.size(); i++) { mCallbacks.get(i).onTilesChanged(); } } } + /** + * Only use with [CustomTile] if the tile doesn't exist anymore (and therefore doesn't need + * its lifecycle terminated). + */ @Override public void removeTile(String spec) { - changeTileSpecs(tileSpecs-> tileSpecs.remove(spec)); + mMainExecutor.execute(() -> changeTileSpecs(tileSpecs-> tileSpecs.remove(spec))); } /** * Remove many tiles at once. * - * It will only save to settings once (as opposed to {@link QSTileHost#removeTile} called + * It will only save to settings once (as opposed to {@link QSTileHost#removeTileByUser} called * multiple times). */ @Override public void removeTiles(Collection specs) { - changeTileSpecs(tileSpecs -> tileSpecs.removeAll(specs)); + mMainExecutor.execute(() -> changeTileSpecs(tileSpecs -> tileSpecs.removeAll(specs))); } @Override @@ -381,27 +421,30 @@ public class QSTileHost implements QSHost, Tunable, PluginListener, D * @param requestPosition -1 for end, 0 for beginning, or X for insertion at position X */ public void addTile(String spec, int requestPosition) { - if (spec.equals("work")) Log.wtfStack(TAG, "Adding work tile"); - changeTileSpecs(tileSpecs -> { - if (tileSpecs.contains(spec)) return false; - - int size = tileSpecs.size(); - if (requestPosition == POSITION_AT_END || requestPosition >= size) { - tileSpecs.add(spec); - } else { - tileSpecs.add(requestPosition, spec); - } - return true; - }); + mMainExecutor.execute(() -> + changeTileSpecs(tileSpecs -> { + if (tileSpecs.contains(spec)) return false; + + int size = tileSpecs.size(); + if (requestPosition == POSITION_AT_END || requestPosition >= size) { + tileSpecs.add(spec); + } else { + tileSpecs.add(requestPosition, spec); + } + return true; + }) + ); } - void saveTilesToSettings(List tileSpecs) { - if (tileSpecs.contains("work")) Log.wtfStack(TAG, "Saving work tile"); + + @MainThread + private void saveTilesToSettings(List tileSpecs) { mSecureSettings.putStringForUser(TILES_SETTING, TextUtils.join(",", tileSpecs), null /* tag */, false /* default */, mCurrentUser, true /* overrideable by restore */); } + @MainThread private void changeTileSpecs(Predicate> changeFunction) { final String setting = mSecureSettings.getStringForUser(TILES_SETTING, mCurrentUser); final List tileSpecs = loadTileSpecs(mContext, setting); @@ -421,29 +464,32 @@ public class QSTileHost implements QSHost, Tunable, PluginListener, D */ public void addTile(ComponentName tile, boolean end) { String spec = CustomTile.toSpec(tile); - if (!mTileSpecs.contains(spec)) { - List newSpecs = new ArrayList<>(mTileSpecs); - if (end) { - newSpecs.add(spec); - } else { - newSpecs.add(0, spec); - } - changeTiles(mTileSpecs, newSpecs); - } + addTile(spec, end ? POSITION_AT_END : 0); } - public void removeTile(ComponentName tile) { - List newSpecs = new ArrayList<>(mTileSpecs); - newSpecs.remove(CustomTile.toSpec(tile)); - changeTiles(mTileSpecs, newSpecs); + /** + * This will call through {@link #changeTilesByUser}. It should only be used when a tile is + * removed by a user action like {@code adb}. + */ + public void removeTileByUser(ComponentName tile) { + mMainExecutor.execute(() -> { + List newSpecs = new ArrayList<>(mTileSpecs); + if (newSpecs.remove(CustomTile.toSpec(tile))) { + changeTilesByUser(mTileSpecs, newSpecs); + } + }); } /** * Change the tiles triggered by the user editing. *

* This is not called on device start, or on user change. + * + * {@link android.service.quicksettings.TileService#onTileRemoved} will be called for tiles + * that are removed. */ - public void changeTiles(List previousTiles, List newTiles) { + @MainThread + public void changeTilesByUser(List previousTiles, List newTiles) { final List copy = new ArrayList<>(previousTiles); final int NP = copy.size(); for (int i = 0; i < NP; i++) { diff --git a/packages/SystemUI/src/com/android/systemui/qs/customize/TileAdapter.java b/packages/SystemUI/src/com/android/systemui/qs/customize/TileAdapter.java index e52bfbd67275..d84b12c714bd 100644 --- a/packages/SystemUI/src/com/android/systemui/qs/customize/TileAdapter.java +++ b/packages/SystemUI/src/com/android/systemui/qs/customize/TileAdapter.java @@ -182,7 +182,7 @@ public class TileAdapter extends RecyclerView.Adapter implements TileSta for (int i = 1; i < mTiles.size() && mTiles.get(i) != null; i++) { newSpecs.add(mTiles.get(i).spec); } - host.changeTiles(mCurrentSpecs, newSpecs); + host.changeTilesByUser(mCurrentSpecs, newSpecs); mCurrentSpecs = newSpecs; } @@ -200,7 +200,7 @@ public class TileAdapter extends RecyclerView.Adapter implements TileSta /** */ public void resetTileSpecs(List specs) { // Notify the host so the tiles get removed callbacks. - mHost.changeTiles(mCurrentSpecs, specs); + mHost.changeTilesByUser(mCurrentSpecs, specs); setTileSpecs(specs); } diff --git a/packages/SystemUI/src/com/android/systemui/qs/external/TileServiceManager.java b/packages/SystemUI/src/com/android/systemui/qs/external/TileServiceManager.java index bf565a8c52e0..cfc57db2eeb8 100644 --- a/packages/SystemUI/src/com/android/systemui/qs/external/TileServiceManager.java +++ b/packages/SystemUI/src/com/android/systemui/qs/external/TileServiceManager.java @@ -289,7 +289,7 @@ public class TileServiceManager { } } - mServices.getHost().removeTile(component); + mServices.getHost().removeTile(CustomTile.toSpec(component)); } }; } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/phone/AutoTileManager.java b/packages/SystemUI/src/com/android/systemui/statusbar/phone/AutoTileManager.java index 8782be50794f..9070eadd9944 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/phone/AutoTileManager.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/phone/AutoTileManager.java @@ -428,7 +428,7 @@ public class AutoTileManager implements UserAwareController { if (isSafetyCenterEnabled && !mAutoTracker.isAdded(mSafetySpec)) { initSafetyTile(); } else if (!isSafetyCenterEnabled && mAutoTracker.isAdded(mSafetySpec)) { - mHost.removeTile(CustomTile.getComponentFromSpec(mSafetySpec)); + mHost.removeTile(mSafetySpec); mHost.unmarkTileAsAutoAdded(mSafetySpec); } } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/phone/CentralSurfacesCommandQueueCallbacks.java b/packages/SystemUI/src/com/android/systemui/statusbar/phone/CentralSurfacesCommandQueueCallbacks.java index 9060d5f67913..ffd50ab5af37 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/phone/CentralSurfacesCommandQueueCallbacks.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/phone/CentralSurfacesCommandQueueCallbacks.java @@ -187,7 +187,7 @@ public class CentralSurfacesCommandQueueCallbacks implements CommandQueue.Callba public void remQsTile(ComponentName tile) { QSPanelController qsPanelController = mCentralSurfaces.getQSPanelController(); if (qsPanelController != null && qsPanelController.getHost() != null) { - qsPanelController.getHost().removeTile(tile); + qsPanelController.getHost().removeTileByUser(tile); } } diff --git a/packages/SystemUI/tests/src/com/android/systemui/qs/QSFragmentTest.java b/packages/SystemUI/tests/src/com/android/systemui/qs/QSFragmentTest.java index 664af75a6529..32c66d25d4aa 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/qs/QSFragmentTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/qs/QSFragmentTest.java @@ -32,8 +32,6 @@ import android.app.Fragment; import android.content.Context; import android.graphics.Rect; import android.os.Bundle; -import android.os.Handler; -import android.os.Looper; import android.testing.AndroidTestingRunner; import android.testing.TestableLooper.RunWithLooper; import android.view.LayoutInflater; @@ -42,36 +40,23 @@ import android.view.ViewGroup; import androidx.test.filters.SmallTest; -import com.android.internal.logging.UiEventLogger; import com.android.keyguard.BouncerPanelExpansionCalculator; import com.android.systemui.R; import com.android.systemui.SysuiBaseFragmentTest; import com.android.systemui.animation.ShadeInterpolation; -import com.android.systemui.broadcast.BroadcastDispatcher; import com.android.systemui.dump.DumpManager; import com.android.systemui.media.MediaHost; import com.android.systemui.plugins.FalsingManager; import com.android.systemui.plugins.statusbar.StatusBarStateController; import com.android.systemui.qs.customize.QSCustomizerController; import com.android.systemui.qs.dagger.QSFragmentComponent; -import com.android.systemui.qs.external.CustomTileStatePersister; -import com.android.systemui.qs.external.TileLifecycleManager; import com.android.systemui.qs.external.TileServiceRequestController; -import com.android.systemui.qs.logging.QSLogger; -import com.android.systemui.qs.tileimpl.QSFactoryImpl; -import com.android.systemui.settings.UserTracker; -import com.android.systemui.shared.plugins.PluginManager; import com.android.systemui.statusbar.CommandQueue; import com.android.systemui.statusbar.StatusBarState; -import com.android.systemui.statusbar.phone.AutoTileManager; -import com.android.systemui.statusbar.phone.CentralSurfaces; import com.android.systemui.statusbar.phone.KeyguardBypassController; -import com.android.systemui.statusbar.phone.StatusBarIconController; import com.android.systemui.statusbar.policy.ConfigurationController; import com.android.systemui.statusbar.policy.RemoteInputQuickSettingsDisabler; -import com.android.systemui.tuner.TunerService; import com.android.systemui.util.animation.UniqueObjectHostView; -import com.android.systemui.util.settings.SecureSettings; import org.junit.Before; import org.junit.Test; @@ -79,8 +64,6 @@ import org.junit.runner.RunWith; import org.mockito.Mock; import org.mockito.MockitoAnnotations; -import java.util.Optional; - @RunWith(AndroidTestingRunner.class) @RunWithLooper(setAsMainLooper = true) @SmallTest @@ -125,34 +108,11 @@ public class QSFragmentTest extends SysuiBaseFragmentTest { mFragments.dispatchResume(); processAllMessages(); - QSTileHost host = - new QSTileHost( - mContext, - mock(StatusBarIconController.class), - mock(QSFactoryImpl.class), - new Handler(), - Looper.myLooper(), - mock(PluginManager.class), - mock(TunerService.class), - () -> mock(AutoTileManager.class), - mock(DumpManager.class), - mock(BroadcastDispatcher.class), - Optional.of(mock(CentralSurfaces.class)), - mock(QSLogger.class), - mock(UiEventLogger.class), - mock(UserTracker.class), - mock(SecureSettings.class), - mock(CustomTileStatePersister.class), - mTileServiceRequestControllerBuilder, - mock(TileLifecycleManager.Factory.class)); - qs.setListening(true); processAllMessages(); qs.setListening(false); processAllMessages(); - host.destroy(); - processAllMessages(); } @Test diff --git a/packages/SystemUI/tests/src/com/android/systemui/qs/QSTileHostTest.java b/packages/SystemUI/tests/src/com/android/systemui/qs/QSTileHostTest.java index 8cf3fe274848..7dbc561fbfc1 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/qs/QSTileHostTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/qs/QSTileHostTest.java @@ -32,12 +32,11 @@ import static org.mockito.Mockito.when; import android.content.ComponentName; import android.content.Context; import android.content.Intent; +import android.database.ContentObserver; import android.os.Handler; import android.os.Looper; import android.os.UserHandle; import android.testing.AndroidTestingRunner; -import android.testing.TestableLooper; -import android.testing.TestableLooper.RunWithLooper; import android.view.View; import androidx.annotation.Nullable; @@ -48,7 +47,6 @@ import com.android.internal.logging.UiEventLogger; import com.android.internal.util.CollectionUtils; import com.android.systemui.R; import com.android.systemui.SysuiTestCase; -import com.android.systemui.broadcast.BroadcastDispatcher; import com.android.systemui.classifier.FalsingManagerFake; import com.android.systemui.dump.DumpManager; import com.android.systemui.plugins.ActivityStarter; @@ -68,8 +66,10 @@ import com.android.systemui.statusbar.phone.AutoTileManager; import com.android.systemui.statusbar.phone.CentralSurfaces; import com.android.systemui.statusbar.phone.StatusBarIconController; import com.android.systemui.tuner.TunerService; +import com.android.systemui.util.concurrency.FakeExecutor; import com.android.systemui.util.settings.FakeSettings; import com.android.systemui.util.settings.SecureSettings; +import com.android.systemui.util.time.FakeSystemClock; import org.junit.Before; import org.junit.Test; @@ -81,18 +81,19 @@ import java.io.PrintWriter; import java.io.StringWriter; import java.util.List; import java.util.Optional; +import java.util.concurrent.Executor; import javax.inject.Provider; @RunWith(AndroidTestingRunner.class) @SmallTest -@RunWithLooper(setAsMainLooper = true) public class QSTileHostTest extends SysuiTestCase { private static String MOCK_STATE_STRING = "MockState"; private static ComponentName CUSTOM_TILE = ComponentName.unflattenFromString("TEST_PKG/.TEST_CLS"); private static final String CUSTOM_TILE_SPEC = CustomTile.toSpec(CUSTOM_TILE); + private static final String SETTING = QSTileHost.TILES_SETTING; @Mock private StatusBarIconController mIconController; @@ -107,8 +108,6 @@ public class QSTileHostTest extends SysuiTestCase { @Mock private DumpManager mDumpManager; @Mock - private BroadcastDispatcher mBroadcastDispatcher; - @Mock private QSTile.State mMockState; @Mock private CentralSurfaces mCentralSurfaces; @@ -132,31 +131,47 @@ public class QSTileHostTest extends SysuiTestCase { @Mock private TileLifecycleManager mTileLifecycleManager; - private Handler mHandler; - private TestableLooper mLooper; + private FakeExecutor mMainExecutor; + private QSTileHost mQSTileHost; @Before public void setUp() { MockitoAnnotations.initMocks(this); - mLooper = TestableLooper.get(this); - mHandler = new Handler(mLooper.getLooper()); + mMainExecutor = new FakeExecutor(new FakeSystemClock()); + when(mTileServiceRequestControllerBuilder.create(any())) .thenReturn(mTileServiceRequestController); when(mTileLifecycleManagerFactory.create(any(Intent.class), any(UserHandle.class))) .thenReturn(mTileLifecycleManager); mSecureSettings = new FakeSettings(); - mSecureSettings.putStringForUser( - QSTileHost.TILES_SETTING, "", "", false, mUserTracker.getUserId(), false); - mQSTileHost = new TestQSTileHost(mContext, mIconController, mDefaultFactory, mHandler, - mLooper.getLooper(), mPluginManager, mTunerService, mAutoTiles, mDumpManager, - mBroadcastDispatcher, mCentralSurfaces, mQSLogger, mUiEventLogger, mUserTracker, - mSecureSettings, mCustomTileStatePersister, mTileServiceRequestControllerBuilder, - mTileLifecycleManagerFactory); + saveSetting(""); + mQSTileHost = new TestQSTileHost(mContext, mIconController, mDefaultFactory, mMainExecutor, + mPluginManager, mTunerService, mAutoTiles, mDumpManager, mCentralSurfaces, + mQSLogger, mUiEventLogger, mUserTracker, mSecureSettings, mCustomTileStatePersister, + mTileServiceRequestControllerBuilder, mTileLifecycleManagerFactory); + + mSecureSettings.registerContentObserverForUser(SETTING, new ContentObserver(null) { + @Override + public void onChange(boolean selfChange) { + super.onChange(selfChange); + mMainExecutor.execute(() -> mQSTileHost.onTuningChanged(SETTING, getSetting())); + mMainExecutor.runAllReady(); + } + }, mUserTracker.getUserId()); setUpTileFactory(); } + private void saveSetting(String value) { + mSecureSettings.putStringForUser( + SETTING, value, "", false, mUserTracker.getUserId(), false); + } + + private String getSetting() { + return mSecureSettings.getStringForUser(SETTING, mUserTracker.getUserId()); + } + private void setUpTileFactory() { when(mMockState.toString()).thenReturn(MOCK_STATE_STRING); // Only create this kind of tiles @@ -173,6 +188,10 @@ public class QSTileHostTest extends SysuiTestCase { return new NotAvailableTile(mQSTileHost); } else if (CUSTOM_TILE_SPEC.equals(spec)) { return mCustomTile; + } else if ("internet".equals(spec) + || "wifi".equals(spec) + || "cell".equals(spec)) { + return new TestTile1(mQSTileHost); } else { return null; } @@ -196,14 +215,14 @@ public class QSTileHostTest extends SysuiTestCase { public void testInvalidSpecUsesDefault() { mContext.getOrCreateTestableResources() .addOverride(R.string.quick_settings_tiles, "spec1,spec2"); - mQSTileHost.onTuningChanged(QSTileHost.TILES_SETTING, "not-valid"); + saveSetting("not-valid"); assertEquals(2, mQSTileHost.getTiles().size()); } @Test public void testRemoveWifiAndCellularWithoutInternet() { - mQSTileHost.onTuningChanged(QSTileHost.TILES_SETTING, "wifi, spec1, cell, spec2"); + saveSetting("wifi, spec1, cell, spec2"); assertEquals("internet", mQSTileHost.mTileSpecs.get(0)); assertEquals("spec1", mQSTileHost.mTileSpecs.get(1)); @@ -212,7 +231,7 @@ public class QSTileHostTest extends SysuiTestCase { @Test public void testRemoveWifiAndCellularWithInternet() { - mQSTileHost.onTuningChanged(QSTileHost.TILES_SETTING, "wifi, spec1, cell, spec2, internet"); + saveSetting("wifi, spec1, cell, spec2, internet"); assertEquals("spec1", mQSTileHost.mTileSpecs.get(0)); assertEquals("spec2", mQSTileHost.mTileSpecs.get(1)); @@ -221,7 +240,7 @@ public class QSTileHostTest extends SysuiTestCase { @Test public void testRemoveWifiWithoutInternet() { - mQSTileHost.onTuningChanged(QSTileHost.TILES_SETTING, "spec1, wifi, spec2"); + saveSetting("spec1, wifi, spec2"); assertEquals("spec1", mQSTileHost.mTileSpecs.get(0)); assertEquals("internet", mQSTileHost.mTileSpecs.get(1)); @@ -230,7 +249,7 @@ public class QSTileHostTest extends SysuiTestCase { @Test public void testRemoveCellWithInternet() { - mQSTileHost.onTuningChanged(QSTileHost.TILES_SETTING, "spec1, spec2, cell, internet"); + saveSetting("spec1, spec2, cell, internet"); assertEquals("spec1", mQSTileHost.mTileSpecs.get(0)); assertEquals("spec2", mQSTileHost.mTileSpecs.get(1)); @@ -239,7 +258,7 @@ public class QSTileHostTest extends SysuiTestCase { @Test public void testNoWifiNoCellularNoInternet() { - mQSTileHost.onTuningChanged(QSTileHost.TILES_SETTING, "spec1,spec2"); + saveSetting("spec1,spec2"); assertEquals("spec1", mQSTileHost.mTileSpecs.get(0)); assertEquals("spec2", mQSTileHost.mTileSpecs.get(1)); @@ -249,7 +268,7 @@ public class QSTileHostTest extends SysuiTestCase { public void testSpecWithInvalidDoesNotUseDefault() { mContext.getOrCreateTestableResources() .addOverride(R.string.quick_settings_tiles, "spec1,spec2"); - mQSTileHost.onTuningChanged(QSTileHost.TILES_SETTING, "spec2,not-valid"); + saveSetting("spec2,not-valid"); assertEquals(1, mQSTileHost.getTiles().size()); QSTile element = CollectionUtils.firstOrNull(mQSTileHost.getTiles()); @@ -258,7 +277,7 @@ public class QSTileHostTest extends SysuiTestCase { @Test public void testDump() { - mQSTileHost.onTuningChanged(QSTileHost.TILES_SETTING, "spec1,spec2"); + saveSetting("spec1,spec2"); StringWriter w = new StringWriter(); PrintWriter pw = new PrintWriter(w); mQSTileHost.dump(pw, new String[]{}); @@ -274,7 +293,7 @@ public class QSTileHostTest extends SysuiTestCase { public void testDefault() { mContext.getOrCreateTestableResources() .addOverride(R.string.quick_settings_tiles_default, "spec1"); - mQSTileHost.onTuningChanged(QSTileHost.TILES_SETTING, "default"); + saveSetting("default"); assertEquals(1, mQSTileHost.getTiles().size()); QSTile element = CollectionUtils.firstOrNull(mQSTileHost.getTiles()); assertTrue(element instanceof TestTile1); @@ -285,7 +304,7 @@ public class QSTileHostTest extends SysuiTestCase { public void testNoRepeatedSpecs_addTile() { mContext.getOrCreateTestableResources() .addOverride(R.string.quick_settings_tiles, "spec1,spec2"); - mQSTileHost.onTuningChanged(QSTileHost.TILES_SETTING, "spec1,spec2"); + saveSetting("spec1,spec2"); mQSTileHost.addTile("spec1"); @@ -298,9 +317,10 @@ public class QSTileHostTest extends SysuiTestCase { public void testAddTileAtValidPosition() { mContext.getOrCreateTestableResources() .addOverride(R.string.quick_settings_tiles, "spec1,spec3"); - mQSTileHost.onTuningChanged(QSTileHost.TILES_SETTING, "spec1,spec3"); + saveSetting("spec1,spec3"); mQSTileHost.addTile("spec2", 1); + mMainExecutor.runAllReady(); assertEquals(3, mQSTileHost.mTileSpecs.size()); assertEquals("spec1", mQSTileHost.mTileSpecs.get(0)); @@ -312,9 +332,10 @@ public class QSTileHostTest extends SysuiTestCase { public void testAddTileAtInvalidPositionAddsToEnd() { mContext.getOrCreateTestableResources() .addOverride(R.string.quick_settings_tiles, "spec1,spec3"); - mQSTileHost.onTuningChanged(QSTileHost.TILES_SETTING, "spec1,spec3"); + saveSetting("spec1,spec3"); mQSTileHost.addTile("spec2", 100); + mMainExecutor.runAllReady(); assertEquals(3, mQSTileHost.mTileSpecs.size()); assertEquals("spec1", mQSTileHost.mTileSpecs.get(0)); @@ -326,9 +347,10 @@ public class QSTileHostTest extends SysuiTestCase { public void testAddTileAtEnd() { mContext.getOrCreateTestableResources() .addOverride(R.string.quick_settings_tiles, "spec1,spec3"); - mQSTileHost.onTuningChanged(QSTileHost.TILES_SETTING, "spec1,spec3"); + saveSetting("spec1,spec3"); mQSTileHost.addTile("spec2", QSTileHost.POSITION_AT_END); + mMainExecutor.runAllReady(); assertEquals(3, mQSTileHost.mTileSpecs.size()); assertEquals("spec1", mQSTileHost.mTileSpecs.get(0)); @@ -338,9 +360,10 @@ public class QSTileHostTest extends SysuiTestCase { @Test public void testNoRepeatedSpecs_customTile() { - mQSTileHost.onTuningChanged(QSTileHost.TILES_SETTING, CUSTOM_TILE_SPEC); + saveSetting(CUSTOM_TILE_SPEC); mQSTileHost.addTile(CUSTOM_TILE, /* end */ false); + mMainExecutor.runAllReady(); assertEquals(1, mQSTileHost.mTileSpecs.size()); assertEquals(CUSTOM_TILE_SPEC, mQSTileHost.mTileSpecs.get(0)); @@ -348,9 +371,10 @@ public class QSTileHostTest extends SysuiTestCase { @Test public void testAddedAtBeginningOnDefault_customTile() { - mQSTileHost.onTuningChanged(QSTileHost.TILES_SETTING, "spec1"); // seed + saveSetting("spec1"); // seed mQSTileHost.addTile(CUSTOM_TILE); + mMainExecutor.runAllReady(); assertEquals(2, mQSTileHost.mTileSpecs.size()); assertEquals(CUSTOM_TILE_SPEC, mQSTileHost.mTileSpecs.get(0)); @@ -358,9 +382,10 @@ public class QSTileHostTest extends SysuiTestCase { @Test public void testAddedAtBeginning_customTile() { - mQSTileHost.onTuningChanged(QSTileHost.TILES_SETTING, "spec1"); // seed + saveSetting("spec1"); // seed mQSTileHost.addTile(CUSTOM_TILE, /* end */ false); + mMainExecutor.runAllReady(); assertEquals(2, mQSTileHost.mTileSpecs.size()); assertEquals(CUSTOM_TILE_SPEC, mQSTileHost.mTileSpecs.get(0)); @@ -368,9 +393,10 @@ public class QSTileHostTest extends SysuiTestCase { @Test public void testAddedAtEnd_customTile() { - mQSTileHost.onTuningChanged(QSTileHost.TILES_SETTING, "spec1"); // seed + saveSetting("spec1"); // seed mQSTileHost.addTile(CUSTOM_TILE, /* end */ true); + mMainExecutor.runAllReady(); assertEquals(2, mQSTileHost.mTileSpecs.size()); assertEquals(CUSTOM_TILE_SPEC, mQSTileHost.mTileSpecs.get(1)); @@ -409,13 +435,13 @@ public class QSTileHostTest extends SysuiTestCase { @Test public void testNotAvailableTile_specNotNull() { - mQSTileHost.onTuningChanged(QSTileHost.TILES_SETTING, "na"); + saveSetting("na"); verify(mQSLogger, never()).logTileDestroyed(isNull(), anyString()); } @Test public void testCustomTileRemoved_stateDeleted() { - mQSTileHost.changeTiles(List.of(CUSTOM_TILE_SPEC), List.of()); + mQSTileHost.changeTilesByUser(List.of(CUSTOM_TILE_SPEC), List.of()); verify(mCustomTileStatePersister) .removeState(new TileServiceKey(CUSTOM_TILE, mQSTileHost.getUserId())); @@ -423,29 +449,99 @@ public class QSTileHostTest extends SysuiTestCase { @Test public void testRemoveTiles() { - List tiles = List.of("spec1", "spec2", "spec3"); - mQSTileHost.saveTilesToSettings(tiles); + saveSetting("spec1,spec2,spec3"); mQSTileHost.removeTiles(List.of("spec1", "spec2")); + mMainExecutor.runAllReady(); assertEquals(List.of("spec3"), mQSTileHost.mTileSpecs); } + @Test + public void testTilesRemovedInQuickSuccession() { + saveSetting("spec1,spec2,spec3"); + mQSTileHost.removeTile("spec1"); + mQSTileHost.removeTile("spec3"); + + mMainExecutor.runAllReady(); + assertEquals(List.of("spec2"), mQSTileHost.mTileSpecs); + assertEquals("spec2", getSetting()); + } + + @Test + public void testAddTileInMainThread() { + saveSetting("spec1,spec2"); + + mQSTileHost.addTile("spec3"); + assertEquals(List.of("spec1", "spec2"), mQSTileHost.mTileSpecs); + + mMainExecutor.runAllReady(); + assertEquals(List.of("spec1", "spec2", "spec3"), mQSTileHost.mTileSpecs); + } + + @Test + public void testRemoveTileInMainThread() { + saveSetting("spec1,spec2"); + + mQSTileHost.removeTile("spec1"); + assertEquals(List.of("spec1", "spec2"), mQSTileHost.mTileSpecs); + + mMainExecutor.runAllReady(); + assertEquals(List.of("spec2"), mQSTileHost.mTileSpecs); + } + + @Test + public void testRemoveTilesInMainThread() { + saveSetting("spec1,spec2,spec3"); + + mQSTileHost.removeTiles(List.of("spec3", "spec1")); + assertEquals(List.of("spec1", "spec2", "spec3"), mQSTileHost.mTileSpecs); + + mMainExecutor.runAllReady(); + assertEquals(List.of("spec2"), mQSTileHost.mTileSpecs); + } + + @Test + public void testRemoveTileByUserInMainThread() { + saveSetting("spec1," + CUSTOM_TILE_SPEC); + + mQSTileHost.removeTileByUser(CUSTOM_TILE); + assertEquals(List.of("spec1", CUSTOM_TILE_SPEC), mQSTileHost.mTileSpecs); + + mMainExecutor.runAllReady(); + assertEquals(List.of("spec1"), mQSTileHost.mTileSpecs); + } + + @Test + public void testNonValidTileNotStoredInSettings() { + saveSetting("spec1,not-valid"); + + assertEquals(List.of("spec1"), mQSTileHost.mTileSpecs); + assertEquals("spec1", getSetting()); + } + + @Test + public void testNotAvailableTileNotStoredInSettings() { + saveSetting("spec1,na"); + + assertEquals(List.of("spec1"), mQSTileHost.mTileSpecs); + assertEquals("spec1", getSetting()); + } + private class TestQSTileHost extends QSTileHost { TestQSTileHost(Context context, StatusBarIconController iconController, - QSFactory defaultFactory, Handler mainHandler, Looper bgLooper, + QSFactory defaultFactory, Executor mainExecutor, PluginManager pluginManager, TunerService tunerService, Provider autoTiles, DumpManager dumpManager, - BroadcastDispatcher broadcastDispatcher, CentralSurfaces centralSurfaces, - QSLogger qsLogger, UiEventLogger uiEventLogger, UserTracker userTracker, - SecureSettings secureSettings, CustomTileStatePersister customTileStatePersister, + CentralSurfaces centralSurfaces, QSLogger qsLogger, UiEventLogger uiEventLogger, + UserTracker userTracker, SecureSettings secureSettings, + CustomTileStatePersister customTileStatePersister, TileServiceRequestController.Builder tileServiceRequestControllerBuilder, TileLifecycleManager.Factory tileLifecycleManagerFactory) { - super(context, iconController, defaultFactory, mainHandler, bgLooper, pluginManager, - tunerService, autoTiles, dumpManager, broadcastDispatcher, - Optional.of(centralSurfaces), qsLogger, uiEventLogger, userTracker, - secureSettings, customTileStatePersister, tileServiceRequestControllerBuilder, - tileLifecycleManagerFactory); + super(context, iconController, defaultFactory, mainExecutor, pluginManager, + tunerService, autoTiles, dumpManager, Optional.of(centralSurfaces), qsLogger, + uiEventLogger, userTracker, secureSettings, customTileStatePersister, + tileServiceRequestControllerBuilder, tileLifecycleManagerFactory); } @Override @@ -455,25 +551,16 @@ public class QSTileHostTest extends SysuiTestCase { @Override public void onPluginDisconnected(QSFactory plugin) { } - - @Override - void saveTilesToSettings(List tileSpecs) { - super.saveTilesToSettings(tileSpecs); - // After tiles are changed, make sure to call onTuningChanged with the new setting if it - // changed - String specs = mSecureSettings.getStringForUser( - QSTileHost.TILES_SETTING, mUserTracker.getUserId()); - onTuningChanged(TILES_SETTING, specs); - } } + private class TestTile extends QSTileImpl { protected TestTile(QSHost host) { super( host, - mLooper.getLooper(), - new Handler(mLooper.getLooper()), + mock(Looper.class), + mock(Handler.class), new FalsingManagerFake(), mock(MetricsLogger.class), mock(StatusBarStateController.class), diff --git a/packages/SystemUI/tests/src/com/android/systemui/qs/customize/TileAdapterTest.java b/packages/SystemUI/tests/src/com/android/systemui/qs/customize/TileAdapterTest.java index 3d53062d7d02..d42cbe3b698a 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/qs/customize/TileAdapterTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/qs/customize/TileAdapterTest.java @@ -55,6 +55,6 @@ public class TileAdapterTest extends SysuiTestCase { @Test public void testResetNotifiesHost() { mTileAdapter.resetTileSpecs(Collections.emptyList()); - verify(mQSTileHost).changeTiles(any(), any()); + verify(mQSTileHost).changeTilesByUser(any(), any()); } } diff --git a/packages/SystemUI/tests/src/com/android/systemui/qs/external/TileServicesTest.java b/packages/SystemUI/tests/src/com/android/systemui/qs/external/TileServicesTest.java index 6b7e5b9335f2..471ddfd3f224 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/qs/external/TileServicesTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/qs/external/TileServicesTest.java @@ -29,6 +29,7 @@ import static org.mockito.Mockito.when; import android.content.ComponentName; import android.content.Intent; import android.os.Handler; +import android.os.HandlerExecutor; import android.os.RemoteException; import android.os.UserHandle; import android.service.quicksettings.IQSTileService; @@ -65,6 +66,7 @@ import org.mockito.MockitoAnnotations; import java.util.ArrayList; import java.util.Optional; +import java.util.concurrent.Executor; import javax.inject.Provider; @@ -130,17 +132,16 @@ public class TileServicesTest extends SysuiTestCase { .thenReturn(mTileLifecycleManager); Provider provider = () -> new Handler(mTestableLooper.getLooper()); + Executor executor = new HandlerExecutor(provider.get()); QSTileHost host = new QSTileHost(mContext, mStatusBarIconController, mQSFactory, - provider.get(), - mTestableLooper.getLooper(), + executor, mPluginManager, mTunerService, () -> mAutoTileManager, mDumpManager, - mock(BroadcastDispatcher.class), Optional.of(mCentralSurfaces), mQSLogger, mUiEventLogger, diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/AutoTileManagerTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/AutoTileManagerTest.java index 371119cc6d01..4ccbc6d45e63 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/AutoTileManagerTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/AutoTileManagerTest.java @@ -490,7 +490,7 @@ public class AutoTileManagerTest extends SysuiTestCase { mAutoTileManager.init(); when(mAutoAddTracker.isAdded(TEST_CUSTOM_SAFETY_SPEC)).thenReturn(true); mAutoTileManager.mSafetyCallback.onSafetyCenterEnableChanged(false); - verify(mQsTileHost, times(1)).removeTile(safetyComponent); + verify(mQsTileHost, times(1)).removeTile(TEST_CUSTOM_SAFETY_SPEC); } @Test -- cgit v1.2.3 From 6062fccc1247932cd3f126ce6fd19dbdef8745f9 Mon Sep 17 00:00:00 2001 From: Fabian Kozynski Date: Tue, 2 Aug 2022 09:54:41 -0400 Subject: Reduce blocking calls to Settings in main thread In most cases, mTileList contains the same information as the Settings value, so there's no need to retrieve it before modifying it. Keep track of that with a dirty flag. This way, we reduce the number of blocking calls in the main thread (as that's the thread that processes the tiles). Test: manual, factory reset Test: existing QSTileHostTest Test: performance metrics are back to baseline Fixes: 240256263 Change-Id: Idebd37d1458c80330b60802729575219b6a7b49a Merged-In: Idebd37d1458c80330b60802729575219b6a7b49a (cherry picked from commit 4da071e035b647f498936bdb2b557aa08bd9df0f) Merged-In: Idebd37d1458c80330b60802729575219b6a7b49a --- .../src/com/android/systemui/qs/QSTileHost.java | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/qs/QSTileHost.java b/packages/SystemUI/src/com/android/systemui/qs/QSTileHost.java index fcd9e10089bc..bf83ad6bff74 100644 --- a/packages/SystemUI/src/com/android/systemui/qs/QSTileHost.java +++ b/packages/SystemUI/src/com/android/systemui/qs/QSTileHost.java @@ -110,6 +110,11 @@ public class QSTileHost implements QSHost, Tunable, PluginListener, D private Context mUserContext; private UserTracker mUserTracker; private SecureSettings mSecureSettings; + // Keep track of whether mTilesList contains the same information as the Settings value. + // This is a performance optimization to reduce the number of blocking calls to Settings from + // main thread. + // This is enforced by only cleaning the flag at the end of a successful run of #onTuningChanged + private boolean mTilesListDirty = true; private final TileServiceRequestController mTileServiceRequestController; private TileLifecycleManager.Factory mTileLifeCycleManagerFactory; @@ -374,6 +379,7 @@ public class QSTileHost implements QSHost, Tunable, PluginListener, D // the ones that are in the setting, update the Setting. saveTilesToSettings(mTileSpecs); } + mTilesListDirty = false; for (int i = 0; i < mCallbacks.size(); i++) { mCallbacks.get(i).onTilesChanged(); } @@ -437,6 +443,7 @@ public class QSTileHost implements QSHost, Tunable, PluginListener, D } + // When calling this, you may want to modify mTilesListDirty accordingly. @MainThread private void saveTilesToSettings(List tileSpecs) { mSecureSettings.putStringForUser(TILES_SETTING, TextUtils.join(",", tileSpecs), @@ -446,9 +453,15 @@ public class QSTileHost implements QSHost, Tunable, PluginListener, D @MainThread private void changeTileSpecs(Predicate> changeFunction) { - final String setting = mSecureSettings.getStringForUser(TILES_SETTING, mCurrentUser); - final List tileSpecs = loadTileSpecs(mContext, setting); + final List tileSpecs; + if (!mTilesListDirty) { + tileSpecs = new ArrayList<>(mTileSpecs); + } else { + tileSpecs = loadTileSpecs(mContext, + mSecureSettings.getStringForUser(TILES_SETTING, mCurrentUser)); + } if (changeFunction.test(tileSpecs)) { + mTilesListDirty = true; saveTilesToSettings(tileSpecs); } } @@ -508,6 +521,7 @@ public class QSTileHost implements QSHost, Tunable, PluginListener, D } } if (DEBUG) Log.d(TAG, "saveCurrentTiles " + newTiles); + mTilesListDirty = true; saveTilesToSettings(newTiles); } -- cgit v1.2.3