From 33139c16611fed10883afe8cae5f03c8a69570b4 Mon Sep 17 00:00:00 2001 From: Paul Hadfield Date: Mon, 12 Jul 2021 16:20:37 +0000 Subject: [PATCH] Fix onPackageChanged handling of disabled packages TransportManager#onPackageChanged was not correctly handling the circumstance where broadcast ACTION_PACKAGE_CHANGED was received with EXTRA_CHANGED_COMPONENT_NAME_LIST containing only the package name, instead of >=1 component names. When that happens it indicates that a package-wide change has occurred, such as the package being enabled or disabled. If the package in question contains backup transports, they should be unregistered if the package is disabled, and re- registered if the package is enabled. But the current TransportManager#onPackageChanged doesn't know enough to do so. We can determine the enabled state of the package by calling PackageManager#getApplicationEnabledSetting. This commit modifies onPackageChanged to do that, and then (un/re)-register the packaged transports as appropriate. To test I extend the Roboelectric ShadowApplicationPackageManager so that the {get,set}ApplicationEnabledState() methods are exposed for use in the test setup. Bug: 162725876 Fixes: 162725876 Test: 1. atest -v TransportManagerTest 2. manual test on device: 1 device:/ # bmgr list transports 2 device:/ # pm disable com.google.android.gms 3 device:/ # bmgr list transports 4 device:/ # pm enable com.google.android.gms 5 device:/ # bmgr list transports observe logcat showing BackupTransportManager MORE_DEBUG: step 2: Package c.g.a.gms was disabled. step 4: Package c.g.a.gms was enabled. Transport c.g.a.gms/.b.c.D2dTransportService registered Change-Id: I7bd397f0f1eadbbca6f371c3c9710df5386aca13 --- .../server/backup/TransportManager.java | 42 +++++++++++ .../server/backup/TransportManagerTest.java | 69 +++++++++++++++++++ .../ShadowApplicationPackageManager.java | 17 +++++ 3 files changed, 128 insertions(+) diff --git a/services/backup/backuplib/java/com/android/server/backup/TransportManager.java b/services/backup/backuplib/java/com/android/server/backup/TransportManager.java index fd573d5e06658..b3773e3bcb534 100644 --- a/services/backup/backuplib/java/com/android/server/backup/TransportManager.java +++ b/services/backup/backuplib/java/com/android/server/backup/TransportManager.java @@ -16,6 +16,9 @@ package com.android.server.backup; +import static android.content.pm.PackageManager.COMPONENT_ENABLED_STATE_DISABLED; +import static android.content.pm.PackageManager.COMPONENT_ENABLED_STATE_ENABLED; + import android.annotation.Nullable; import android.annotation.UserIdInt; import android.annotation.WorkerThread; @@ -57,6 +60,7 @@ import java.util.function.Predicate; /** Handles in-memory bookkeeping of all BackupTransport objects. */ public class TransportManager { private static final String TAG = "BackupTransportManager"; + private static final boolean MORE_DEBUG = false; @VisibleForTesting public static final String SERVICE_ACTION_TRANSPORT_HOST = "android.backup.TRANSPORT_HOST"; @@ -128,14 +132,52 @@ public class TransportManager { } } + void onPackageEnabled(String packageName) { + onPackageAdded(packageName); + } + + void onPackageDisabled(String packageName) { + onPackageRemoved(packageName); + } + @WorkerThread void onPackageChanged(String packageName, String... components) { + // Determine if the overall package has changed and not just its + // components - see {@link EXTRA_CHANGED_COMPONENT_NAME_LIST}. When we + // know a package was enabled/disabled we'll handle that directly and + // not continue with onPackageChanged. + if (components.length == 1 && components[0].equals(packageName)) { + final int enabled = mPackageManager.getApplicationEnabledSetting(packageName); + switch (enabled) { + case COMPONENT_ENABLED_STATE_ENABLED: { + if (MORE_DEBUG) { + Slog.d(TAG, "Package " + packageName + " was enabled."); + } + onPackageEnabled(packageName); + return; + } + case COMPONENT_ENABLED_STATE_DISABLED: { + if (MORE_DEBUG) { + Slog.d(TAG, "Package " + packageName + " was disabled."); + } + onPackageDisabled(packageName); + return; + } + default: { + Slog.w(TAG, "Package " + packageName + " enabled setting: " + enabled); + return; + } + } + } // Unfortunately this can't be atomic because we risk a deadlock if // registerTransportsFromPackage() is put inside the synchronized block Set transportComponents = new ArraySet<>(components.length); for (String componentName : components) { transportComponents.add(new ComponentName(packageName, componentName)); } + if (transportComponents.isEmpty()) { + return; + } synchronized (mTransportLock) { mRegisteredTransportsDescriptionMap.keySet().removeIf(transportComponents::contains); } diff --git a/services/robotests/backup/src/com/android/server/backup/TransportManagerTest.java b/services/robotests/backup/src/com/android/server/backup/TransportManagerTest.java index 42115d437ee06..fc36655bf516c 100644 --- a/services/robotests/backup/src/com/android/server/backup/TransportManagerTest.java +++ b/services/robotests/backup/src/com/android/server/backup/TransportManagerTest.java @@ -16,6 +16,10 @@ package com.android.server.backup; +import static android.content.pm.PackageManager.COMPONENT_ENABLED_STATE_DEFAULT; +import static android.content.pm.PackageManager.COMPONENT_ENABLED_STATE_DISABLED; +import static android.content.pm.PackageManager.COMPONENT_ENABLED_STATE_ENABLED; + import static com.android.server.backup.testing.TransportData.genericTransport; import static com.android.server.backup.testing.TransportTestUtils.mockTransport; import static com.android.server.backup.testing.TransportTestUtils.setUpTransportsForTransportManager; @@ -311,6 +315,71 @@ public class TransportManagerTest { .onTransportRegistered(mTransportA2.transportName, mTransportA2.transportDirName); } + @Test + public void testOnPackageChanged_onPackageChanged_packageDisabledUnregistersTransport() + throws Exception { + TransportManager transportManager = + createTransportManagerWithRegisteredTransports(mTransportA1, mTransportB1); + reset(mListener); + + mContext.getPackageManager() + .setApplicationEnabledSetting( + PACKAGE_A, + Integer.valueOf(COMPONENT_ENABLED_STATE_DISABLED), + 0 /*flags*/); + transportManager.onPackageChanged(PACKAGE_A, PACKAGE_A); + + assertRegisteredTransports(transportManager, singletonList(mTransportB1)); + verify(mListener, never()).onTransportRegistered(any(), any()); + } + + @Test + public void testOnPackageChanged_onPackageChanged_packageEnabledRegistersTransport() + throws Exception { + TransportManager transportManager = + createTransportManagerWithRegisteredTransports(mTransportA1, mTransportB1); + reset(mListener); + + mContext.getPackageManager() + .setApplicationEnabledSetting( + PACKAGE_A, + Integer.valueOf(COMPONENT_ENABLED_STATE_DISABLED), + 0 /*flags*/); + transportManager.onPackageChanged(PACKAGE_A, PACKAGE_A); + + assertRegisteredTransports(transportManager, singletonList(mTransportB1)); + verify(mListener, never()).onTransportRegistered(any(), any()); + + mContext.getPackageManager() + .setApplicationEnabledSetting( + PACKAGE_A, + Integer.valueOf(COMPONENT_ENABLED_STATE_ENABLED), + 0 /*flags*/); + transportManager.onPackageChanged(PACKAGE_A, PACKAGE_A); + + assertRegisteredTransports(transportManager, asList(mTransportA1, mTransportB1)); + verify(mListener) + .onTransportRegistered(mTransportA1.transportName, mTransportA1.transportDirName); + } + + @Test + public void testOnPackageChanged_onPackageChanged_unknownComponentStateIsIgnored() + throws Exception { + TransportManager transportManager = + createTransportManagerWithRegisteredTransports(mTransportA1, mTransportB1); + reset(mListener); + + mContext.getPackageManager() + .setApplicationEnabledSetting( + PACKAGE_A, + Integer.valueOf(COMPONENT_ENABLED_STATE_DEFAULT), + 0 /*flags*/); + transportManager.onPackageChanged(PACKAGE_A, PACKAGE_A); + + assertRegisteredTransports(transportManager, asList(mTransportA1, mTransportB1)); + verify(mListener, never()).onTransportRegistered(any(), any()); + } + @Test public void testRegisterAndSelectTransport_whenTransportRegistered() throws Exception { TransportManager transportManager = diff --git a/services/robotests/src/com/android/server/testing/shadows/ShadowApplicationPackageManager.java b/services/robotests/src/com/android/server/testing/shadows/ShadowApplicationPackageManager.java index aea36e555ad7b..e4c34a307568f 100644 --- a/services/robotests/src/com/android/server/testing/shadows/ShadowApplicationPackageManager.java +++ b/services/robotests/src/com/android/server/testing/shadows/ShadowApplicationPackageManager.java @@ -16,6 +16,7 @@ package com.android.server.testing.shadows; +import static android.content.pm.PackageManager.COMPONENT_ENABLED_STATE_DEFAULT; import static android.content.pm.PackageManager.NameNotFoundException; import android.app.ApplicationPackageManager; @@ -44,6 +45,7 @@ public class ShadowApplicationPackageManager private static final List sInstalledPackages = new ArrayList<>(); private static final Map sPackageUids = new ArrayMap<>(); private static final Map> sUserPackageUids = new ArrayMap<>(); + private static final Map sPackageAppEnabledStates = new ArrayMap<>(); /** * Registers the package {@code packageName} to be returned when invoking {@link @@ -53,6 +55,7 @@ public class ShadowApplicationPackageManager public static void addInstalledPackage(String packageName, PackageInfo packageInfo) { sPackageInfos.put(packageName, packageInfo); sInstalledPackages.add(packageInfo); + sPackageAppEnabledStates.put(packageName, Integer.valueOf(COMPONENT_ENABLED_STATE_DEFAULT)); } /** @@ -76,6 +79,19 @@ public class ShadowApplicationPackageManager sUserPackageUids.put(userId, userPackageUids); } + @Override + protected int getApplicationEnabledSetting(String packageName) { + if (!sPackageAppEnabledStates.containsKey(packageName)) { + return COMPONENT_ENABLED_STATE_DEFAULT; + } + return sPackageAppEnabledStates.get(packageName); + } + + @Override + protected void setApplicationEnabledSetting(String packageName, int newState, int flags) { + sPackageAppEnabledStates.put(packageName, Integer.valueOf(newState)); // flags unused here. + } + @Override protected PackageInfo getPackageInfoAsUser(String packageName, int flags, int userId) throws NameNotFoundException { @@ -115,6 +131,7 @@ public class ShadowApplicationPackageManager public static void reset() { sPackageInfos.clear(); sInstalledPackages.clear(); + sPackageAppEnabledStates.clear(); org.robolectric.shadows.ShadowApplicationPackageManager.reset(); } }