From 4262f949e38af248098ff20cad5d44fe08d594ba Mon Sep 17 00:00:00 2001 From: Xiang Wang Date: Tue, 2 Aug 2022 11:02:46 -0700 Subject: [PATCH 1/2] Check if it's game package before updating configs Rename "invalid config" to "inactive config and gate the warning behind DEBUG flag, as it's expected for games to opt in game modes Bug: b/240335717 Test: atest GameManagerServiceTests Change-Id: I0aae9bb4c68e01a8d4797fd6595535467f7d1fe6 --- .../server/app/GameManagerService.java | 30 ++++++++++++------- .../server/app/GameManagerServiceTests.java | 26 ++++++++-------- 2 files changed, 32 insertions(+), 24 deletions(-) diff --git a/services/core/java/com/android/server/app/GameManagerService.java b/services/core/java/com/android/server/app/GameManagerService.java index dd73cbed06d90..99ce925e0b6d6 100644 --- a/services/core/java/com/android/server/app/GameManagerService.java +++ b/services/core/java/com/android/server/app/GameManagerService.java @@ -333,7 +333,7 @@ public final class GameManagerService extends IGameManagerService.Stub { removeMessages(POPULATE_GAME_MODE_SETTINGS, msg.obj); final int userId = (int) msg.obj; final String[] packageNames = getInstalledGamePackageNames(userId); - updateConfigsForUser(userId, packageNames); + updateConfigsForUser(userId, false /*checkGamePackage*/, packageNames); break; } case SET_GAME_STATE: { @@ -402,7 +402,8 @@ public final class GameManagerService extends IGameManagerService.Stub { @Override public void onPropertiesChanged(Properties properties) { final String[] packageNames = properties.getKeyset().toArray(new String[0]); - updateConfigsForUser(ActivityManager.getCurrentUser(), packageNames); + updateConfigsForUser(ActivityManager.getCurrentUser(), true /*checkGamePackage*/, + packageNames); } @Override @@ -718,7 +719,7 @@ public final class GameManagerService extends IGameManagerService.Stub { mFps = fpsStr; } - public boolean isValid() { + public boolean isActive() { return (mGameMode == GameManager.GAME_MODE_STANDARD || mGameMode == GameManager.GAME_MODE_PERFORMANCE || mGameMode == GameManager.GAME_MODE_BATTERY) @@ -810,15 +811,15 @@ public final class GameManagerService extends IGameManagerService.Stub { * Insert a new GameModeConfiguration */ public void addModeConfig(GameModeConfiguration config) { - if (config.isValid()) { + if (config.isActive()) { mModeConfigs.put(config.getGameMode(), config); } else { - Slog.w(TAG, "Invalid game mode config for " + Slog.w(TAG, "Attempt to add inactive game mode config for " + mPackageName + ":" + config.toString()); } } - public boolean isValid() { + public boolean isActive() { return mModeConfigs.size() > 0 || mBatteryModeOptedIn || mPerfModeOptedIn; } @@ -1570,20 +1571,27 @@ public final class GameManagerService extends IGameManagerService.Stub { * @hide */ @VisibleForTesting - void updateConfigsForUser(@UserIdInt int userId, String... packageNames) { + void updateConfigsForUser(@UserIdInt int userId, boolean checkGamePackage, + String... packageNames) { + if (checkGamePackage) { + packageNames = Arrays.stream(packageNames).filter( + p -> isPackageGame(p, userId)).toArray(String[]::new); + } try { synchronized (mDeviceConfigLock) { for (final String packageName : packageNames) { final GamePackageConfiguration config = new GamePackageConfiguration(packageName, userId); - if (config.isValid()) { + if (config.isActive()) { if (DEBUG) { Slog.i(TAG, "Adding config: " + config.toString()); } mConfigs.put(packageName, config); } else { - Slog.w(TAG, "Invalid package config for " - + config.getPackageName() + ":" + config.toString()); + if (DEBUG) { + Slog.w(TAG, "Inactive package config for " + + config.getPackageName() + ":" + config.toString()); + } mConfigs.remove(packageName); } } @@ -1761,7 +1769,7 @@ public final class GameManagerService extends IGameManagerService.Stub { } switch (intent.getAction()) { case ACTION_PACKAGE_ADDED: - updateConfigsForUser(userId, packageName); + updateConfigsForUser(userId, true /*checkGamePackage*/, packageName); break; case ACTION_PACKAGE_REMOVED: disableCompatScale(packageName); diff --git a/services/tests/mockingservicestests/src/com/android/server/app/GameManagerServiceTests.java b/services/tests/mockingservicestests/src/com/android/server/app/GameManagerServiceTests.java index d675b0aa4973a..d325e47f76879 100644 --- a/services/tests/mockingservicestests/src/com/android/server/app/GameManagerServiceTests.java +++ b/services/tests/mockingservicestests/src/com/android/server/app/GameManagerServiceTests.java @@ -450,13 +450,13 @@ public class GameManagerServiceTests { startUser(gameManagerService, USER_ID_1); - gameManagerService.updateConfigsForUser(USER_ID_1, mPackageName); + gameManagerService.updateConfigsForUser(USER_ID_1, true, mPackageName); mockModifyGameModeGranted(); assertEquals(GameManager.GAME_MODE_UNSUPPORTED, gameManagerService.getGameMode(mPackageName, USER_ID_1)); // We need to make sure the mode is supported before setting it. mockDeviceConfigAll(); - gameManagerService.updateConfigsForUser(USER_ID_1, mPackageName); + gameManagerService.updateConfigsForUser(USER_ID_1, true, mPackageName); gameManagerService.setGameMode(mPackageName, GameManager.GAME_MODE_STANDARD, USER_ID_1); assertEquals(GameManager.GAME_MODE_STANDARD, gameManagerService.getGameMode(mPackageName, USER_ID_1)); @@ -534,8 +534,8 @@ public class GameManagerServiceTests { startUser(gameManagerService, USER_ID_1); startUser(gameManagerService, USER_ID_2); - gameManagerService.updateConfigsForUser(USER_ID_1, mPackageName); - gameManagerService.updateConfigsForUser(USER_ID_2, mPackageName); + gameManagerService.updateConfigsForUser(USER_ID_1, true, mPackageName); + gameManagerService.updateConfigsForUser(USER_ID_2, true, mPackageName); // Set User 1 to Standard gameManagerService.setGameMode(mPackageName, GameManager.GAME_MODE_STANDARD, USER_ID_1); @@ -563,7 +563,7 @@ public class GameManagerServiceTests { if (gameManagerService == null) { gameManagerService = new GameManagerService(mMockContext, mTestLooper.getLooper()); startUser(gameManagerService, USER_ID_1); - gameManagerService.updateConfigsForUser(USER_ID_1, mPackageName); + gameManagerService.updateConfigsForUser(USER_ID_1, true, mPackageName); } ArraySet reportedModes = new ArraySet<>(); int[] modes = gameManagerService.getAvailableGameModes(mPackageName); @@ -582,7 +582,7 @@ public class GameManagerServiceTests { if (gameManagerService == null) { gameManagerService = new GameManagerService(mMockContext, mTestLooper.getLooper()); startUser(gameManagerService, USER_ID_1); - gameManagerService.updateConfigsForUser(USER_ID_1, mPackageName); + gameManagerService.updateConfigsForUser(USER_ID_1, true, mPackageName); } GameManagerService.GamePackageConfiguration config = gameManagerService.getConfig(mPackageName); @@ -591,7 +591,7 @@ public class GameManagerServiceTests { private void checkAngleEnabled(GameManagerService gameManagerService, int gameMode, boolean angleEnabled) { - gameManagerService.updateConfigsForUser(USER_ID_1, mPackageName); + gameManagerService.updateConfigsForUser(USER_ID_1, true, mPackageName); // Validate GamePackageConfiguration returns the correct value. GameManagerService.GamePackageConfiguration config = @@ -604,7 +604,7 @@ public class GameManagerServiceTests { private void checkLoadingBoost(GameManagerService gameManagerService, int gameMode, int loadingBoost) { - gameManagerService.updateConfigsForUser(USER_ID_1, mPackageName); + gameManagerService.updateConfigsForUser(USER_ID_1, true, mPackageName); // Validate GamePackageConfiguration returns the correct value. GameManagerService.GamePackageConfiguration config = @@ -621,7 +621,7 @@ public class GameManagerServiceTests { if (gameManagerService == null) { gameManagerService = new GameManagerService(mMockContext, mTestLooper.getLooper()); startUser(gameManagerService, USER_ID_1); - gameManagerService.updateConfigsForUser(USER_ID_1, mPackageName); + gameManagerService.updateConfigsForUser(USER_ID_1, true, mPackageName); } GameManagerService.GamePackageConfiguration config = gameManagerService.getConfig(mPackageName); @@ -1091,7 +1091,7 @@ public class GameManagerServiceTests { GameManagerService gameManagerService = new GameManagerService(mMockContext, mTestLooper.getLooper()); startUser(gameManagerService, USER_ID_1); - gameManagerService.updateConfigsForUser(USER_ID_1, mPackageName); + gameManagerService.updateConfigsForUser(USER_ID_1, true, mPackageName); GameManagerService.GamePackageConfiguration config = gameManagerService.getConfig(mPackageName); assertNull(config.getGameModeConfiguration(GameManager.GAME_MODE_PERFORMANCE)); @@ -1109,7 +1109,7 @@ public class GameManagerServiceTests { new GameManagerService(mMockContext, mTestLooper.getLooper()); startUser(gameManagerService, USER_ID_1); gameManagerService.setGameMode(mPackageName, GameManager.GAME_MODE_PERFORMANCE, USER_ID_1); - gameManagerService.updateConfigsForUser(USER_ID_1, mPackageName); + gameManagerService.updateConfigsForUser(USER_ID_1, true, mPackageName); assertEquals(GameManager.GAME_MODE_UNSUPPORTED, gameManagerService.getGameMode(mPackageName, USER_ID_1)); } @@ -1126,7 +1126,7 @@ public class GameManagerServiceTests { new GameManagerService(mMockContext, mTestLooper.getLooper()); startUser(gameManagerService, USER_ID_1); gameManagerService.setGameMode(mPackageName, GameManager.GAME_MODE_BATTERY, USER_ID_1); - gameManagerService.updateConfigsForUser(USER_ID_1, mPackageName); + gameManagerService.updateConfigsForUser(USER_ID_1, true, mPackageName); assertEquals(GameManager.GAME_MODE_STANDARD, gameManagerService.getGameMode(mPackageName, USER_ID_1)); } @@ -1143,7 +1143,7 @@ public class GameManagerServiceTests { new GameManagerService(mMockContext, mTestLooper.getLooper()); startUser(gameManagerService, USER_ID_1); gameManagerService.setGameMode(mPackageName, GameManager.GAME_MODE_UNSUPPORTED, USER_ID_1); - gameManagerService.updateConfigsForUser(USER_ID_1, mPackageName); + gameManagerService.updateConfigsForUser(USER_ID_1, true, mPackageName); assertEquals(GameManager.GAME_MODE_STANDARD, gameManagerService.getGameMode(mPackageName, USER_ID_1)); } From 24a64eac53d484354b714ef2e4219b0f3844c069 Mon Sep 17 00:00:00 2001 From: Xiang Wang Date: Tue, 2 Aug 2022 11:02:46 -0700 Subject: [PATCH 2/2] Guard the mode configs map in GamePackageConfiguration with lock Synchronizes the getter and setter of GameModeConfiguration as the sub fields can be read/write by different callers Bug: b/240335717 Test: atest GameManagerServiceTests Change-Id: Iac718a6a5d91e7aa3452b5cdb9aaee587f6f7773 --- .../server/app/GameManagerService.java | 162 ++++++++---------- 1 file changed, 68 insertions(+), 94 deletions(-) diff --git a/services/core/java/com/android/server/app/GameManagerService.java b/services/core/java/com/android/server/app/GameManagerService.java index 99ce925e0b6d6..d5e893984bf37 100644 --- a/services/core/java/com/android/server/app/GameManagerService.java +++ b/services/core/java/com/android/server/app/GameManagerService.java @@ -554,7 +554,9 @@ public final class GameManagerService extends IGameManagerService.Stub { private static final String GAME_MODE_CONFIG_NODE_NAME = "game-mode-config"; private final String mPackageName; - private final ArrayMap mModeConfigs; + private final Object mModeConfigLock = new Object(); + @GuardedBy("mModeConfigLock") + private final ArrayMap mModeConfigs = new ArrayMap<>(); private boolean mPerfModeOptedIn; private boolean mBatteryModeOptedIn; private boolean mAllowDownscale; @@ -563,7 +565,6 @@ public final class GameManagerService extends IGameManagerService.Stub { GamePackageConfiguration(String packageName, int userId) { mPackageName = packageName; - mModeConfigs = new ArrayMap<>(); try { final ApplicationInfo ai = mPackageManager.getApplicationInfoAsUser(packageName, PackageManager.GET_META_DATA, userId); @@ -695,11 +696,11 @@ public final class GameManagerService extends IGameManagerService.Stub { return mGameMode; } - public String getScaling() { + public synchronized String getScaling() { return mScaling; } - public int getFps() { + public synchronized int getFps() { return GameManagerService.getFpsInt(mFps); } @@ -711,11 +712,11 @@ public final class GameManagerService extends IGameManagerService.Stub { return mLoadingBoostDuration; } - public void setScaling(String scaling) { + public synchronized void setScaling(String scaling) { mScaling = scaling; } - public void setFpsStr(String fpsStr) { + public synchronized void setFpsStr(String fpsStr) { mFps = fpsStr; } @@ -762,8 +763,10 @@ public final class GameManagerService extends IGameManagerService.Stub { private int getAvailableGameModesBitfield() { int field = 0; - for (final int mode : mModeConfigs.keySet()) { - field |= modeToBitmask(mode); + synchronized (mModeConfigLock) { + for (final int mode : mModeConfigs.keySet()) { + field |= modeToBitmask(mode); + } } if (mBatteryModeOptedIn) { field |= modeToBitmask(GameManager.GAME_MODE_BATTERY); @@ -804,7 +807,9 @@ public final class GameManagerService extends IGameManagerService.Stub { * @return The package's GameModeConfiguration for the provided mode or null if absent */ public GameModeConfiguration getGameModeConfiguration(@GameMode int gameMode) { - return mModeConfigs.get(gameMode); + synchronized (mModeConfigLock) { + return mModeConfigs.get(gameMode); + } } /** @@ -812,7 +817,9 @@ public final class GameManagerService extends IGameManagerService.Stub { */ public void addModeConfig(GameModeConfiguration config) { if (config.isActive()) { - mModeConfigs.put(config.getGameMode(), config); + synchronized (mModeConfigLock) { + mModeConfigs.put(config.getGameMode(), config); + } } else { Slog.w(TAG, "Attempt to add inactive game mode config for " + mPackageName + ":" + config.toString()); @@ -820,11 +827,15 @@ public final class GameManagerService extends IGameManagerService.Stub { } public boolean isActive() { - return mModeConfigs.size() > 0 || mBatteryModeOptedIn || mPerfModeOptedIn; + synchronized (mModeConfigLock) { + return mModeConfigs.size() > 0 || mBatteryModeOptedIn || mPerfModeOptedIn; + } } public String toString() { - return "[Name:" + mPackageName + " Modes: " + mModeConfigs.toString() + "]"; + synchronized (mModeConfigLock) { + return "[Name:" + mPackageName + " Modes: " + mModeConfigs.toString() + "]"; + } } } @@ -895,15 +906,7 @@ public final class GameManagerService extends IGameManagerService.Stub { } private @GameMode int[] getAvailableGameModesUnchecked(String packageName) { - GamePackageConfiguration config = null; - synchronized (mOverrideConfigLock) { - config = mOverrideConfigs.get(packageName); - } - if (config == null) { - synchronized (mDeviceConfigLock) { - config = mConfigs.get(packageName); - } - } + final GamePackageConfiguration config = getConfig(packageName); if (config == null) { return new int[]{}; } @@ -1056,19 +1059,19 @@ public final class GameManagerService extends IGameManagerService.Stub { if (gameMode == GameManager.GAME_MODE_UNSUPPORTED) { return false; } - + final GamePackageConfiguration config; synchronized (mDeviceConfigLock) { - final GamePackageConfiguration config = mConfigs.get(packageName); + config = mConfigs.get(packageName); if (config == null) { return false; } - GamePackageConfiguration.GameModeConfiguration gameModeConfiguration = - config.getGameModeConfiguration(gameMode); - if (gameModeConfiguration == null) { - return false; - } - return gameModeConfiguration.getUseAngle(); } + GamePackageConfiguration.GameModeConfiguration gameModeConfiguration = + config.getGameModeConfiguration(gameMode); + if (gameModeConfiguration == null) { + return false; + } + return gameModeConfiguration.getUseAngle(); } /** @@ -1083,19 +1086,19 @@ public final class GameManagerService extends IGameManagerService.Stub { if (gameMode == GameManager.GAME_MODE_UNSUPPORTED) { return -1; } - + final GamePackageConfiguration config; synchronized (mDeviceConfigLock) { - final GamePackageConfiguration config = mConfigs.get(packageName); - if (config == null) { - return -1; - } - GamePackageConfiguration.GameModeConfiguration gameModeConfiguration = - config.getGameModeConfiguration(gameMode); - if (gameModeConfiguration == null) { - return -1; - } - return gameModeConfiguration.getLoadingBoostDuration(); + config = mConfigs.get(packageName); } + if (config == null) { + return -1; + } + GamePackageConfiguration.GameModeConfiguration gameModeConfiguration = + config.getGameModeConfiguration(gameMode); + if (gameModeConfiguration == null) { + return -1; + } + return gameModeConfiguration.getLoadingBoostDuration(); } /** @@ -1365,18 +1368,7 @@ public final class GameManagerService extends IGameManagerService.Stub { resetFps(packageName, userId); return; } - GamePackageConfiguration packageConfig = null; - - synchronized (mOverrideConfigLock) { - packageConfig = mOverrideConfigs.get(packageName); - } - - if (packageConfig == null) { - synchronized (mDeviceConfigLock) { - packageConfig = mConfigs.get(packageName); - } - } - + final GamePackageConfiguration packageConfig = getConfig(packageName); if (packageConfig == null) { disableCompatScale(packageName); Slog.v(TAG, "Package configuration not found for " + packageName); @@ -1405,34 +1397,34 @@ public final class GameManagerService extends IGameManagerService.Stub { } } // Adding override game mode configuration of the given package name + GamePackageConfiguration overrideConfig; synchronized (mOverrideConfigLock) { // look for the existing override GamePackageConfiguration - GamePackageConfiguration overrideConfig = mOverrideConfigs.get(packageName); + overrideConfig = mOverrideConfigs.get(packageName); if (overrideConfig == null) { overrideConfig = new GamePackageConfiguration(packageName, userId); mOverrideConfigs.put(packageName, overrideConfig); } - - // modify GameModeConfiguration intervention settings - GamePackageConfiguration.GameModeConfiguration overrideModeConfig = - overrideConfig.getGameModeConfiguration(gameMode); - - if (fpsStr != null) { - overrideModeConfig.setFpsStr(fpsStr); - } else { - overrideModeConfig.setFpsStr( - GamePackageConfiguration.GameModeConfiguration.DEFAULT_FPS); - } - if (scaling != null) { - overrideModeConfig.setScaling(scaling); - } else { - overrideModeConfig.setScaling( - GamePackageConfiguration.GameModeConfiguration.DEFAULT_SCALING); - } - Slog.i(TAG, "Package Name: " + packageName - + " FPS: " + String.valueOf(overrideModeConfig.getFps()) - + " Scaling: " + overrideModeConfig.getScaling()); } + // modify GameModeConfiguration intervention settings + GamePackageConfiguration.GameModeConfiguration overrideModeConfig = + overrideConfig.getGameModeConfiguration(gameMode); + + if (fpsStr != null) { + overrideModeConfig.setFpsStr(fpsStr); + } else { + overrideModeConfig.setFpsStr( + GamePackageConfiguration.GameModeConfiguration.DEFAULT_FPS); + } + if (scaling != null) { + overrideModeConfig.setScaling(scaling); + } else { + overrideModeConfig.setScaling( + GamePackageConfiguration.GameModeConfiguration.DEFAULT_SCALING); + } + Slog.i(TAG, "Package Name: " + packageName + + " FPS: " + String.valueOf(overrideModeConfig.getFps()) + + " Scaling: " + overrideModeConfig.getScaling()); setGameMode(packageName, gameMode, userId); } @@ -1498,15 +1490,7 @@ public final class GameManagerService extends IGameManagerService.Stub { // If not, set the game mode to standard int gameMode = getGameMode(packageName, userId); - GamePackageConfiguration config = null; - synchronized (mOverrideConfigLock) { - config = mOverrideConfigs.get(packageName); - } - if (config == null) { - synchronized (mDeviceConfigLock) { - config = mConfigs.get(packageName); - } - } + final GamePackageConfiguration config = getConfig(packageName); final int newGameMode = getNewGameMode(gameMode, config); if (gameMode != newGameMode) { setGameMode(packageName, GameManager.GAME_MODE_STANDARD, userId); @@ -1545,18 +1529,8 @@ public final class GameManagerService extends IGameManagerService.Stub { * Returns the string listing all the interventions currently set to a game. */ public String getInterventionList(String packageName) { - GamePackageConfiguration packageConfig = null; - synchronized (mOverrideConfigLock) { - packageConfig = mOverrideConfigs.get(packageName); - } - - if (packageConfig == null) { - synchronized (mDeviceConfigLock) { - packageConfig = mConfigs.get(packageName); - } - } - - StringBuilder listStrSb = new StringBuilder(); + final GamePackageConfiguration packageConfig = getConfig(packageName); + final StringBuilder listStrSb = new StringBuilder(); if (packageConfig == null) { listStrSb.append("\n No intervention found for package ") .append(packageName);