diff options
6 files changed, 179 insertions, 37 deletions
diff --git a/core/java/android/app/AppCompatCallbacks.java b/core/java/android/app/AppCompatCallbacks.java index f2debfcfa6b1..4bfa3b340ec9 100644 --- a/core/java/android/app/AppCompatCallbacks.java +++ b/core/java/android/app/AppCompatCallbacks.java @@ -82,7 +82,7 @@ public final class AppCompatCallbacks implements Compatibility.BehaviorChangeDel private void reportChange(long changeId, int state, boolean isLoggable) { int uid = Process.myUid(); - mChangeReporter.reportChange(uid, changeId, state, isLoggable); + mChangeReporter.reportChange(uid, changeId, state, false, isLoggable); } } diff --git a/core/java/com/android/internal/compat/ChangeReporter.java b/core/java/com/android/internal/compat/ChangeReporter.java index ded142ce49e5..f61157175f9d 100644 --- a/core/java/com/android/internal/compat/ChangeReporter.java +++ b/core/java/com/android/internal/compat/ChangeReporter.java @@ -42,7 +42,7 @@ import java.util.function.Function; * * @hide */ -public final class ChangeReporter { +public class ChangeReporter { private static final String TAG = "CompatChangeReporter"; private static final Function<Integer, Set<ChangeReport>> NEW_CHANGE_REPORT_SET = uid -> Collections.synchronizedSet(new HashSet<>()); @@ -88,17 +88,20 @@ public final class ChangeReporter { * Report the change to stats log and to the debug log if the change was not previously * logged already. * - * @param uid affected by the change - * @param changeId the reported change id - * @param state of the reported change - enabled/disabled/only logged - * @param isLoggableBySdk whether debug logging is allowed for this change based on target - * SDK version. This is combined with other logic to determine whether to - * actually log. If the sdk version does not matter, should be true. + * @param uid affected by the change + * @param changeId the reported change id + * @param state of the reported change - enabled/disabled/only logged + * @param isKnownSystemApp do we know that the affected app is a system app? (if true is + * definitely a system app, if false it may or may not be a system app) + * @param isLoggableBySdk whether debug logging is allowed for this change based on target + * SDK version. This is combined with other logic to determine whether + * to actually log. If the sdk version does not matter, should be true. */ - public void reportChange(int uid, long changeId, int state, boolean isLoggableBySdk) { + public void reportChange(int uid, long changeId, int state, boolean isKnownSystemApp, + boolean isLoggableBySdk) { boolean isAlreadyReported = checkAndSetIsAlreadyReported(uid, new ChangeReport(changeId, state)); - if (!isAlreadyReported) { + if (shouldWriteToStatsLog(isKnownSystemApp, isAlreadyReported)) { FrameworkStatsLog.write(FrameworkStatsLog.APP_COMPATIBILITY_CHANGE_REPORTED, uid, changeId, state, mSource); } @@ -116,7 +119,7 @@ public final class ChangeReporter { * @param state of the reported change - enabled/disabled/only logged */ public void reportChange(int uid, long changeId, int state) { - reportChange(uid, changeId, state, true); + reportChange(uid, changeId, state, false, true); } /** @@ -136,14 +139,15 @@ public final class ChangeReporter { /** * Returns whether the next report should be logged to FrameworkStatsLog. * - * @param uid affected by the change - * @param changeId the reported change id - * @param state of the reported change - enabled/disabled/only logged + * @param isKnownSystemApp do we know that the affected app is a system app? (if true is + * definitely a system app, if false it may or may not be a system app) + * @param isAlreadyReported is the change already reported * @return true if the report should be logged */ @VisibleForTesting - boolean shouldWriteToStatsLog(int uid, long changeId, int state) { - return !isAlreadyReported(uid, new ChangeReport(changeId, state)); + boolean shouldWriteToStatsLog(boolean isKnownSystemApp, boolean isAlreadyReported) { + // We don't log where we know the source is a system app or is already reported + return !isKnownSystemApp && !isAlreadyReported; } /** @@ -224,6 +228,19 @@ public final class ChangeReporter { return mReportedChanges.getOrDefault(uid, EMPTY_SET).contains(report); } + /** + * Returns whether the next report should be logged. + * + * @param uid affected by the change + * @param changeId the reported change id + * @param state of the reported change - enabled/disabled/only logged + * @return true if the report should be logged + */ + @VisibleForTesting + boolean isAlreadyReported(int uid, long changeId, int state) { + return isAlreadyReported(uid, new ChangeReport(changeId, state)); + } + private void markAsReported(int uid, ChangeReport report) { mReportedChanges.computeIfAbsent(uid, NEW_CHANGE_REPORT_SET).add(report); } diff --git a/core/tests/PlatformCompatFramework/src/com/android/internal/compat/ChangeReporterTest.java b/core/tests/PlatformCompatFramework/src/com/android/internal/compat/ChangeReporterTest.java index 12a42f975cd7..ed5e4aff0279 100644 --- a/core/tests/PlatformCompatFramework/src/com/android/internal/compat/ChangeReporterTest.java +++ b/core/tests/PlatformCompatFramework/src/com/android/internal/compat/ChangeReporterTest.java @@ -37,15 +37,20 @@ public class ChangeReporterTest { long myChangeId = 500L, otherChangeId = 600L; int myState = ChangeReporter.STATE_ENABLED, otherState = ChangeReporter.STATE_DISABLED; - assertTrue(reporter.shouldWriteToStatsLog(myUid, myChangeId, myState)); + assertTrue(reporter.shouldWriteToStatsLog(false, + reporter.isAlreadyReported(myUid, myChangeId, myState))); reporter.reportChange(myUid, myChangeId, myState); // Same report will not be logged again. - assertFalse(reporter.shouldWriteToStatsLog(myUid, myChangeId, myState)); + assertFalse(reporter.shouldWriteToStatsLog(false, + reporter.isAlreadyReported(myUid, myChangeId, myState))); // Other reports will be logged. - assertTrue(reporter.shouldWriteToStatsLog(otherUid, myChangeId, myState)); - assertTrue(reporter.shouldWriteToStatsLog(myUid, otherChangeId, myState)); - assertTrue(reporter.shouldWriteToStatsLog(myUid, myChangeId, otherState)); + assertTrue(reporter.shouldWriteToStatsLog(false, + reporter.isAlreadyReported(otherUid, myChangeId, myState))); + assertTrue(reporter.shouldWriteToStatsLog(false, + reporter.isAlreadyReported(myUid, otherChangeId, myState))); + assertTrue(reporter.shouldWriteToStatsLog(false, + reporter.isAlreadyReported(myUid, myChangeId, otherState))); } @Test @@ -55,15 +60,18 @@ public class ChangeReporterTest { long myChangeId = 500L; int myState = ChangeReporter.STATE_ENABLED; - assertTrue(reporter.shouldWriteToStatsLog(myUid, myChangeId, myState)); + assertTrue(reporter.shouldWriteToStatsLog(false, + reporter.isAlreadyReported(myUid, myChangeId, myState))); reporter.reportChange(myUid, myChangeId, myState); // Same report will not be logged again. - assertFalse(reporter.shouldWriteToStatsLog(myUid, myChangeId, myState)); + assertFalse(reporter.shouldWriteToStatsLog(false, + reporter.isAlreadyReported(myUid, myChangeId, myState))); reporter.resetReportedChanges(myUid); // Same report will be logged again after reset. - assertTrue(reporter.shouldWriteToStatsLog(myUid, myChangeId, myState)); + assertTrue(reporter.shouldWriteToStatsLog(false, + reporter.isAlreadyReported(myUid, myChangeId, myState))); } @Test @@ -196,4 +204,21 @@ public class ChangeReporterTest { // off. assertTrue(reporter.shouldWriteToDebug(myUid, myChangeId, myDisabledState, false)); } + + @Test + public void testDontLogSystemApps() { + // Verify we don't log an app if we know it's a system app when source is system server. + ChangeReporter systemServerReporter = + new ChangeReporter(ChangeReporter.SOURCE_SYSTEM_SERVER); + + assertFalse(systemServerReporter.shouldWriteToStatsLog(true, false)); + assertFalse(systemServerReporter.shouldWriteToStatsLog(true, true)); + + // Verify we don't log an app if we know it's a system app when source is unknown. + ChangeReporter unknownReporter = + new ChangeReporter(ChangeReporter.SOURCE_UNKNOWN_SOURCE); + + assertFalse(unknownReporter.shouldWriteToStatsLog(true, false)); + assertFalse(unknownReporter.shouldWriteToStatsLog(true, true)); + } } diff --git a/services/core/java/com/android/server/compat/PlatformCompat.java b/services/core/java/com/android/server/compat/PlatformCompat.java index f8fd0a0790e0..22b85d45d15e 100644 --- a/services/core/java/com/android/server/compat/PlatformCompat.java +++ b/services/core/java/com/android/server/compat/PlatformCompat.java @@ -83,9 +83,10 @@ public class PlatformCompat extends IPlatformCompat.Stub { @VisibleForTesting PlatformCompat(Context context, CompatConfig compatConfig, - AndroidBuildClassifier buildClassifier) { + AndroidBuildClassifier buildClassifier, + ChangeReporter changeReporter) { mContext = context; - mChangeReporter = new ChangeReporter(ChangeReporter.SOURCE_SYSTEM_SERVER); + mChangeReporter = changeReporter; mCompatConfig = compatConfig; mBuildClassifier = buildClassifier; @@ -96,8 +97,11 @@ public class PlatformCompat extends IPlatformCompat.Stub { @EnforcePermission(LOG_COMPAT_CHANGE) public void reportChange(long changeId, ApplicationInfo appInfo) { super.reportChange_enforcePermission(); - - reportChangeInternal(changeId, appInfo.uid, ChangeReporter.STATE_LOGGED); + reportChangeInternal( + changeId, + appInfo.uid, + appInfo.isSystemApp(), + ChangeReporter.STATE_LOGGED); } @Override @@ -108,7 +112,11 @@ public class PlatformCompat extends IPlatformCompat.Stub { ApplicationInfo appInfo = getApplicationInfo(packageName, userId); if (appInfo != null) { - reportChangeInternal(changeId, appInfo.uid, ChangeReporter.STATE_LOGGED); + reportChangeInternal( + changeId, + appInfo.uid, + appInfo.isSystemApp(), + ChangeReporter.STATE_LOGGED); } } @@ -117,7 +125,7 @@ public class PlatformCompat extends IPlatformCompat.Stub { public void reportChangeByUid(long changeId, int uid) { super.reportChangeByUid_enforcePermission(); - reportChangeInternal(changeId, uid, ChangeReporter.STATE_LOGGED); + reportChangeInternal(changeId, uid, false, ChangeReporter.STATE_LOGGED); } /** @@ -128,8 +136,8 @@ public class PlatformCompat extends IPlatformCompat.Stub { * @param uid of the user * @param state of the change - enabled/disabled/logged */ - private void reportChangeInternal(long changeId, int uid, int state) { - mChangeReporter.reportChange(uid, changeId, state, true); + private void reportChangeInternal(long changeId, int uid, boolean isKnownSystemApp, int state) { + mChangeReporter.reportChange(uid, changeId, state, isKnownSystemApp, true); } @Override @@ -190,7 +198,11 @@ public class PlatformCompat extends IPlatformCompat.Stub { if (appInfo != null) { boolean isTargetingLatestSdk = mCompatConfig.isChangeTargetingLatestSdk(c, appInfo.targetSdkVersion); - mChangeReporter.reportChange(appInfo.uid, changeId, state, isTargetingLatestSdk); + mChangeReporter.reportChange(appInfo.uid, + changeId, + state, + appInfo.isSystemApp(), + isTargetingLatestSdk); } return enabled; } diff --git a/services/tests/servicestests/src/com/android/server/compat/ApplicationInfoBuilder.java b/services/tests/servicestests/src/com/android/server/compat/ApplicationInfoBuilder.java index c165c661a625..d8fd266ab37f 100644 --- a/services/tests/servicestests/src/com/android/server/compat/ApplicationInfoBuilder.java +++ b/services/tests/servicestests/src/com/android/server/compat/ApplicationInfoBuilder.java @@ -21,8 +21,10 @@ import android.content.pm.ApplicationInfo; class ApplicationInfoBuilder { private boolean mIsDebuggable; private int mTargetSdk; + private int mUid; private String mPackageName; private long mVersionCode; + private boolean mIsSystemApp; private ApplicationInfoBuilder() { mTargetSdk = -1; @@ -42,6 +44,16 @@ class ApplicationInfoBuilder { return this; } + ApplicationInfoBuilder systemApp() { + mIsSystemApp = true; + return this; + } + + ApplicationInfoBuilder withUid(int uid) { + mUid = uid; + return this; + } + ApplicationInfoBuilder withPackageName(String packageName) { mPackageName = packageName; return this; @@ -60,6 +72,10 @@ class ApplicationInfoBuilder { applicationInfo.packageName = mPackageName; applicationInfo.targetSdkVersion = mTargetSdk; applicationInfo.longVersionCode = mVersionCode; + applicationInfo.uid = mUid; + if (mIsSystemApp) { + applicationInfo.flags |= ApplicationInfo.FLAG_SYSTEM; + } return applicationInfo; } } diff --git a/services/tests/servicestests/src/com/android/server/compat/PlatformCompatTest.java b/services/tests/servicestests/src/com/android/server/compat/PlatformCompatTest.java index 9accd49bc392..9df7a3612a92 100644 --- a/services/tests/servicestests/src/com/android/server/compat/PlatformCompatTest.java +++ b/services/tests/servicestests/src/com/android/server/compat/PlatformCompatTest.java @@ -29,6 +29,7 @@ import static org.testng.Assert.assertThrows; import android.compat.Compatibility.ChangeConfig; import android.content.Context; +import android.content.pm.ApplicationInfo; import android.content.pm.PackageManager; import android.content.pm.PackageManagerInternal; import android.os.Build; @@ -36,6 +37,7 @@ import android.os.Build; import androidx.test.runner.AndroidJUnit4; import com.android.internal.compat.AndroidBuildClassifier; +import com.android.internal.compat.ChangeReporter; import com.android.internal.compat.CompatibilityChangeConfig; import com.android.internal.compat.CompatibilityChangeInfo; import com.android.server.LocalServices; @@ -65,6 +67,8 @@ public class PlatformCompatTest { CompatConfig mCompatConfig; @Mock private AndroidBuildClassifier mBuildClassifier; + @Mock + private ChangeReporter mChangeReporter; @Before public void setUp() throws Exception { @@ -78,7 +82,8 @@ public class PlatformCompatTest { when(mPackageManager.getApplicationInfo(eq(PACKAGE_NAME), anyInt())) .thenThrow(new PackageManager.NameNotFoundException()); mCompatConfig = new CompatConfig(mBuildClassifier, mContext); - mPlatformCompat = new PlatformCompat(mContext, mCompatConfig, mBuildClassifier); + mPlatformCompat = + new PlatformCompat(mContext, mCompatConfig, mBuildClassifier, mChangeReporter); // Assume userdebug/eng non-final build mCompatConfig.forceNonDebuggableFinalForTest(false); when(mBuildClassifier.isDebuggableBuild()).thenReturn(true); @@ -100,7 +105,8 @@ public class PlatformCompatTest { .addLoggingOnlyChangeWithId(7L) .addDisabledOverridableChangeWithId(8L) .build(); - mPlatformCompat = new PlatformCompat(mContext, mCompatConfig, mBuildClassifier); + mPlatformCompat = + new PlatformCompat(mContext, mCompatConfig, mBuildClassifier, mChangeReporter); assertThat(mPlatformCompat.listAllChanges()).asList().containsExactly( new CompatibilityChangeInfo(1L, "", -1, -1, false, false, "", false), new CompatibilityChangeInfo(2L, "change2", -1, -1, true, false, "", false), @@ -128,7 +134,8 @@ public class PlatformCompatTest { .addLoggingOnlyChangeWithId(7L) .addEnableSinceSdkChangeWithId(31, 8L) .build(); - mPlatformCompat = new PlatformCompat(mContext, mCompatConfig, mBuildClassifier); + mPlatformCompat = + new PlatformCompat(mContext, mCompatConfig, mBuildClassifier, mChangeReporter); assertThat(mPlatformCompat.listUIChanges()).asList().containsExactly( new CompatibilityChangeInfo(1L, "", -1, -1, false, false, "", false), new CompatibilityChangeInfo(2L, "change2", -1, -1, true, false, "", false), @@ -146,7 +153,8 @@ public class PlatformCompatTest { .addEnableAfterSdkChangeWithId(Build.VERSION_CODES.O, 3L) .build(); mCompatConfig.forceNonDebuggableFinalForTest(true); - mPlatformCompat = new PlatformCompat(mContext, mCompatConfig, mBuildClassifier); + mPlatformCompat = + new PlatformCompat(mContext, mCompatConfig, mBuildClassifier, mChangeReporter); // Before adding overrides. assertThat(mPlatformCompat.isChangeEnabledByPackageName(1, PACKAGE_NAME, 0)).isTrue(); @@ -369,4 +377,68 @@ public class PlatformCompatTest { // Listener not called when a non existing override is removed. verify(mListener1, never()).onCompatChange(PACKAGE_NAME); } + + @Test + public void testReportChange() throws Exception { + ApplicationInfo appInfo = ApplicationInfoBuilder.create().withUid(123).build(); + mPlatformCompat.reportChange(1L, appInfo); + verify(mChangeReporter).reportChange(123, 1L, ChangeReporter.STATE_LOGGED, false, true); + + ApplicationInfo systemAppInfo = + ApplicationInfoBuilder.create().withUid(123).systemApp().build(); + mPlatformCompat.reportChange(1L, systemAppInfo); + verify(mChangeReporter).reportChange(123, 1L, ChangeReporter.STATE_LOGGED, true, true); + } + + @Test + public void testReportChangeByPackageName() throws Exception { + when(mPackageManagerInternal.getApplicationInfo( + eq(PACKAGE_NAME), eq(0L), anyInt(), anyInt())) + .thenReturn( + ApplicationInfoBuilder.create() + .withPackageName(PACKAGE_NAME) + .withUid(123) + .build()); + + mPlatformCompat.reportChangeByPackageName(1L, PACKAGE_NAME, 123); + verify(mChangeReporter).reportChange(123, 1L, ChangeReporter.STATE_LOGGED, false, true); + + String SYSTEM_PACKAGE_NAME = "my.system.package"; + + when(mPackageManagerInternal.getApplicationInfo( + eq(SYSTEM_PACKAGE_NAME), eq(0L), anyInt(), anyInt())) + .thenReturn( + ApplicationInfoBuilder.create() + .withPackageName(SYSTEM_PACKAGE_NAME) + .withUid(123) + .systemApp() + .build()); + + mPlatformCompat.reportChangeByPackageName(1L, SYSTEM_PACKAGE_NAME, 123); + verify(mChangeReporter).reportChange(123, 1L, ChangeReporter.STATE_LOGGED, true, true); + } + + @Test + public void testIsChangeEnabled() throws Exception { + mCompatConfig = + CompatConfigBuilder.create(mBuildClassifier, mContext) + .addEnabledChangeWithId(1L) + .addDisabledChangeWithId(2L) + .addEnabledChangeWithId(3L) + .build(); + mCompatConfig.forceNonDebuggableFinalForTest(true); + mPlatformCompat = + new PlatformCompat(mContext, mCompatConfig, mBuildClassifier, mChangeReporter); + + ApplicationInfo appInfo = ApplicationInfoBuilder.create().withUid(123).build(); + assertThat(mPlatformCompat.isChangeEnabled(1L, appInfo)).isTrue(); + verify(mChangeReporter).reportChange(123, 1L, ChangeReporter.STATE_ENABLED, false, false); + assertThat(mPlatformCompat.isChangeEnabled(2L, appInfo)).isFalse(); + verify(mChangeReporter).reportChange(123, 2L, ChangeReporter.STATE_DISABLED, false, false); + + ApplicationInfo systemAppInfo = + ApplicationInfoBuilder.create().withUid(123).systemApp().build(); + assertThat(mPlatformCompat.isChangeEnabled(3L, systemAppInfo)).isTrue(); + verify(mChangeReporter).reportChange(123, 3L, ChangeReporter.STATE_ENABLED, true, false); + } } |