From cfab8e00310a3810933b0f73cbe7bab31daf5f25 Mon Sep 17 00:00:00 2001 From: Riddle Hsu Date: Mon, 7 Mar 2022 14:22:38 +0800 Subject: Fix WMS instance leakage of WmTests It may be common to register a non-static inner class object to DeviceConfig.addOnPropertiesChangedListener: WindowManagerConstants HighRefreshRateDenylist SplashScreenExceptionList WindowOrientationListener.OrientationSensorJudge That may hold reference chain to WindowManagerService. It may be annoying to add test-only code to dispose them once a new path is added, so hook the registration to make sure that all listeners can be released after a test method is finished. For test using FakeDeviceConfig, it only registers to the local fake instance, so it doesn't need to dispose. Bug: 219640050 Test: atest SystemServicesTestRuleTest Change-Id: Ic06d85978886caaea3a9eb123a7a59712b58bb92 Merged-In: Ic06d85978886caaea3a9eb123a7a59712b58bb92 --- .../android/server/wm/HighRefreshRateDenylist.java | 18 +----- .../android/server/wm/WindowManagerConstants.java | 6 -- .../server/wm/HighRefreshRateDenylistTest.java | 6 -- .../android/server/wm/SystemServicesTestRule.java | 52 ++++++++++++--- .../server/wm/SystemServicesTestRuleTest.java | 73 ++++++++++++---------- 5 files changed, 84 insertions(+), 71 deletions(-) diff --git a/services/core/java/com/android/server/wm/HighRefreshRateDenylist.java b/services/core/java/com/android/server/wm/HighRefreshRateDenylist.java index 92baadf5ee69..7a6e86ce7919 100644 --- a/services/core/java/com/android/server/wm/HighRefreshRateDenylist.java +++ b/services/core/java/com/android/server/wm/HighRefreshRateDenylist.java @@ -41,9 +41,6 @@ class HighRefreshRateDenylist { private final String[] mDefaultDenylist; private final Object mLock = new Object(); - private DeviceConfigInterface mDeviceConfig; - private OnPropertiesChangedListener mListener = new OnPropertiesChangedListener(); - static HighRefreshRateDenylist create(@NonNull Resources r) { return new HighRefreshRateDenylist(r, DeviceConfigInterface.REAL); } @@ -51,10 +48,9 @@ class HighRefreshRateDenylist { @VisibleForTesting HighRefreshRateDenylist(Resources r, DeviceConfigInterface deviceConfig) { mDefaultDenylist = r.getStringArray(R.array.config_highRefreshRateBlacklist); - mDeviceConfig = deviceConfig; - mDeviceConfig.addOnPropertiesChangedListener(DeviceConfig.NAMESPACE_DISPLAY_MANAGER, - BackgroundThread.getExecutor(), mListener); - final String property = mDeviceConfig.getProperty(DeviceConfig.NAMESPACE_DISPLAY_MANAGER, + deviceConfig.addOnPropertiesChangedListener(DeviceConfig.NAMESPACE_DISPLAY_MANAGER, + BackgroundThread.getExecutor(), new OnPropertiesChangedListener()); + final String property = deviceConfig.getProperty(DeviceConfig.NAMESPACE_DISPLAY_MANAGER, KEY_HIGH_REFRESH_RATE_BLACKLIST); updateDenylist(property); } @@ -95,14 +91,6 @@ class HighRefreshRateDenylist { } } - /** Used to prevent WmTests leaking issues. */ - @VisibleForTesting - void dispose() { - mDeviceConfig.removeOnPropertiesChangedListener(mListener); - mDeviceConfig = null; - mDenylistedPackages.clear(); - } - private class OnPropertiesChangedListener implements DeviceConfig.OnPropertiesChangedListener { public void onPropertiesChanged(@NonNull DeviceConfig.Properties properties) { if (properties.getKeyset().contains(KEY_HIGH_REFRESH_RATE_BLACKLIST)) { diff --git a/services/core/java/com/android/server/wm/WindowManagerConstants.java b/services/core/java/com/android/server/wm/WindowManagerConstants.java index a5ebf9ac74b9..ba62091c694d 100644 --- a/services/core/java/com/android/server/wm/WindowManagerConstants.java +++ b/services/core/java/com/android/server/wm/WindowManagerConstants.java @@ -91,12 +91,6 @@ final class WindowManagerConstants { updateSystemGestureExcludedByPreQStickyImmersive(); } - @VisibleForTesting - void dispose() { - mDeviceConfig.removeOnPropertiesChangedListener(mListenerAndroid); - mDeviceConfig.removeOnPropertiesChangedListener(mListenerWindowManager); - } - private void onAndroidPropertiesChanged(DeviceConfig.Properties properties) { synchronized (mGlobalLock) { boolean updateSystemGestureExclusionLimit = false; diff --git a/services/tests/wmtests/src/com/android/server/wm/HighRefreshRateDenylistTest.java b/services/tests/wmtests/src/com/android/server/wm/HighRefreshRateDenylistTest.java index dfc2e35ffedf..38ab683f81fe 100644 --- a/services/tests/wmtests/src/com/android/server/wm/HighRefreshRateDenylistTest.java +++ b/services/tests/wmtests/src/com/android/server/wm/HighRefreshRateDenylistTest.java @@ -33,7 +33,6 @@ import com.android.internal.R; import com.android.internal.util.Preconditions; import com.android.server.testutils.FakeDeviceConfigInterface; -import org.junit.After; import org.junit.Test; import java.util.concurrent.Executor; @@ -51,11 +50,6 @@ public class HighRefreshRateDenylistTest { private HighRefreshRateDenylist mDenylist; - @After - public void tearDown() { - mDenylist.dispose(); - } - @Test public void testDefaultDenylist() { final Resources r = createResources(APP1, APP2); diff --git a/services/tests/wmtests/src/com/android/server/wm/SystemServicesTestRule.java b/services/tests/wmtests/src/com/android/server/wm/SystemServicesTestRule.java index f41ca65c9439..db937c100b62 100644 --- a/services/tests/wmtests/src/com/android/server/wm/SystemServicesTestRule.java +++ b/services/tests/wmtests/src/com/android/server/wm/SystemServicesTestRule.java @@ -18,6 +18,7 @@ package com.android.server.wm; import static android.app.WindowConfiguration.ACTIVITY_TYPE_HOME; import static android.app.WindowConfiguration.WINDOWING_MODE_FULLSCREEN; +import static android.provider.DeviceConfig.NAMESPACE_CONSTRAIN_DISPLAY_APIS; import static android.testing.DexmakerShareClassLoaderRule.runWithDexmakerShareClassLoader; import static android.view.Display.DEFAULT_DISPLAY; @@ -27,6 +28,7 @@ import static com.android.dx.mockito.inline.extended.ExtendedMockito.any; import static com.android.dx.mockito.inline.extended.ExtendedMockito.anyBoolean; import static com.android.dx.mockito.inline.extended.ExtendedMockito.anyInt; import static com.android.dx.mockito.inline.extended.ExtendedMockito.anyString; +import static com.android.dx.mockito.inline.extended.ExtendedMockito.doAnswer; import static com.android.dx.mockito.inline.extended.ExtendedMockito.doNothing; import static com.android.dx.mockito.inline.extended.ExtendedMockito.doReturn; import static com.android.dx.mockito.inline.extended.ExtendedMockito.eq; @@ -36,6 +38,9 @@ import static com.android.dx.mockito.inline.extended.ExtendedMockito.nullable; import static com.android.dx.mockito.inline.extended.ExtendedMockito.spy; import static com.android.dx.mockito.inline.extended.ExtendedMockito.spyOn; +import static org.mockito.Mockito.CALLS_REAL_METHODS; +import static org.mockito.Mockito.withSettings; + import android.app.ActivityManagerInternal; import android.app.AppOpsManager; import android.app.usage.UsageStatsManagerInternal; @@ -57,6 +62,7 @@ import android.os.PowerManagerInternal; import android.os.PowerSaveState; import android.os.StrictMode; import android.os.UserHandle; +import android.provider.DeviceConfig; import android.util.Log; import android.view.InputChannel; import android.view.Surface; @@ -83,10 +89,12 @@ import com.android.server.uri.UriGrantsManagerInternal; import org.junit.rules.TestRule; import org.junit.runner.Description; import org.junit.runners.model.Statement; +import org.mockito.MockSettings; import org.mockito.Mockito; import org.mockito.quality.Strictness; import java.io.File; +import java.util.ArrayList; import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Supplier; @@ -107,6 +115,13 @@ public class SystemServicesTestRule implements TestRule { .getSystemService(PowerManager.class).newWakeLock(PowerManager.PARTIAL_WAKE_LOCK, TAG); private PowerManager.WakeLock mStubbedWakeLock; + /** + * The captured listeners will be unregistered in {@link #tearDown()} to avoid keeping static + * references of test instances from DeviceConfig. + */ + private final ArrayList mDeviceConfigListeners = + new ArrayList<>(); + private Description mDescription; private Context mContext; private StaticMockitoSession mMockitoSession; @@ -144,19 +159,27 @@ public class SystemServicesTestRule implements TestRule { Log.e("SystemServicesTestRule", "Suppressed: ", throwable); t.addSuppressed(throwable); } - throw t; + throwable = t; } - if (throwable != null) throw throwable; } + if (throwable != null) throw throwable; } }; } private void setUp() { + // Use stubOnly() to reduce memory usage if it doesn't need verification. + final MockSettings spyStubOnly = withSettings().stubOnly() + .defaultAnswer(CALLS_REAL_METHODS); + final MockSettings mockStubOnly = withSettings().stubOnly(); + // Return mocked services: LocalServices.getService + // Avoid leakage: DeviceConfig.addOnPropertiesChangedListener, LockGuard.installLock + // Watchdog.getInstance/addMonitor mMockitoSession = mockitoSession() - .spyStatic(LocalServices.class) - .mockStatic(LockGuard.class) - .mockStatic(Watchdog.class) + .mockStatic(LocalServices.class, spyStubOnly) + .mockStatic(DeviceConfig.class, spyStubOnly) + .mockStatic(LockGuard.class, mockStubOnly) + .mockStatic(Watchdog.class, mockStubOnly) .strictness(Strictness.LENIENT) .startMocking(); @@ -168,6 +191,16 @@ public class SystemServicesTestRule implements TestRule { private void setUpSystemCore() { doReturn(mock(Watchdog.class)).when(Watchdog::getInstance); + doAnswer(invocation -> { + // Exclude CONSTRAIN_DISPLAY_APIS because ActivityRecord#sConstrainDisplayApisConfig + // only registers once and it doesn't reference to outside. + if (!NAMESPACE_CONSTRAIN_DISPLAY_APIS.equals(invocation.getArgument(0))) { + mDeviceConfigListeners.add(invocation.getArgument(2)); + } + // SizeCompatTests uses setNeverConstrainDisplayApisFlag, and ActivityRecordTests + // uses splash_screen_exception_list. So still execute real registration. + return invocation.callRealMethod(); + }).when(() -> DeviceConfig.addOnPropertiesChangedListener(anyString(), any(), any())); mContext = getInstrumentation().getTargetContext(); spyOn(mContext); @@ -346,11 +379,10 @@ public class SystemServicesTestRule implements TestRule { // Unregister display listener from root to avoid issues with subsequent tests. mContext.getSystemService(DisplayManager.class) .unregisterDisplayListener(mAtmService.mRootWindowContainer); - // The constructor of WindowManagerService registers WindowManagerConstants and - // HighRefreshRateBlacklist with DeviceConfig. We need to undo that here to avoid - // leaking mWmService. - mWmService.mConstants.dispose(); - mWmService.mHighRefreshRateDenylist.dispose(); + + for (int i = mDeviceConfigListeners.size() - 1; i >= 0; i--) { + DeviceConfig.removeOnPropertiesChangedListener(mDeviceConfigListeners.get(i)); + } // This makes sure the posted messages without delay are processed, e.g. // DisplayPolicy#release, WindowManagerService#setAnimationScale. diff --git a/services/tests/wmtests/src/com/android/server/wm/SystemServicesTestRuleTest.java b/services/tests/wmtests/src/com/android/server/wm/SystemServicesTestRuleTest.java index 4056c7195c9b..2e7cc2736410 100644 --- a/services/tests/wmtests/src/com/android/server/wm/SystemServicesTestRuleTest.java +++ b/services/tests/wmtests/src/com/android/server/wm/SystemServicesTestRuleTest.java @@ -16,59 +16,64 @@ package com.android.server.wm; -import static junit.framework.Assert.assertTrue; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.fail; import android.platform.test.annotations.Presubmit; -import org.junit.Rule; +import com.android.internal.util.GcUtils; + import org.junit.Test; -import org.junit.rules.ExpectedException; import org.junit.runners.model.Statement; -import java.io.IOException; +import java.lang.ref.WeakReference; +import java.util.ArrayList; +import java.util.function.Predicate; @Presubmit public class SystemServicesTestRuleTest { - @Rule - public ExpectedException mExpectedException = ExpectedException.none(); @Test - public void testRule_rethrows_unchecked_exceptions() throws Throwable { - final SystemServicesTestRule mWmsRule = new SystemServicesTestRule(); - Statement statement = new Statement() { - @Override - public void evaluate() throws Throwable { - throw new RuntimeException("A failing test!"); - } - }; - mExpectedException.expect(RuntimeException.class); - mWmsRule.apply(statement, null /* Description*/).evaluate(); + public void testRule_rethrows_throwable() { + assertThrows(Throwable.class, () -> applyRule(rule -> false)); } @Test - public void testRule_rethrows_checked_exceptions() throws Throwable { - final SystemServicesTestRule mWmsRule = new SystemServicesTestRule(); - Statement statement = new Statement() { - @Override - public void evaluate() throws Throwable { - throw new IOException("A failing test!"); + public void testRule_ranSuccessfully() throws Throwable { + final int iterations = 5; + final ArrayList> wmsRefs = new ArrayList<>(); + for (int i = 0; i < iterations; i++) { + applyRule(rule -> { + final WindowManagerService wms = rule.getWindowManagerService(); + assertNotNull(wms); + wmsRefs.add(new WeakReference<>(wms)); + return true; + }); + } + assertEquals(iterations, wmsRefs.size()); + + GcUtils.runGcAndFinalizersSync(); + // Only ensure that at least one instance is released because some references may be kept + // temporally by the message of other thread or single static reference. + for (int i = wmsRefs.size() - 1; i >= 0; i--) { + if (wmsRefs.get(i).get() == null) { + return; } - }; - mExpectedException.expect(IOException.class); - mWmsRule.apply(statement, null /* Description*/).evaluate(); + } + fail("WMS instance is leaked"); } - @Test - public void testRule_ranSuccessfully() throws Throwable { - final boolean[] testRan = {false}; - final SystemServicesTestRule mWmsRule = new SystemServicesTestRule(); - Statement statement = new Statement() { + private static void applyRule(Predicate action) throws Throwable { + final SystemServicesTestRule wmsRule = new SystemServicesTestRule(); + wmsRule.apply(new Statement() { @Override public void evaluate() throws Throwable { - testRan[0] = true; + if (!action.test(wmsRule)) { + throw new Throwable("A failing test!"); + } } - }; - mWmsRule.apply(statement, null /* Description*/).evaluate(); - assertTrue(testRan[0]); + }, null /* description */).evaluate(); } } -- cgit v1.2.3-59-g8ed1b