Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 7f2bb15f authored by Fabian Kozynski's avatar Fabian Kozynski
Browse files

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
parent 928a1ea2

packages/SystemUI/src/com/android/systemui/qs/QSTileHost.java

+89 −43
Original line number Diff line number Diff line
@@ -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<QSFactory>, Dumpable {

    private static final String TAG = "QSTileHost";
@@ -89,11 +94,11 @@ public class QSTileHost implements QSHost, Tunable, PluginListener<QSFactory>, 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<Callback> mCallbacks = new ArrayList<>();

    @Nullable
@@ -113,13 +118,11 @@ public class QSTileHost implements QSHost, Tunable, PluginListener<QSFactory>, D 
    public QSTileHost(Context context,

            StatusBarIconController iconController,

            QSFactory defaultFactory,

            @Main Handler mainHandler,

            @Background Looper bgLooper,

            @Main Executor mainExecutor,

            PluginManager pluginManager,

            TunerService tunerService,

            Provider<AutoTileManager> autoTiles,

            DumpManager dumpManager,

            BroadcastDispatcher broadcastDispatcher,

            Optional<CentralSurfaces> centralSurfacesOptional,

            QSLogger qsLogger,

            UiEventLogger uiEventLogger,
@@ -137,7 +140,7 @@ public class QSTileHost implements QSHost, Tunable, PluginListener<QSFactory>, 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<QSFactory>, 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<QSFactory>, 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:

     * <ol>

     *     <li>Destroy any existing tile that's not one of the current tiles (in the setting)</li>

     *     <li>Create new tiles for those that don't already exist. If this tiles end up being

     *         not available, they'll also be destroyed.</li>

     *     <li>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.

     *         </li>

     * </ol>

     *

     * Additionally, if the user has changed, it'll do the following:

     * <ul>

     *     <li>Change the user for SystemUI tiles: {@link QSTile#userSwitch}.</li>

     *     <li>Destroy any {@link CustomTile} and recreate it for the new user.</li>

     * </ul>

     *

     * 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<QSFactory>, D 
        mCurrentUser = currentUser;

        List<String> 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<String> specs) {

        changeTileSpecs(tileSpecs -> tileSpecs.removeAll(specs));

        mMainExecutor.execute(() -> changeTileSpecs(tileSpecs -> tileSpecs.removeAll(specs)));

    }



    @Override
@@ -381,7 +421,7 @@ public class QSTileHost implements QSHost, Tunable, PluginListener<QSFactory>, 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");

        mMainExecutor.execute(() ->

                changeTileSpecs(tileSpecs -> {

                    if (tileSpecs.contains(spec)) return false;
@@ -392,15 +432,18 @@ public class QSTileHost implements QSHost, Tunable, PluginListener<QSFactory>, D 
                        tileSpecs.add(requestPosition, spec);

                    }

                    return true;

        });

                })

        );

    }



    void saveTilesToSettings(List<String> tileSpecs) {

    @MainThread

    private void saveTilesToSettings(List<String> tileSpecs) {

        mSecureSettings.putStringForUser(TILES_SETTING, TextUtils.join(",", tileSpecs),

                null /* tag */, false /* default */, mCurrentUser,

                true /* overrideable by restore */);

    }



    @MainThread

    private void changeTileSpecs(Predicate<List<String>> changeFunction) {

        final String setting = mSecureSettings.getStringForUser(TILES_SETTING, mCurrentUser);

        final List<String> tileSpecs = loadTileSpecs(mContext, setting);
@@ -420,29 +463,32 @@ public class QSTileHost implements QSHost, Tunable, PluginListener<QSFactory>, D 
     */

    public void addTile(ComponentName tile, boolean end) {

        String spec = CustomTile.toSpec(tile);

        if (!mTileSpecs.contains(spec)) {

            List<String> 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) {

    /**

     * This will call through {@link #changeTilesByUser}. It should only be used when a tile is

     * removed by a <b>user action</b> like {@code adb}.

     */

    public void removeTileByUser(ComponentName tile) {

        mMainExecutor.execute(() -> {

            List<String> newSpecs = new ArrayList<>(mTileSpecs);

        newSpecs.remove(CustomTile.toSpec(tile));

        changeTiles(mTileSpecs, newSpecs);

            if (newSpecs.remove(CustomTile.toSpec(tile))) {

                changeTilesByUser(mTileSpecs, newSpecs);

            }

        });

    }



    /**

     * Change the tiles triggered by the user editing.

     * <p>

     * 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<String> previousTiles, List<String> newTiles) {

    @MainThread

    public void changeTilesByUser(List<String> previousTiles, List<String> newTiles) {

        final List<String> copy = new ArrayList<>(previousTiles);

        final int NP = copy.size();

        for (int i = 0; i < NP; i++) {

packages/SystemUI/src/com/android/systemui/qs/customize/TileAdapter.java

+2 −2
Original line number Diff line number Diff line
@@ -182,7 +182,7 @@ public class TileAdapter extends RecyclerView.Adapter<Holder> 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<Holder> implements TileSta 
    /** */

    public void resetTileSpecs(List<String> specs) {

        // Notify the host so the tiles get removed callbacks.

        mHost.changeTiles(mCurrentSpecs, specs);

        mHost.changeTilesByUser(mCurrentSpecs, specs);

        setTileSpecs(specs);

    }

packages/SystemUI/src/com/android/systemui/qs/external/TileServiceManager.java

+1 −1
Original line number Diff line number Diff line
@@ -289,7 +289,7 @@ public class TileServiceManager { 
                }

            }



            mServices.getHost().removeTile(component);

            mServices.getHost().removeTile(CustomTile.toSpec(component));

        }

    };

}

packages/SystemUI/src/com/android/systemui/statusbar/phone/AutoTileManager.java

+1 −1
Original line number Diff line number Diff line
@@ -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);

            }

        }

packages/SystemUI/src/com/android/systemui/statusbar/phone/CentralSurfacesCommandQueueCallbacks.java

+1 −1
Original line number Diff line number Diff line
@@ -191,7 +191,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);

        }

    }