Merge "Fix WMS instance leakage of WmTests" am: 49c0c87c79

Original change: https://android-review.googlesource.com/c/platform/frameworks/base/+/2036104

Change-Id: Ie82ccf06032f149aec3f2163e20b1f47bb3a15e8

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;
+        deviceConfig.addOnPropertiesChangedListener(DeviceConfig.NAMESPACE_DISPLAY_MANAGER,
-        mDeviceConfig.addOnPropertiesChangedListener(DeviceConfig.NAMESPACE_DISPLAY_MANAGER,
+                BackgroundThread.getExecutor(), new OnPropertiesChangedListener());
-                BackgroundThread.getExecutor(), mListener);
+        final String property = deviceConfig.getProperty(DeviceConfig.NAMESPACE_DISPLAY_MANAGER,
-        final String property = mDeviceConfig.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)) {

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;

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);

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<DeviceConfig.OnPropertiesChangedListener> 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(LocalServices.class, spyStubOnly)
-                .mockStatic(LockGuard.class)
+                .mockStatic(DeviceConfig.class, spyStubOnly)
-                .mockStatic(Watchdog.class)
+                .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
+        for (int i = mDeviceConfigListeners.size() - 1; i >= 0; i--) {
-        // leaking mWmService.
+            DeviceConfig.removeOnPropertiesChangedListener(mDeviceConfigListeners.get(i));
-        mWmService.mConstants.dispose();
+        }
-        mWmService.mHighRefreshRateDenylist.dispose();
 
         // This makes sure the posted messages without delay are processed, e.g.
         // DisplayPolicy#release, WindowManagerService#setAnimationScale.

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 {
+    public void testRule_rethrows_throwable() {
-        final SystemServicesTestRule mWmsRule = new SystemServicesTestRule();
+        assertThrows(Throwable.class, () -> applyRule(rule -> false));
-        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();
-    }
-
-    @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!");
-            }
-        };
-        mExpectedException.expect(IOException.class);
-        mWmsRule.apply(statement, null /* Description*/).evaluate();
     }
 
     @Test
     public void testRule_ranSuccessfully() throws Throwable {
-        final boolean[] testRan = {false};
+        final int iterations = 5;
-        final SystemServicesTestRule mWmsRule = new SystemServicesTestRule();
+        final ArrayList<WeakReference<WindowManagerService>> wmsRefs = new ArrayList<>();
-        Statement statement = new Statement() {
+        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;
+            }
+        }
+        fail("WMS instance is leaked");
+    }
+
+    private static void applyRule(Predicate<SystemServicesTestRule> 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!");
+                }
             }
-        };
+        }, null /* description */).evaluate();
-        mWmsRule.apply(statement, null /* Description*/).evaluate();
-        assertTrue(testRan[0]);
     }
 }