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

Commit 83b1f585 authored by Ted Bauer's avatar Ted Bauer
Browse files

Stage any flag on DeviceConfig writes. 

Currently, flags are staged to be on reboot by being written to the
staging namespace. The server-side explicitly writes to the staging
namespace. That means flags can still be toggled immediately locally,
and when the server-side doesn't have staging enabled. A race condition
on the first wifi connection means that sometimes flags can get
immediately toggled. This CL makes all writes stage, even local ones.

Bug: 326598713
Test: atest SettingsStateTest
Change-Id: I9a551843ef710bf074b40cbe581caf075cfafc07
parent 198545c4

packages/SettingsProvider/Android.bp

+1 −0
Original line number Diff line number Diff line
@@ -60,6 +60,7 @@ android_test { 
        // because this test is not an instrumentation test. (because the target runs in the system process.)

        "SettingsProviderLib",

        "androidx.test.rules",

        "device_config_service_flags_java",

        "flag-junit",

        "junit",

        "libaconfig_java_proto_lite",

packages/SettingsProvider/src/com/android/providers/settings/SettingsState.java

+31 −0
Original line number Diff line number Diff line
@@ -357,6 +357,15 @@ final class SettingsState { 
        }

    }



    @VisibleForTesting

    @GuardedBy("mLock")

    public void addAconfigDefaultValuesFromMap(

            @NonNull Map<String, Map<String, String>> defaultMap) {

        if (mNamespaceDefaults != null) {

            mNamespaceDefaults.putAll(defaultMap);

        }

    }



    @VisibleForTesting

    @GuardedBy("mLock")

    public static void loadAconfigDefaultValues(byte[] fileContents,
@@ -510,6 +519,28 @@ final class SettingsState { 
            return false;

        }



        // Aconfig flags are always boot stable, so we anytime we write one, we staged it to be

        // applied on reboot.

        if (Flags.stageAllAconfigFlags() && mNamespaceDefaults != null) {

            int slashIndex = name.indexOf("/");

            boolean stageFlag = isConfigSettingsKey(mKey)

                    && slashIndex != -1

                    && slashIndex != 0

                    && slashIndex != name.length();



            if (stageFlag) {

                String namespace = name.substring(0, slashIndex);

                String flag = name.substring(slashIndex + 1);



                boolean isAconfig = mNamespaceDefaults.containsKey(namespace)

                        && mNamespaceDefaults.get(namespace).containsKey(name);



                if (isAconfig) {

                    name = "staged/" + namespace + "*" + flag;

                }

            }

        }



        final boolean isNameTooLong = name.length() > SettingsState.MAX_LENGTH_PER_STRING;

        final boolean isValueTooLong =

                value != null && value.length() > SettingsState.MAX_LENGTH_PER_STRING;

packages/SettingsProvider/src/com/android/providers/settings/device_config_service.aconfig

+11 −0
Original line number Diff line number Diff line
@@ -14,3 +14,14 @@ flag { 
    bug: "311155098"

    is_fixed_read_only: true

}



flag {

    name: "stage_all_aconfig_flags"

    namespace: "core_experiments_team_internal"

    description: "Stage _all_ aconfig flags on writes, even local ones."

    bug: "326598713"

    is_fixed_read_only: true

    metadata {

        purpose: PURPOSE_BUGFIX

    }

}

packages/SettingsProvider/test/src/com/android/providers/settings/SettingsStateTest.java

+178 −32
Original line number Diff line number Diff line
@@ -15,13 +15,25 @@ 
 */

package com.android.providers.settings;



import static junit.framework.Assert.assertEquals;

import static junit.framework.Assert.assertFalse;

import static junit.framework.Assert.assertNull;

import static junit.framework.Assert.assertTrue;

import static junit.framework.Assert.fail;



import android.aconfig.Aconfig;

import android.aconfig.Aconfig.parsed_flag;

import android.aconfig.Aconfig.parsed_flags;

import android.os.Looper;

import android.test.AndroidTestCase;

import android.platform.test.annotations.RequiresFlagsDisabled;

import android.platform.test.annotations.RequiresFlagsEnabled;

import android.platform.test.flag.junit.CheckFlagsRule;

import android.platform.test.flag.junit.DeviceFlagsValueProvider;

import android.util.Xml;



import androidx.test.InstrumentationRegistry;

import androidx.test.runner.AndroidJUnit4;



import com.android.modules.utils.TypedXmlSerializer;



import com.google.common.base.Strings;
@@ -34,7 +46,18 @@ import java.util.HashMap; 
import java.util.List;

import java.util.Map;



public class SettingsStateTest extends AndroidTestCase {

import org.junit.After;

import org.junit.Before;

import org.junit.Rule;

import org.junit.Test;

import org.junit.runner.RunWith;



@RunWith(AndroidJUnit4.class)

public class SettingsStateTest {

    @Rule

    public final CheckFlagsRule mCheckFlagsRule =

            DeviceFlagsValueProvider.createCheckFlagsRule();



    public static final String CRAZY_STRING =

            "\u0000\u0001\u0002\u0003\u0004\u0005\u0006\u0007\u0008\u0009\n\u000b\u000c\r" +

                    "\u000e\u000f\u0010\u0011\u0012\u0013\u0014\u0015\u0016\u0017\u0018\u0019\u001a" +
@@ -76,25 +99,25 @@ public class SettingsStateTest extends AndroidTestCase { 


    private File mSettingsFile;



    @Override

    protected void setUp() {

        mSettingsFile = new File(getContext().getCacheDir(), "setting.xml");

    @Before

    public void setUp() {

        mSettingsFile = new File(InstrumentationRegistry.getContext().getCacheDir(), "setting.xml");

        mSettingsFile.delete();

    }



    @Override

    protected void tearDown() throws Exception {

    @After

    public void tearDown() throws Exception {

        if (mSettingsFile != null) {

            mSettingsFile.delete();

        }

        super.tearDown();

    }



    @Test

    public void testLoadValidAconfigProto() {

        int configKey = SettingsState.makeKey(SettingsState.SETTINGS_TYPE_CONFIG, 0);

        Object lock = new Object();

        SettingsState settingsState = new SettingsState(

                getContext(), lock, mSettingsFile, configKey,

                InstrumentationRegistry.getContext(), lock, mSettingsFile, configKey,

                SettingsState.MAX_BYTES_PER_APP_PACKAGE_UNLIMITED, Looper.getMainLooper());

        parsed_flags flags = parsed_flags

                .newBuilder()
@@ -129,11 +152,12 @@ public class SettingsStateTest extends AndroidTestCase { 
        }

    }



    @Test

    public void testSkipLoadingAconfigFlagWithMissingFields() {

        int configKey = SettingsState.makeKey(SettingsState.SETTINGS_TYPE_CONFIG, 0);

        Object lock = new Object();

        SettingsState settingsState = new SettingsState(

                getContext(), lock, mSettingsFile, configKey,

                InstrumentationRegistry.getContext(), lock, mSettingsFile, configKey,

                SettingsState.MAX_BYTES_PER_APP_PACKAGE_UNLIMITED, Looper.getMainLooper());



        parsed_flags flags = parsed_flags
@@ -155,12 +179,97 @@ public class SettingsStateTest extends AndroidTestCase { 
        }

    }



    @Test

    @RequiresFlagsEnabled(Flags.FLAG_STAGE_ALL_ACONFIG_FLAGS)

    public void testWritingAconfigFlagStages() {

        int configKey = SettingsState.makeKey(SettingsState.SETTINGS_TYPE_CONFIG, 0);

        Object lock = new Object();

        SettingsState settingsState = new SettingsState(

                InstrumentationRegistry.getContext(), lock, mSettingsFile, configKey,

                SettingsState.MAX_BYTES_PER_APP_PACKAGE_UNLIMITED, Looper.getMainLooper());

        parsed_flags flags = parsed_flags

                .newBuilder()

                .addParsedFlag(parsed_flag

                    .newBuilder()

                        .setPackage("com.android.flags")

                        .setName("flag5")

                        .setNamespace("test_namespace")

                        .setDescription("test flag")

                        .addBug("12345678")

                        .setState(Aconfig.flag_state.DISABLED)

                        .setPermission(Aconfig.flag_permission.READ_WRITE))

                .build();



        synchronized (lock) {

            Map<String, Map<String, String>> defaults = new HashMap<>();

            settingsState.loadAconfigDefaultValues(flags.toByteArray(), defaults);

            settingsState.addAconfigDefaultValuesFromMap(defaults);



            settingsState.insertSettingLocked("test_namespace/com.android.flags.flag5",

                    "true", null, false, "com.android.flags");

            settingsState.insertSettingLocked("test_namespace/com.android.flags.flag6",

                    "true", null, false, "com.android.flags");



            assertEquals("true",

                    settingsState

                        .getSettingLocked("staged/test_namespace*com.android.flags.flag5")

                        .getValue());

            assertEquals(null,

                    settingsState

                        .getSettingLocked("test_namespace/com.android.flags.flag5")

                        .getValue());



            assertEquals(null,

                    settingsState

                        .getSettingLocked("staged/test_namespace*com.android.flags.flag6")

                        .getValue());

            assertEquals("true",

                    settingsState

                        .getSettingLocked("test_namespace/com.android.flags.flag6")

                        .getValue());

        }

    }



    @Test

    @RequiresFlagsDisabled(Flags.FLAG_LOAD_ACONFIG_DEFAULTS)

    public void testAddingAconfigMapOnNullIsNoOp() {

        int configKey = SettingsState.makeKey(SettingsState.SETTINGS_TYPE_CONFIG, 0);

        Object lock = new Object();

        SettingsState settingsState = new SettingsState(

                InstrumentationRegistry.getContext(), lock, mSettingsFile, configKey,

                SettingsState.MAX_BYTES_PER_APP_PACKAGE_UNLIMITED, Looper.getMainLooper());



        parsed_flags flags = parsed_flags

                .newBuilder()

                .addParsedFlag(parsed_flag

                    .newBuilder()

                        .setPackage("com.android.flags")

                        .setName("flag5")

                        .setNamespace("test_namespace")

                        .setDescription("test flag")

                        .addBug("12345678")

                        .setState(Aconfig.flag_state.DISABLED)

                        .setPermission(Aconfig.flag_permission.READ_WRITE))

                .build();



        synchronized (lock) {

            Map<String, Map<String, String>> defaults = new HashMap<>();

            settingsState.loadAconfigDefaultValues(flags.toByteArray(), defaults);

            settingsState.addAconfigDefaultValuesFromMap(defaults);



            assertEquals(null, settingsState.getAconfigDefaultValues());

        }



    }



    @Test

    public void testInvalidAconfigProtoDoesNotCrash() {

        Map<String, Map<String, String>> defaults = new HashMap<>();

        SettingsState settingsState = getSettingStateObject();

        settingsState.loadAconfigDefaultValues("invalid protobuf".getBytes(), defaults);

    }



    @Test

    public void testIsBinary() {

        assertFalse(SettingsState.isBinary(" abc 日本語"));
@@ -191,6 +300,7 @@ public class SettingsStateTest extends AndroidTestCase { 
    }



    /** Make sure we won't pass invalid characters to XML serializer. */

    @Test

    public void testWriteReadNoCrash() throws Exception {

        ByteArrayOutputStream os = new ByteArrayOutputStream();
@@ -233,11 +343,14 @@ public class SettingsStateTest extends AndroidTestCase { 
    /**

     * Make sure settings can be written to a file and also can be read.

     */

    @Test

    public void testReadWrite() {

        final Object lock = new Object();



        assertFalse(mSettingsFile.exists());

        final SettingsState ssWriter = new SettingsState(getContext(), lock, mSettingsFile, 1,

        final SettingsState ssWriter =

                new SettingsState(

                        InstrumentationRegistry.getContext(), lock, mSettingsFile, 1,

                        SettingsState.MAX_BYTES_PER_APP_PACKAGE_UNLIMITED, Looper.getMainLooper());

        ssWriter.setVersionLocked(SettingsState.SETTINGS_VERSION_NEW_ENCODING);
@@ -250,7 +363,9 @@ public class SettingsStateTest extends AndroidTestCase { 
        }

        ssWriter.waitForHandler();

        assertTrue(mSettingsFile.exists());

        final SettingsState ssReader = new SettingsState(getContext(), lock, mSettingsFile, 1,

        final SettingsState ssReader =

                new SettingsState(

                        InstrumentationRegistry.getContext(), lock, mSettingsFile, 1,

                        SettingsState.MAX_BYTES_PER_APP_PACKAGE_UNLIMITED, Looper.getMainLooper());



        synchronized (lock) {
@@ -264,6 +379,7 @@ public class SettingsStateTest extends AndroidTestCase { 
    /**

     * In version 120, value "null" meant {code NULL}.

     */

    @Test

    public void testUpgrade() throws Exception {

        final Object lock = new Object();

        final PrintStream os = new PrintStream(new FileOutputStream(mSettingsFile));
@@ -276,7 +392,9 @@ public class SettingsStateTest extends AndroidTestCase { 
                        "</settings>");

        os.close();



        final SettingsState ss = new SettingsState(getContext(), lock, mSettingsFile, 1,

        final SettingsState ss =

                new SettingsState(

                        InstrumentationRegistry.getContext(), lock, mSettingsFile, 1,

                        SettingsState.MAX_BYTES_PER_APP_PACKAGE_UNLIMITED, Looper.getMainLooper());

        synchronized (lock) {

            SettingsState.Setting s;
@@ -294,6 +412,7 @@ public class SettingsStateTest extends AndroidTestCase { 
        }

    }



    @Test

    public void testInitializeSetting_preserveFlagNotSet() {

        SettingsState settingsWriter = getSettingStateObject();

        settingsWriter.insertSettingLocked(SETTING_NAME, "1", null, false, TEST_PACKAGE);
@@ -304,6 +423,7 @@ public class SettingsStateTest extends AndroidTestCase { 
        assertFalse(settingsReader.getSettingLocked(SETTING_NAME).isValuePreservedInRestore());

    }



    @Test

    public void testModifySetting_preserveFlagSet() {

        SettingsState settingsWriter = getSettingStateObject();

        settingsWriter.insertSettingLocked(SETTING_NAME, "1", null, false, TEST_PACKAGE);
@@ -315,6 +435,7 @@ public class SettingsStateTest extends AndroidTestCase { 
        assertTrue(settingsReader.getSettingLocked(SETTING_NAME).isValuePreservedInRestore());

    }



    @Test

    public void testModifySettingOverrideableByRestore_preserveFlagNotSet() {

        SettingsState settingsWriter = getSettingStateObject();

        settingsWriter.insertSettingLocked(SETTING_NAME, "1", null, false, TEST_PACKAGE);
@@ -327,6 +448,7 @@ public class SettingsStateTest extends AndroidTestCase { 
        assertFalse(settingsReader.getSettingLocked(SETTING_NAME).isValuePreservedInRestore());

    }



    @Test

    public void testModifySettingOverrideableByRestore_preserveFlagAlreadySet_flagValueUnchanged() {

        SettingsState settingsWriter = getSettingStateObject();

        // Init the setting.
@@ -344,6 +466,7 @@ public class SettingsStateTest extends AndroidTestCase { 
        assertTrue(settingsReader.getSettingLocked(SETTING_NAME).isValuePreservedInRestore());

    }



    @Test

    public void testResetSetting_preservedFlagIsReset() {

        SettingsState settingsState = getSettingStateObject();

        // Initialize the setting.
@@ -356,6 +479,7 @@ public class SettingsStateTest extends AndroidTestCase { 


    }



    @Test

    public void testModifySettingBySystemPackage_sameValue_preserveFlagNotSet() {

        SettingsState settingsState = getSettingStateObject();

        // Initialize the setting.
@@ -366,6 +490,7 @@ public class SettingsStateTest extends AndroidTestCase { 
        assertFalse(settingsState.getSettingLocked(SETTING_NAME).isValuePreservedInRestore());

    }



    @Test

    public void testModifySettingBySystemPackage_newValue_preserveFlagSet() {

        SettingsState settingsState = getSettingStateObject();

        // Initialize the setting.
@@ -377,12 +502,15 @@ public class SettingsStateTest extends AndroidTestCase { 
    }



    private SettingsState getSettingStateObject() {

        SettingsState settingsState = new SettingsState(getContext(), mLock, mSettingsFile, 1,

        SettingsState settingsState =

                new SettingsState(

                        InstrumentationRegistry.getContext(), mLock, mSettingsFile, 1,

                        SettingsState.MAX_BYTES_PER_APP_PACKAGE_UNLIMITED, Looper.getMainLooper());

        settingsState.setVersionLocked(SettingsState.SETTINGS_VERSION_NEW_ENCODING);

        return settingsState;

    }



    @Test

    public void testInsertSetting_memoryUsage() {

        SettingsState settingsState = getSettingStateObject();

        // No exception should be thrown when there is no cap
@@ -390,7 +518,9 @@ public class SettingsStateTest extends AndroidTestCase { 
                null, false, "p1");

        settingsState.deleteSettingLocked(SETTING_NAME);



        settingsState = new SettingsState(getContext(), mLock, mSettingsFile, 1,

        settingsState =

                new SettingsState(

                        InstrumentationRegistry.getContext(), mLock, mSettingsFile, 1,

                        SettingsState.MAX_BYTES_PER_APP_PACKAGE_LIMITED, Looper.getMainLooper());

        // System package doesn't have memory usage limit

        settingsState.insertSettingLocked(SETTING_NAME, Strings.repeat("A", 20001),
@@ -425,8 +555,11 @@ public class SettingsStateTest extends AndroidTestCase { 
        }

    }



    @Test

    public void testMemoryUsagePerPackage() {

        SettingsState settingsState = new SettingsState(getContext(), mLock, mSettingsFile, 1,

        SettingsState settingsState =

                new SettingsState(

                        InstrumentationRegistry.getContext(), mLock, mSettingsFile, 1,

                        SettingsState.MAX_BYTES_PER_APP_PACKAGE_LIMITED, Looper.getMainLooper());



        // Test inserting one key with default
@@ -512,8 +645,11 @@ public class SettingsStateTest extends AndroidTestCase { 
        assertEquals(expectedMemUsage, settingsState.getMemoryUsage(TEST_PACKAGE));

    }



    @Test

    public void testLargeSettingKey() {

        SettingsState settingsState = new SettingsState(getContext(), mLock, mSettingsFile, 1,

        SettingsState settingsState =

                new SettingsState(

                        InstrumentationRegistry.getContext(), mLock, mSettingsFile, 1,

                        SettingsState.MAX_BYTES_PER_APP_PACKAGE_LIMITED, Looper.getMainLooper());

        final String largeKey = Strings.repeat("A", SettingsState.MAX_LENGTH_PER_STRING + 1);

        final String testValue = "testValue";
@@ -535,8 +671,11 @@ public class SettingsStateTest extends AndroidTestCase { 
        }

    }



    @Test

    public void testLargeSettingValue() {

        SettingsState settingsState = new SettingsState(getContext(), mLock, mSettingsFile, 1,

        SettingsState settingsState =

                new SettingsState(

                        InstrumentationRegistry.getContext(), mLock, mSettingsFile, 1,

                        SettingsState.MAX_BYTES_PER_APP_PACKAGE_UNLIMITED, Looper.getMainLooper());

        final String testKey = "testKey";

        final String largeValue = Strings.repeat("A", SettingsState.MAX_LENGTH_PER_STRING + 1);
@@ -558,11 +697,12 @@ public class SettingsStateTest extends AndroidTestCase { 
        }

    }



    @Test

    public void testApplyStagedConfigValues() {

        int configKey = SettingsState.makeKey(SettingsState.SETTINGS_TYPE_CONFIG, 0);

        Object lock = new Object();

        SettingsState settingsState = new SettingsState(

                getContext(), lock, mSettingsFile, configKey,

                InstrumentationRegistry.getContext(), lock, mSettingsFile, configKey,

                SettingsState.MAX_BYTES_PER_APP_PACKAGE_UNLIMITED, Looper.getMainLooper());



        synchronized (lock) {
@@ -578,7 +718,8 @@ public class SettingsStateTest extends AndroidTestCase { 
            assertEquals(VALUE2, settingsState.getSettingLocked(FLAG_NAME_2).getValue());

        }



        settingsState = new SettingsState(getContext(), lock, mSettingsFile, configKey,

        settingsState = new SettingsState(

                InstrumentationRegistry.getContext(), lock, mSettingsFile, configKey,

                SettingsState.MAX_BYTES_PER_APP_PACKAGE_UNLIMITED, Looper.getMainLooper());



        synchronized (lock) {
@@ -589,6 +730,7 @@ public class SettingsStateTest extends AndroidTestCase { 
        }

    }



    @Test

    public void testStagingTransformation() {

        assertEquals(INVALID_STAGED_FLAG_1,

                SettingsState.createRealFlagName(INVALID_STAGED_FLAG_1));
@@ -603,11 +745,12 @@ public class SettingsStateTest extends AndroidTestCase { 
                SettingsState.createRealFlagName(VALID_STAGED_FLAG_1));

    }



    @Test

    public void testInvalidStagedFlagsUnaffectedByReboot() {

        int configKey = SettingsState.makeKey(SettingsState.SETTINGS_TYPE_CONFIG, 0);

        Object lock = new Object();

        SettingsState settingsState = new SettingsState(

                getContext(), lock, mSettingsFile, configKey,

                InstrumentationRegistry.getContext(), lock, mSettingsFile, configKey,

                SettingsState.MAX_BYTES_PER_APP_PACKAGE_UNLIMITED, Looper.getMainLooper());



        synchronized (lock) {
@@ -620,7 +763,8 @@ public class SettingsStateTest extends AndroidTestCase { 
            assertEquals(VALUE2, settingsState.getSettingLocked(INVALID_STAGED_FLAG_1).getValue());

        }



        settingsState = new SettingsState(getContext(), lock, mSettingsFile, configKey,

        settingsState = new SettingsState(

                InstrumentationRegistry.getContext(), lock, mSettingsFile, configKey,

                SettingsState.MAX_BYTES_PER_APP_PACKAGE_UNLIMITED, Looper.getMainLooper());



        synchronized (lock) {
@@ -628,6 +772,7 @@ public class SettingsStateTest extends AndroidTestCase { 
        }

    }



    @Test

    public void testsetSettingsLockedKeepTrunkDefault() throws Exception {

        final PrintStream os = new PrintStream(new FileOutputStream(mSettingsFile));

        os.print(
@@ -648,7 +793,7 @@ public class SettingsStateTest extends AndroidTestCase { 
        int configKey = SettingsState.makeKey(SettingsState.SETTINGS_TYPE_CONFIG, 0);



        SettingsState settingsState = new SettingsState(

                getContext(), mLock, mSettingsFile, configKey,

                InstrumentationRegistry.getContext(), mLock, mSettingsFile, configKey,

                SettingsState.MAX_BYTES_PER_APP_PACKAGE_UNLIMITED, Looper.getMainLooper());



        String prefix = "test_namespace";
@@ -705,6 +850,7 @@ public class SettingsStateTest extends AndroidTestCase { 
        }

    }



    @Test

    public void testsetSettingsLockedNoTrunkDefault() throws Exception {

        final PrintStream os = new PrintStream(new FileOutputStream(mSettingsFile));

        os.print(
@@ -720,7 +866,7 @@ public class SettingsStateTest extends AndroidTestCase { 
        int configKey = SettingsState.makeKey(SettingsState.SETTINGS_TYPE_CONFIG, 0);



        SettingsState settingsState = new SettingsState(

                getContext(), mLock, mSettingsFile, configKey,

                InstrumentationRegistry.getContext(), mLock, mSettingsFile, configKey,

                SettingsState.MAX_BYTES_PER_APP_PACKAGE_UNLIMITED, Looper.getMainLooper());



        Map<String, String> keyValues =