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

Commit fd0184f1 authored by Ben Murdoch's avatar Ben Murdoch
Browse files

Fix multiuser race in FeatureFlagsClassicDebug constructor. 

Under HSUM, the constructor may be called while the system is still
running as user 0. In this case, we register the iGET_FLAGS receiver
under user 0. However, the flippin app runs as user 10 so will then
see an empty response to it's GET_FLAGS ordered broadcast and crash.

This change installs a callback to capture the system switching
from user 0 to user 10 (or indeed, any subsequent user switch) and
ensure re-registers the receiver so it's running in the correct user.

Bug: 345443431
Flag: com.android.systemui.classic_flags_multi_user
Test: Manually verified; atest -c com.android.systemui.flags.FeatureFlagsClassicDebugTest
Change-Id: If8b5273963e67ab23fb10a9ad930fa9fa7b09590
parent 5eb9df51

packages/SystemUI/multivalentTests/src/com/android/systemui/flags/FeatureFlagsClassicDebugTest.kt

+11 −6
Original line number Diff line number Diff line
@@ -28,11 +28,13 @@ import androidx.test.filters.SmallTest 
import com.android.systemui.Flags.FLAG_SYSUI_TEAMFOOD

import com.android.systemui.SysuiTestCase

import com.android.systemui.settings.FakeUserTracker

import com.android.systemui.util.concurrency.FakeExecutor

import com.android.systemui.util.mockito.any

import com.android.systemui.util.mockito.eq

import com.android.systemui.util.mockito.nullable

import com.android.systemui.util.mockito.withArgCaptor

import com.android.systemui.util.settings.GlobalSettings

import com.android.systemui.util.time.FakeSystemClock

import com.google.common.truth.Truth.assertThat

import java.io.PrintWriter

import java.io.Serializable
@@ -68,6 +70,7 @@ class FeatureFlagsClassicDebugTest : SysuiTestCase() { 
    @Mock private lateinit var systemProperties: SystemPropertiesHelper

    @Mock private lateinit var resources: Resources

    @Mock private lateinit var restarter: Restarter

    private lateinit var fakeExecutor: FakeExecutor

    private lateinit var userTracker: FakeUserTracker

    private val flagMap = mutableMapOf<String, Flag<*>>()

    private lateinit var broadcastReceiver: BroadcastReceiver
@@ -83,6 +86,7 @@ class FeatureFlagsClassicDebugTest : SysuiTestCase() { 
        flagMap.put(teamfoodableFlagA.name, teamfoodableFlagA)

        flagMap.put(releasedFlagB.name, releasedFlagB)



        fakeExecutor = FakeExecutor(FakeSystemClock())

        userTracker = FakeUserTracker(onCreateCurrentUserContext = { mockContext })



        mFeatureFlagsClassicDebug =
@@ -95,7 +99,8 @@ class FeatureFlagsClassicDebugTest : SysuiTestCase() { 
                serverFlagReader,

                flagMap,

                restarter,

                userTracker

                userTracker,

                fakeExecutor,

            )

        mFeatureFlagsClassicDebug.init()

        verify(flagManager).onSettingsChangedAction = any()
@@ -325,14 +330,14 @@ class FeatureFlagsClassicDebugTest : SysuiTestCase() { 
        // trying to erase an id not in the map does nothing

        broadcastReceiver.onReceive(

            mockContext,

            Intent(FlagManager.ACTION_SET_FLAG).putExtra(FlagManager.EXTRA_NAME, "")

            Intent(FlagManager.ACTION_SET_FLAG).putExtra(FlagManager.EXTRA_NAME, ""),

        )

        verifyNoMoreInteractions(flagManager, globalSettings)



        // valid id with no value puts empty string in the setting

        broadcastReceiver.onReceive(

            mockContext,

            Intent(FlagManager.ACTION_SET_FLAG).putExtra(FlagManager.EXTRA_NAME, "1")

            Intent(FlagManager.ACTION_SET_FLAG).putExtra(FlagManager.EXTRA_NAME, "1"),

        )

        verifyPutData("1", "", numReads = 0)

    }
@@ -415,7 +420,7 @@ class FeatureFlagsClassicDebugTest : SysuiTestCase() { 
        serverFlagReader.setFlagValue(

            teamfoodableFlagA.namespace,

            teamfoodableFlagA.name,

            !teamfoodableFlagA.default

            !teamfoodableFlagA.default,

        )

        verify(restarter, never()).restartSystemUI(anyString())

    }
@@ -428,7 +433,7 @@ class FeatureFlagsClassicDebugTest : SysuiTestCase() { 
        serverFlagReader.setFlagValue(

            teamfoodableFlagA.namespace,

            teamfoodableFlagA.name,

            !teamfoodableFlagA.default

            !teamfoodableFlagA.default,

        )

        verify(restarter).restartSystemUI(anyString())

    }
@@ -441,7 +446,7 @@ class FeatureFlagsClassicDebugTest : SysuiTestCase() { 
        serverFlagReader.setFlagValue(

            teamfoodableFlagA.namespace,

            teamfoodableFlagA.name,

            teamfoodableFlagA.default

            teamfoodableFlagA.default,

        )

        verify(restarter, never()).restartSystemUI(anyString())

    }

packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsClassicDebug.java

+32 −8
Original line number Diff line number Diff line
@@ -42,7 +42,7 @@ import androidx.annotation.Nullable; 


import com.android.systemui.dagger.SysUISingleton;

import com.android.systemui.dagger.qualifiers.Main;

import com.android.systemui.settings.UserContextProvider;

import com.android.systemui.settings.UserTracker;

import com.android.systemui.util.settings.GlobalSettings;



import java.io.PrintWriter;
@@ -51,6 +51,7 @@ import java.util.Map; 
import java.util.Objects;

import java.util.TreeMap;

import java.util.concurrent.ConcurrentHashMap;

import java.util.concurrent.Executor;

import java.util.function.Consumer;



import javax.inject.Inject;
@@ -74,7 +75,6 @@ public class FeatureFlagsClassicDebug implements FeatureFlagsClassic { 
    static final String TAG = "SysUIFlags";



    private final FlagManager mFlagManager;

    private final Context mContext;

    private final GlobalSettings mGlobalSettings;

    private final Resources mResources;

    private final SystemPropertiesHelper mSystemProperties;
@@ -84,6 +84,8 @@ public class FeatureFlagsClassicDebug implements FeatureFlagsClassic { 
    private final Map<String, String> mStringFlagCache = new ConcurrentHashMap<>();

    private final Map<String, Integer> mIntFlagCache = new ConcurrentHashMap<>();

    private final Restarter mRestarter;

    private final UserTracker mUserTracker;

    private final Executor mMainExecutor;



    private final ServerFlagReader.ChangeListener mOnPropertiesChanged =

            new ServerFlagReader.ChangeListener() {
@@ -118,6 +120,18 @@ public class FeatureFlagsClassicDebug implements FeatureFlagsClassic { 
                }

            };



    private final UserTracker.Callback mUserChangedCallback =

            new UserTracker.Callback() {

                @Override

                public void onUserChanged(int newUser, @NonNull Context userContext) {

                    mContext.unregisterReceiver(mReceiver);

                    mContext = userContext;

                    registerReceiver();

                }

            };



    private Context mContext;



    @Inject

    public FeatureFlagsClassicDebug(

            FlagManager flagManager,
@@ -128,10 +142,11 @@ public class FeatureFlagsClassicDebug implements FeatureFlagsClassic { 
            ServerFlagReader serverFlagReader,

            @Named(ALL_FLAGS) Map<String, Flag<?>> allFlags,

            Restarter restarter,

            UserContextProvider userContextProvider) {

            UserTracker userTracker,

            @Main Executor executor) {

        mFlagManager = flagManager;

        if (classicFlagsMultiUser()) {

            mContext = userContextProvider.createCurrentUserContext(context);

            mContext = userTracker.createCurrentUserContext(context);

        } else {

            mContext = context;

        }
@@ -141,19 +156,28 @@ public class FeatureFlagsClassicDebug implements FeatureFlagsClassic { 
        mServerFlagReader = serverFlagReader;

        mAllFlags = allFlags;

        mRestarter = restarter;

        mUserTracker = userTracker;

        mMainExecutor = executor;

    }



    /** Call after construction to setup listeners. */

    void init() {

        IntentFilter filter = new IntentFilter();

        filter.addAction(ACTION_SET_FLAG);

        filter.addAction(ACTION_GET_FLAGS);

        mFlagManager.setOnSettingsChangedAction(

                suppressRestart -> restartSystemUI(suppressRestart, "Settings changed"));

        mFlagManager.setClearCacheAction(this::removeFromCache);

        registerReceiver();

        if (classicFlagsMultiUser()) {

            mUserTracker.addCallback(mUserChangedCallback, mMainExecutor);

        }

        mServerFlagReader.listenForChanges(mAllFlags.values(), mOnPropertiesChanged);

    }



    void registerReceiver() {

        IntentFilter filter = new IntentFilter();

        filter.addAction(ACTION_SET_FLAG);

        filter.addAction(ACTION_GET_FLAGS);

        mContext.registerReceiver(mReceiver, filter, null, null,

                Context.RECEIVER_EXPORTED_UNAUDITED);

        mServerFlagReader.listenForChanges(mAllFlags.values(), mOnPropertiesChanged);

    }



    @Override