From 3eee43845adba7f01efbbb639dfe52737e58f060 Mon Sep 17 00:00:00 2001 From: Zimuzo Date: Tue, 8 Jan 2019 20:42:39 +0000 Subject: [PATCH] Fix PackageWatchdog and add PackageWatchdogTest Fixes: 1. Remove registered observer when removed from persisted file 2. Only call external observers after threshold is exceeded 3. Handle edge case where we reschedule package cleanup and elapsed time is longer than scheduled duration 4. Modify code to allow easier testing Bug: 120598832 Test: atest PackageWatchdogTest Change-Id: I92181136fb5994a4d8ebe976be3138f210e853a5 --- .../com/android/server/PackageWatchdog.java | 106 +++++-- tests/PackageWatchdog/Android.mk | 34 +++ tests/PackageWatchdog/AndroidManifest.xml | 28 ++ .../android/server/PackageWatchdogTest.java | 286 ++++++++++++++++++ 4 files changed, 424 insertions(+), 30 deletions(-) create mode 100644 tests/PackageWatchdog/Android.mk create mode 100644 tests/PackageWatchdog/AndroidManifest.xml create mode 100644 tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java diff --git a/services/core/java/com/android/server/PackageWatchdog.java b/services/core/java/com/android/server/PackageWatchdog.java index f27d37315ada9..8adc416fda950 100644 --- a/services/core/java/com/android/server/PackageWatchdog.java +++ b/services/core/java/com/android/server/PackageWatchdog.java @@ -16,6 +16,7 @@ package com.android.server; +import android.annotation.Nullable; import android.content.Context; import android.os.Environment; import android.os.Handler; @@ -29,6 +30,7 @@ import android.util.Slog; import android.util.Xml; import com.android.internal.annotations.GuardedBy; +import com.android.internal.annotations.VisibleForTesting; import com.android.internal.os.BackgroundThread; import com.android.internal.util.FastXmlSerializer; import com.android.internal.util.XmlUtils; @@ -46,8 +48,10 @@ import java.io.IOException; import java.io.InputStream; import java.nio.charset.StandardCharsets; import java.util.ArrayList; +import java.util.Collections; import java.util.Iterator; import java.util.List; +import java.util.Set; /** * Monitors the health of packages on the system and notifies interested observers when packages @@ -58,7 +62,7 @@ public class PackageWatchdog { // Duration to count package failures before it resets to 0 private static final int TRIGGER_DURATION_MS = 60000; // Number of package failures within the duration above before we notify observers - private static final int TRIGGER_FAILURE_COUNT = 5; + static final int TRIGGER_FAILURE_COUNT = 5; private static final int DB_VERSION = 1; private static final String TAG_PACKAGE_WATCHDOG = "package-watchdog"; private static final String TAG_PACKAGE = "package"; @@ -75,20 +79,13 @@ public class PackageWatchdog { // Handler to run package cleanup runnables private final Handler mTimerHandler; private final Handler mIoHandler; - // Contains (observer-name -> external-observer-handle) that have been registered during the - // current boot. - // It is populated when observers call #registerHealthObserver and it does not survive reboots. - @GuardedBy("mLock") - final ArrayMap mRegisteredObservers = new ArrayMap<>(); - // Contains (observer-name -> internal-observer-handle) that have ever been registered from + // Contains (observer-name -> observer-handle) that have ever been registered from // previous boots. Observers with all packages expired are periodically pruned. // It is saved to disk on system shutdown and repouplated on startup so it survives reboots. @GuardedBy("mLock") - final ArrayMap mAllObservers = new ArrayMap<>(); + private final ArrayMap mAllObservers = new ArrayMap<>(); // File containing the XML data of monitored packages /data/system/package-watchdog.xml - private final AtomicFile mPolicyFile = - new AtomicFile(new File(new File(Environment.getDataDirectory(), "system"), - "package-watchdog.xml")); + private final AtomicFile mPolicyFile; // Runnable to prune monitored packages that have expired private final Runnable mPackageCleanup; // Last SystemClock#uptimeMillis a package clean up was executed. @@ -98,14 +95,32 @@ public class PackageWatchdog { // 0 if mPackageCleanup not running. private long mDurationAtLastReschedule; + // TODO(zezeozue): Remove redundant context param private PackageWatchdog(Context context) { mContext = context; + mPolicyFile = new AtomicFile(new File(new File(Environment.getDataDirectory(), "system"), + "package-watchdog.xml")); mTimerHandler = new Handler(Looper.myLooper()); mIoHandler = BackgroundThread.getHandler(); mPackageCleanup = this::rescheduleCleanup; loadFromFile(); } + /** + * Creates a PackageWatchdog for testing that uses the same {@code looper} for all handlers + * and creates package-watchdog.xml in an apps data directory. + */ + @VisibleForTesting + PackageWatchdog(Context context, Looper looper) { + mContext = context; + mPolicyFile = new AtomicFile(new File(context.getFilesDir(), "package-watchdog.xml")); + mTimerHandler = new Handler(looper); + mIoHandler = mTimerHandler; + mPackageCleanup = this::rescheduleCleanup; + loadFromFile(); + } + + /** Creates or gets singleton instance of PackageWatchdog. */ public static PackageWatchdog getInstance(Context context) { synchronized (PackageWatchdog.class) { @@ -124,7 +139,10 @@ public class PackageWatchdog { */ public void registerHealthObserver(PackageHealthObserver observer) { synchronized (mLock) { - mRegisteredObservers.put(observer.getName(), observer); + ObserverInternal internalObserver = mAllObservers.get(observer.getName()); + if (internalObserver != null) { + internalObserver.mRegisteredObserver = observer; + } if (mDurationAtLastReschedule == 0) { // Nothing running, schedule rescheduleCleanup(); @@ -143,7 +161,7 @@ public class PackageWatchdog { * or {@code durationMs} is less than 1 */ public void startObservingHealth(PackageHealthObserver observer, List packageNames, - int durationMs) { + long durationMs) { if (packageNames.isEmpty() || durationMs < 1) { throw new IllegalArgumentException("Observation not started, no packages specified" + "or invalid duration"); @@ -180,11 +198,32 @@ public class PackageWatchdog { public void unregisterHealthObserver(PackageHealthObserver observer) { synchronized (mLock) { mAllObservers.remove(observer.getName()); - mRegisteredObservers.remove(observer.getName()); } saveToFileAsync(); } + /** + * Returns packages observed by {@code observer} + * + * @return an empty set if {@code observer} has some packages observerd from a previous boot + * but has not registered itself in the current boot to receive notifications. Returns null + * if there are no active packages monitored from any boot. + */ + @Nullable + public Set getPackages(PackageHealthObserver observer) { + synchronized (mLock) { + for (int i = 0; i < mAllObservers.size(); i++) { + if (observer.getName().equals(mAllObservers.keyAt(i))) { + if (observer.equals(mAllObservers.valueAt(i).mRegisteredObserver)) { + return mAllObservers.valueAt(i).mPackages.keySet(); + } + return Collections.emptySet(); + } + } + } + return null; + } + // TODO(zezeozue:) Accept current versionCodes of failing packages? /** * Called when a process fails either due to a crash or ANR. @@ -198,33 +237,35 @@ public class PackageWatchdog { public void onPackageFailure(String[] packages) { ArrayMap> packagesToReport = new ArrayMap<>(); synchronized (mLock) { - if (mRegisteredObservers.isEmpty()) { + if (mAllObservers.isEmpty()) { return; } for (int pIndex = 0; pIndex < packages.length; pIndex++) { + // Observers interested in receiving packageName failures + List observersToNotify = new ArrayList<>(); for (int oIndex = 0; oIndex < mAllObservers.size(); oIndex++) { - // Observers interested in receiving packageName failures - List observersToNotify = new ArrayList<>(); - PackageHealthObserver activeObserver = - mRegisteredObservers.get(mAllObservers.valueAt(oIndex).mName); - if (activeObserver != null) { - observersToNotify.add(activeObserver); - } - - // Save interested observers and notify them outside the lock - if (!observersToNotify.isEmpty()) { - packagesToReport.put(packages[pIndex], observersToNotify); + PackageHealthObserver registeredObserver = + mAllObservers.valueAt(oIndex).mRegisteredObserver; + if (registeredObserver != null) { + observersToNotify.add(registeredObserver); } } + // Save interested observers and notify them outside the lock + if (!observersToNotify.isEmpty()) { + packagesToReport.put(packages[pIndex], observersToNotify); + } } } // Notify observers for (int pIndex = 0; pIndex < packagesToReport.size(); pIndex++) { List observers = packagesToReport.valueAt(pIndex); + String packageName = packages[pIndex]; for (int oIndex = 0; oIndex < observers.size(); oIndex++) { - if (observers.get(oIndex).onHealthCheckFailed(packages[pIndex])) { + PackageHealthObserver observer = observers.get(oIndex); + if (mAllObservers.get(observer.getName()).onPackageFailure(packageName) + && observer.onHealthCheckFailed(packageName)) { // Observer has handled, do not notify others break; } @@ -275,10 +316,12 @@ public class PackageWatchdog { // O if mPackageCleanup not running long elapsedDurationMs = mUptimeAtLastRescheduleMs == 0 ? 0 : uptimeMs - mUptimeAtLastRescheduleMs; - // O if mPackageCleanup not running + // Less than O if mPackageCleanup unexpectedly didn't run yet even though + // and we are past the last duration scheduled to run long remainingDurationMs = mDurationAtLastReschedule - elapsedDurationMs; - - if (mUptimeAtLastRescheduleMs == 0 || nextDurationToScheduleMs < remainingDurationMs) { + if (mUptimeAtLastRescheduleMs == 0 + || remainingDurationMs <= 0 + || nextDurationToScheduleMs < remainingDurationMs) { // First schedule or an earlier reschedule pruneObservers(elapsedDurationMs); mTimerHandler.removeCallbacks(mPackageCleanup); @@ -305,6 +348,7 @@ public class PackageWatchdog { } } Slog.v(TAG, "Earliest package time is " + shortestDurationMs); + return shortestDurationMs; } @@ -409,6 +453,8 @@ public class PackageWatchdog { static class ObserverInternal { public final String mName; public final ArrayMap mPackages; + @Nullable + public PackageHealthObserver mRegisteredObserver; ObserverInternal(String name, List packages) { mName = name; diff --git a/tests/PackageWatchdog/Android.mk b/tests/PackageWatchdog/Android.mk new file mode 100644 index 0000000000000..b53f27d28740e --- /dev/null +++ b/tests/PackageWatchdog/Android.mk @@ -0,0 +1,34 @@ +# Copyright (C) 2019 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. + +LOCAL_PATH:= $(call my-dir) + +# PackageWatchdogTest +include $(CLEAR_VARS) +LOCAL_SRC_FILES := $(call all-java-files-under, src) +LOCAL_PACKAGE_NAME := PackageWatchdogTest +LOCAL_MODULE_TAGS := tests +LOCAL_STATIC_JAVA_LIBRARIES := \ + junit \ + frameworks-base-testutils \ + android-support-test \ + services + +LOCAL_JAVA_LIBRARIES := \ + android.test.runner + +LOCAL_PRIVATE_PLATFORM_APIS := true +LOCAL_COMPATIBILITY_SUITE := device-tests + +include $(BUILD_PACKAGE) diff --git a/tests/PackageWatchdog/AndroidManifest.xml b/tests/PackageWatchdog/AndroidManifest.xml new file mode 100644 index 0000000000000..fa89528403c30 --- /dev/null +++ b/tests/PackageWatchdog/AndroidManifest.xml @@ -0,0 +1,28 @@ + + + + + + + + + + + + diff --git a/tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java b/tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java new file mode 100644 index 0000000000000..ec07037b3a8f5 --- /dev/null +++ b/tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java @@ -0,0 +1,286 @@ +/* + * Copyright (C) 2019 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.server; + +import static com.android.server.PackageWatchdog.TRIGGER_FAILURE_COUNT; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; + +import android.os.test.TestLooper; +import android.support.test.InstrumentationRegistry; + +import com.android.server.PackageWatchdog.PackageHealthObserver; + +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +import java.io.File; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.concurrent.TimeUnit; + +// TODO(zezeozue): Write test without using PackageWatchdog#getPackages. Just rely on +// behavior of observers receiving crash notifications or not to determine if it's registered +/** + * Test PackageWatchdog. + */ +public class PackageWatchdogTest { + private static final String APP_A = "com.package.a"; + private static final String APP_B = "com.package.b"; + private static final String OBSERVER_NAME_1 = "observer1"; + private static final String OBSERVER_NAME_2 = "observer2"; + private static final String OBSERVER_NAME_3 = "observer3"; + private static final long SHORT_DURATION = TimeUnit.SECONDS.toMillis(1); + private static final long LONG_DURATION = TimeUnit.SECONDS.toMillis(5); + private TestLooper mTestLooper; + + @Before + public void setUp() throws Exception { + mTestLooper = new TestLooper(); + mTestLooper.startAutoDispatch(); + } + + @After + public void tearDown() throws Exception { + new File(InstrumentationRegistry.getContext().getFilesDir(), + "package-watchdog.xml").delete(); + } + + /** + * Test registration, unregistration, package expiry and duration reduction + */ + @Test + public void testRegistration() throws Exception { + PackageWatchdog watchdog = createWatchdog(); + TestObserver observer1 = new TestObserver(OBSERVER_NAME_1); + TestObserver observer2 = new TestObserver(OBSERVER_NAME_2); + TestObserver observer3 = new TestObserver(OBSERVER_NAME_3); + + // Start observing for observer1 which will be unregistered + watchdog.startObservingHealth(observer1, Arrays.asList(APP_A), SHORT_DURATION); + // Start observing for observer2 which will expire + watchdog.startObservingHealth(observer2, Arrays.asList(APP_A, APP_B), SHORT_DURATION); + // Start observing for observer3 which will have expiry duration reduced + watchdog.startObservingHealth(observer3, Arrays.asList(APP_A), LONG_DURATION); + + // Verify packages observed at start + // 1 + assertEquals(1, watchdog.getPackages(observer1).size()); + assertTrue(watchdog.getPackages(observer1).contains(APP_A)); + // 2 + assertEquals(2, watchdog.getPackages(observer2).size()); + assertTrue(watchdog.getPackages(observer2).contains(APP_A)); + assertTrue(watchdog.getPackages(observer2).contains(APP_B)); + // 3 + assertEquals(1, watchdog.getPackages(observer3).size()); + assertTrue(watchdog.getPackages(observer3).contains(APP_A)); + + // Then unregister observer1 + watchdog.unregisterHealthObserver(observer1); + + // Verify observer2 and observer3 left + // 1 + assertNull(watchdog.getPackages(observer1)); + // 2 + assertEquals(2, watchdog.getPackages(observer2).size()); + assertTrue(watchdog.getPackages(observer2).contains(APP_A)); + assertTrue(watchdog.getPackages(observer2).contains(APP_B)); + // 3 + assertEquals(1, watchdog.getPackages(observer3).size()); + assertTrue(watchdog.getPackages(observer3).contains(APP_A)); + + // Then advance time a little and run messages in Handlers so observer2 expires + Thread.sleep(SHORT_DURATION); + mTestLooper.dispatchAll(); + + // Verify observer3 left with reduced expiry duration + // 1 + assertNull(watchdog.getPackages(observer1)); + // 2 + assertNull(watchdog.getPackages(observer2)); + // 3 + assertEquals(1, watchdog.getPackages(observer3).size()); + assertTrue(watchdog.getPackages(observer3).contains(APP_A)); + + // Then advance time some more and run messages in Handlers so observer3 expires + Thread.sleep(LONG_DURATION); + mTestLooper.dispatchAll(); + + // Verify observer3 expired + // 1 + assertNull(watchdog.getPackages(observer1)); + // 2 + assertNull(watchdog.getPackages(observer2)); + // 3 + assertNull(watchdog.getPackages(observer3)); + } + + /** + * Test package observers are persisted and loaded on startup + */ + @Test + public void testPersistence() throws Exception { + PackageWatchdog watchdog1 = createWatchdog(); + TestObserver observer1 = new TestObserver(OBSERVER_NAME_1); + TestObserver observer2 = new TestObserver(OBSERVER_NAME_2); + + watchdog1.startObservingHealth(observer1, Arrays.asList(APP_A), SHORT_DURATION); + watchdog1.startObservingHealth(observer2, Arrays.asList(APP_A, APP_B), SHORT_DURATION); + + // Verify 2 observers are registered and saved internally + // 1 + assertEquals(1, watchdog1.getPackages(observer1).size()); + assertTrue(watchdog1.getPackages(observer1).contains(APP_A)); + // 2 + assertEquals(2, watchdog1.getPackages(observer2).size()); + assertTrue(watchdog1.getPackages(observer2).contains(APP_A)); + assertTrue(watchdog1.getPackages(observer2).contains(APP_B)); + + + // Then advance time and run IO Handler so file is saved + mTestLooper.dispatchAll(); + + // Then start a new watchdog + PackageWatchdog watchdog2 = createWatchdog(); + + // Verify the new watchdog loads observers on startup but nothing registered + assertEquals(0, watchdog2.getPackages(observer1).size()); + assertEquals(0, watchdog2.getPackages(observer2).size()); + // Verify random observer not saved returns null + assertNull(watchdog2.getPackages(new TestObserver(OBSERVER_NAME_3))); + + // Then regiser observer1 + watchdog2.registerHealthObserver(observer1); + watchdog2.registerHealthObserver(observer2); + + // Verify 2 observers are registered after reload + // 1 + assertEquals(1, watchdog1.getPackages(observer1).size()); + assertTrue(watchdog1.getPackages(observer1).contains(APP_A)); + // 2 + assertEquals(2, watchdog1.getPackages(observer2).size()); + assertTrue(watchdog1.getPackages(observer2).contains(APP_A)); + assertTrue(watchdog1.getPackages(observer2).contains(APP_B)); + } + + /** + * Test package failure under threshold does not notify observers + */ + @Test + public void testNoPackageFailureBeforeThreshold() throws Exception { + PackageWatchdog watchdog = createWatchdog(); + TestObserver observer1 = new TestObserver(OBSERVER_NAME_1); + TestObserver observer2 = new TestObserver(OBSERVER_NAME_2); + + watchdog.startObservingHealth(observer2, Arrays.asList(APP_A), SHORT_DURATION); + watchdog.startObservingHealth(observer1, Arrays.asList(APP_A), SHORT_DURATION); + + // Then fail APP_A below the threshold + for (int i = 0; i < TRIGGER_FAILURE_COUNT - 1; i++) { + watchdog.onPackageFailure(new String[]{APP_A}); + } + + // Verify that observers are not notified + assertEquals(0, observer1.mFailedPackages.size()); + assertEquals(0, observer2.mFailedPackages.size()); + } + + /** + * Test package failure and notifies all observer since none handles the failure + */ + @Test + public void testPackageFailureNotifyAll() throws Exception { + PackageWatchdog watchdog = createWatchdog(); + TestObserver observer1 = new TestObserver(OBSERVER_NAME_1); + TestObserver observer2 = new TestObserver(OBSERVER_NAME_2); + + // Start observing for observer1 and observer2 without handling failures + watchdog.startObservingHealth(observer2, Arrays.asList(APP_A), SHORT_DURATION); + watchdog.startObservingHealth(observer1, Arrays.asList(APP_A, APP_B), SHORT_DURATION); + + // Then fail APP_A and APP_B above the threshold + for (int i = 0; i < TRIGGER_FAILURE_COUNT; i++) { + watchdog.onPackageFailure(new String[]{APP_A, APP_B}); + } + + // Verify all observers are notifed of all package failures + List observer1Packages = observer1.mFailedPackages; + List observer2Packages = observer2.mFailedPackages; + assertEquals(2, observer1Packages.size()); + assertEquals(1, observer2Packages.size()); + assertEquals(APP_A, observer1Packages.get(0)); + assertEquals(APP_B, observer1Packages.get(1)); + assertEquals(APP_A, observer2Packages.get(0)); + } + + /** + * Test package failure and notifies only one observer because it handles the failure + */ + @Test + public void testPackageFailureNotifyOne() throws Exception { + PackageWatchdog watchdog = createWatchdog(); + TestObserver observer1 = new TestObserver(OBSERVER_NAME_1, true /* shouldHandle */); + TestObserver observer2 = new TestObserver(OBSERVER_NAME_2, true /* shouldHandle */); + + // Start observing for observer1 and observer2 with failure handling + watchdog.startObservingHealth(observer2, Arrays.asList(APP_A), SHORT_DURATION); + watchdog.startObservingHealth(observer1, Arrays.asList(APP_A), SHORT_DURATION); + + // Then fail APP_A above the threshold + for (int i = 0; i < TRIGGER_FAILURE_COUNT; i++) { + watchdog.onPackageFailure(new String[]{APP_A}); + } + + // Verify only one observer is notifed + assertEquals(1, observer1.mFailedPackages.size()); + assertEquals(APP_A, observer1.mFailedPackages.get(0)); + assertEquals(0, observer2.mFailedPackages.size()); + } + + private PackageWatchdog createWatchdog() { + return new PackageWatchdog(InstrumentationRegistry.getContext(), + mTestLooper.getLooper()); + } + + private static class TestObserver implements PackageHealthObserver { + private final String mName; + private boolean mShouldHandle; + final List mFailedPackages = new ArrayList<>(); + + TestObserver(String name) { + mName = name; + } + + TestObserver(String name, boolean shouldHandle) { + mName = name; + mShouldHandle = shouldHandle; + } + + public boolean onHealthCheckFailed(String packageName) { + mFailedPackages.add(packageName); + return mShouldHandle; + } + + public String getName() { + return mName; + } + } +}