From b3e8ae823098886206c37f4677877235e150b48a Mon Sep 17 00:00:00 2001 From: vadimt Date: Fri, 17 Apr 2020 15:50:52 -0700 Subject: [PATCH] Makeshift analog of Strictmode leak detector MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Strictmode leak detector is still a goal, but we might not be able to achieve it in R. Strictmode has several framework-side bugs that perhaps hide Launcher-side strictmode violations, while the time to fix everything is limited, and new leaks get introduced all the time. For now, implementing a check that is slightly more relaxed than Strictmode, but still ensures the absence of leaks. I’ll keep eliminating Strictmode violations as well as keep strengthening the makeshift checker conditions until we’ll be able to enable Strictmode in continuous testing. I’m disabling Strictmode checks for now so that they don’t generate unnecessary hprof dumps, but leaving the code dealing with strictmode. Bug: 139137636 Change-Id: Ib10136b0d4e9892f70a19cd052ae5a54cf0a4efb --- .../quickstep/NavigationModeSwitchRule.java | 2 +- .../launcher3/ui/AbstractLauncherUiTest.java | 56 +++++++++---- .../launcher3/ui/ActivityLeakTracker.java | 83 +++++++++++++++++++ .../launcher3/ui/PortraitLandscapeRunner.java | 4 +- .../launcher3/ui/TaplTestsLauncher3.java | 2 +- 5 files changed, 126 insertions(+), 21 deletions(-) create mode 100644 tests/src/com/android/launcher3/ui/ActivityLeakTracker.java diff --git a/quickstep/tests/src/com/android/quickstep/NavigationModeSwitchRule.java b/quickstep/tests/src/com/android/quickstep/NavigationModeSwitchRule.java index 0afe4a8c88..40265c47e2 100644 --- a/quickstep/tests/src/com/android/quickstep/NavigationModeSwitchRule.java +++ b/quickstep/tests/src/com/android/quickstep/NavigationModeSwitchRule.java @@ -203,7 +203,7 @@ public class NavigationModeSwitchRule implements TestRule { + launcher.getNavigationModeMismatchError(), () -> launcher.getNavigationModeMismatchError() == null, 60000 /* b/148422894 */, launcher); - AbstractLauncherUiTest.checkDetectedLeaks(); + AbstractLauncherUiTest.checkDetectedLeaks(launcher); return true; } diff --git a/tests/src/com/android/launcher3/ui/AbstractLauncherUiTest.java b/tests/src/com/android/launcher3/ui/AbstractLauncherUiTest.java index 86faddb41d..b8c650c671 100644 --- a/tests/src/com/android/launcher3/ui/AbstractLauncherUiTest.java +++ b/tests/src/com/android/launcher3/ui/AbstractLauncherUiTest.java @@ -100,9 +100,10 @@ public abstract class AbstractLauncherUiTest { public static final long DEFAULT_UI_TIMEOUT = 10000; private static final String TAG = "AbstractLauncherUiTest"; - private static String sDetectedActivityLeak; + private static String sStrictmodeDetectedActivityLeak; private static boolean sActivityLeakReported; private static final String SYSTEMUI_PACKAGE = "com.android.systemui"; + private static final ActivityLeakTracker ACTIVITY_LEAK_TRACKER = new ActivityLeakTracker(); protected LooperExecutor mMainThreadExecutor = MAIN_EXECUTOR; protected final UiDevice mDevice = UiDevice.getInstance(getInstrumentation()); @@ -115,30 +116,51 @@ public abstract class AbstractLauncherUiTest { if (TestHelpers.isInLauncherProcess()) { StrictMode.VmPolicy.Builder builder = new StrictMode.VmPolicy.Builder() - .detectActivityLeaks() +// b/154772063 +// .detectActivityLeaks() .penaltyLog() .penaltyListener(Runnable::run, violation -> { - // Runs in the main thread. We can't dumpheap in the main thread, - // so let's just mark the fact that the leak has happened. - if (sDetectedActivityLeak == null) { - sDetectedActivityLeak = violation.toString(); - try { - Debug.dumpHprofData( - getInstrumentation().getTargetContext() - .getFilesDir().getPath() - + "/ActivityLeakHeapDump.hprof"); - } catch (Throwable e) { - Log.e(TAG, "dumpHprofData failed", e); - } + if (sStrictmodeDetectedActivityLeak == null) { + sStrictmodeDetectedActivityLeak = violation.toString() + ", " + + dumpHprofData() + "."; } }); StrictMode.setVmPolicy(builder.build()); } } - public static void checkDetectedLeaks() { - if (sDetectedActivityLeak != null && !sActivityLeakReported) { + public static void checkDetectedLeaks(LauncherInstrumentation launcher) { + if (sActivityLeakReported) return; + + if (sStrictmodeDetectedActivityLeak != null) { + // Report from the test thread strictmode violations detected in the main thread. sActivityLeakReported = true; + Assert.fail(sStrictmodeDetectedActivityLeak); + } + + // Check whether activity leak detector has found leaked activities. + Wait.atMost(AbstractLauncherUiTest::getActivityLeakErrorMessage, + () -> { + launcher.getTotalPssKb(); // Triggers GC + return MAIN_EXECUTOR.submit( + () -> ACTIVITY_LEAK_TRACKER.noLeakedActivities()).get(); + }, DEFAULT_UI_TIMEOUT, launcher); + } + + private static String getActivityLeakErrorMessage() { + sActivityLeakReported = true; + return "Activity leak detector has found leaked activities, " + dumpHprofData() + "."; + } + + private static String dumpHprofData() { + try { + final String fileName = getInstrumentation().getTargetContext().getFilesDir().getPath() + + "/ActivityLeakHeapDump.hprof"; + Debug.dumpHprofData(fileName); + return "memory dump filename: " + fileName; + } catch (Throwable e) { + Log.e(TAG, "dumpHprofData failed", e); + return "failed to save memory dump"; } } @@ -265,7 +287,7 @@ public abstract class AbstractLauncherUiTest { if (mLauncherPid != 0) { assertEquals("Launcher crashed, pid mismatch:", mLauncherPid, mLauncher.getPid()); } - checkDetectedLeaks(); + checkDetectedLeaks(mLauncher); } protected void clearLauncherData() throws IOException, InterruptedException { diff --git a/tests/src/com/android/launcher3/ui/ActivityLeakTracker.java b/tests/src/com/android/launcher3/ui/ActivityLeakTracker.java new file mode 100644 index 0000000000..0b5ce02c5c --- /dev/null +++ b/tests/src/com/android/launcher3/ui/ActivityLeakTracker.java @@ -0,0 +1,83 @@ +/* + * Copyright (C) 2020 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.launcher3.ui; + +import android.app.Activity; +import android.app.Application; +import android.os.Bundle; + +import androidx.test.InstrumentationRegistry; + +import com.android.launcher3.tapl.TestHelpers; + +import java.util.WeakHashMap; + +class ActivityLeakTracker implements Application.ActivityLifecycleCallbacks { + private final WeakHashMap mActivities = new WeakHashMap<>(); + + ActivityLeakTracker() { + if (!TestHelpers.isInLauncherProcess()) return; + final Application app = + (Application) InstrumentationRegistry.getTargetContext().getApplicationContext(); + app.registerActivityLifecycleCallbacks(this); + } + + @Override + public void onActivityCreated(Activity activity, Bundle bundle) { + mActivities.put(activity, true); + } + + @Override + public void onActivityStarted(Activity activity) { + } + + @Override + public void onActivityResumed(Activity activity) { + } + + @Override + public void onActivityPaused(Activity activity) { + } + + @Override + public void onActivityStopped(Activity activity) { + } + + @Override + public void onActivitySaveInstanceState(Activity activity, Bundle bundle) { + } + + @Override + public void onActivityDestroyed(Activity activity) { + } + + public boolean noLeakedActivities() { + int liveActivities = 0; + int destroyedActivities = 0; + + for (Activity activity : mActivities.keySet()) { + if (activity.isDestroyed()) { + ++destroyedActivities; + } else { + ++liveActivities; + } + } + + // It's OK to have 1 leaked activity if no active activities exist. + return liveActivities == 0 ? destroyedActivities <= 1 : destroyedActivities == 0; + } +} diff --git a/tests/src/com/android/launcher3/ui/PortraitLandscapeRunner.java b/tests/src/com/android/launcher3/ui/PortraitLandscapeRunner.java index 38f50c1ef2..266f0aeb1d 100644 --- a/tests/src/com/android/launcher3/ui/PortraitLandscapeRunner.java +++ b/tests/src/com/android/launcher3/ui/PortraitLandscapeRunner.java @@ -56,7 +56,7 @@ class PortraitLandscapeRunner implements TestRule { private void evaluateInPortrait() throws Throwable { mTest.mDevice.setOrientationNatural(); mTest.mLauncher.setExpectedRotation(Surface.ROTATION_0); - AbstractLauncherUiTest.checkDetectedLeaks(); + AbstractLauncherUiTest.checkDetectedLeaks(mTest.mLauncher); base.evaluate(); mTest.getDevice().pressHome(); } @@ -64,7 +64,7 @@ class PortraitLandscapeRunner implements TestRule { private void evaluateInLandscape() throws Throwable { mTest.mDevice.setOrientationLeft(); mTest.mLauncher.setExpectedRotation(Surface.ROTATION_90); - AbstractLauncherUiTest.checkDetectedLeaks(); + AbstractLauncherUiTest.checkDetectedLeaks(mTest.mLauncher); base.evaluate(); mTest.getDevice().pressHome(); } diff --git a/tests/src/com/android/launcher3/ui/TaplTestsLauncher3.java b/tests/src/com/android/launcher3/ui/TaplTestsLauncher3.java index 57000a06c3..34e425d5fc 100644 --- a/tests/src/com/android/launcher3/ui/TaplTestsLauncher3.java +++ b/tests/src/com/android/launcher3/ui/TaplTestsLauncher3.java @@ -64,7 +64,7 @@ public class TaplTestsLauncher3 extends AbstractLauncherUiTest { test.waitForResumed("Launcher internal state is still Background"); // Check that we switched to home. test.mLauncher.getWorkspace(); - AbstractLauncherUiTest.checkDetectedLeaks(); + AbstractLauncherUiTest.checkDetectedLeaks(test.mLauncher); } // Please don't add negative test cases for methods that fail only after a long wait.