From 5f4e3f8fdf785864f64276c7978158d998bd4199 Mon Sep 17 00:00:00 2001 From: JW Wang Date: Fri, 29 Jan 2021 17:20:19 +0800 Subject: [PATCH 1/5] Exhaust due runnables before time-leap (1/n) Fix test failures caused by runnables incorrectly executed after time-leap. Bug: 178675924 Test: atest PackageWatchdogTest Change-Id: Iacad3a2f5a26e6bd28cd04e96a43d1c6d917513a --- .../src/com/android/server/PackageWatchdogTest.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java b/tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java index 104758de49f19..41897ee14106f 100644 --- a/tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java +++ b/tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java @@ -850,7 +850,6 @@ public class PackageWatchdogTest { watchdog.startObservingHealth(observer, Arrays.asList(APP_A, APP_B), Long.MAX_VALUE); watchdog.onPackageFailure(Arrays.asList(new VersionedPackage(APP_A, VERSION_CODE)), PackageWatchdog.FAILURE_REASON_UNKNOWN); - mTestLooper.dispatchAll(); moveTimeForwardAndDispatch(PackageWatchdog.DEFAULT_TRIGGER_FAILURE_DURATION_MS + 1); watchdog.onPackageFailure(Arrays.asList(new VersionedPackage(APP_A, VERSION_CODE)), PackageWatchdog.FAILURE_REASON_UNKNOWN); @@ -862,7 +861,6 @@ public class PackageWatchdogTest { watchdog.onPackageFailure(Arrays.asList(new VersionedPackage(APP_B, VERSION_CODE)), PackageWatchdog.FAILURE_REASON_UNKNOWN); - mTestLooper.dispatchAll(); moveTimeForwardAndDispatch(PackageWatchdog.DEFAULT_TRIGGER_FAILURE_DURATION_MS - 1); watchdog.onPackageFailure(Arrays.asList(new VersionedPackage(APP_B, VERSION_CODE)), PackageWatchdog.FAILURE_REASON_UNKNOWN); @@ -933,11 +931,9 @@ public class PackageWatchdogTest { // Raise 2 failures at t=0 and t=900 respectively watchdog.onPackageFailure(Arrays.asList(new VersionedPackage(APP_A, VERSION_CODE)), PackageWatchdog.FAILURE_REASON_UNKNOWN); - mTestLooper.dispatchAll(); moveTimeForwardAndDispatch(900); watchdog.onPackageFailure(Arrays.asList(new VersionedPackage(APP_A, VERSION_CODE)), PackageWatchdog.FAILURE_REASON_UNKNOWN); - mTestLooper.dispatchAll(); // Raise 2 failures at t=1100 moveTimeForwardAndDispatch(200); @@ -1312,6 +1308,8 @@ public class PackageWatchdogTest { } private void moveTimeForwardAndDispatch(long milliSeconds) { + // Exhaust all due runnables now which shouldn't be executed after time-leap + mTestLooper.dispatchAll(); mTestClock.moveTimeForward(milliSeconds); mTestLooper.moveTimeForward(milliSeconds); mTestLooper.dispatchAll(); From a6fedf6e26a80d46d9f1c59964ac3e2c55c8a19c Mon Sep 17 00:00:00 2001 From: JW Wang Date: Fri, 29 Jan 2021 17:40:44 +0800 Subject: [PATCH 2/5] Set device configs before starting tests (2/n) Otherwise some tests (testFailureHistoryIsPreserved) which assume default device config values might fail. Bug: 178675924 Test: atest PackageWatchdogTest Change-Id: I44c51d0df6973420becf01582ed2603d869a7231 --- .../android/server/PackageWatchdogTest.java | 24 ++++++++----------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java b/tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java index 41897ee14106f..9d4b74d4253aa 100644 --- a/tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java +++ b/tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java @@ -112,7 +112,8 @@ public class PackageWatchdogTest { MockitoAnnotations.initMocks(this); new File(InstrumentationRegistry.getContext().getFilesDir(), "package-watchdog.xml").delete(); - adoptShellPermissions(Manifest.permission.READ_DEVICE_CONFIG); + adoptShellPermissions(Manifest.permission.READ_DEVICE_CONFIG, + Manifest.permission.WRITE_DEVICE_CONFIG); mTestLooper = new TestLooper(); mSpyContext = spy(InstrumentationRegistry.getContext()); when(mSpyContext.getPackageManager()).thenReturn(mMockPackageManager); @@ -157,6 +158,14 @@ public class PackageWatchdogTest { return storedValue == null ? defaultValue : Long.parseLong(storedValue); } ).when(() -> SystemProperties.getLong(anyString(), anyLong())); + + DeviceConfig.setProperty(DeviceConfig.NAMESPACE_ROLLBACK, + PackageWatchdog.PROPERTY_WATCHDOG_EXPLICIT_HEALTH_CHECK_ENABLED, + Boolean.toString(true), false); + + DeviceConfig.setProperty(DeviceConfig.NAMESPACE_ROLLBACK, + PackageWatchdog.PROPERTY_WATCHDOG_TRIGGER_FAILURE_COUNT, + Integer.toString(PackageWatchdog.DEFAULT_TRIGGER_FAILURE_COUNT), false); } @After @@ -611,10 +620,6 @@ public class PackageWatchdogTest { */ @Test public void testExplicitHealthCheckStateChanges() throws Exception { - adoptShellPermissions( - Manifest.permission.WRITE_DEVICE_CONFIG, - Manifest.permission.READ_DEVICE_CONFIG); - TestController controller = new TestController(); PackageWatchdog watchdog = createWatchdog(controller, true /* withPackagesReady */); TestObserver observer = new TestObserver(OBSERVER_NAME_1, @@ -807,9 +812,6 @@ public class PackageWatchdogTest { /** Test default values are used when device property is invalid. */ @Test public void testInvalidConfig_watchdogTriggerFailureCount() { - adoptShellPermissions( - Manifest.permission.WRITE_DEVICE_CONFIG, - Manifest.permission.READ_DEVICE_CONFIG); DeviceConfig.setProperty(DeviceConfig.NAMESPACE_ROLLBACK, PackageWatchdog.PROPERTY_WATCHDOG_TRIGGER_FAILURE_COUNT, Integer.toString(-1), /*makeDefault*/false); @@ -835,9 +837,6 @@ public class PackageWatchdogTest { /** Test default values are used when device property is invalid. */ @Test public void testInvalidConfig_watchdogTriggerDurationMillis() { - adoptShellPermissions( - Manifest.permission.WRITE_DEVICE_CONFIG, - Manifest.permission.READ_DEVICE_CONFIG); DeviceConfig.setProperty(DeviceConfig.NAMESPACE_ROLLBACK, PackageWatchdog.PROPERTY_WATCHDOG_TRIGGER_FAILURE_COUNT, Integer.toString(2), /*makeDefault*/false); @@ -915,9 +914,6 @@ public class PackageWatchdogTest { /** Test we are notified when enough failures are triggered within any window. */ @Test public void testFailureTriggerWindow() { - adoptShellPermissions( - Manifest.permission.WRITE_DEVICE_CONFIG, - Manifest.permission.READ_DEVICE_CONFIG); DeviceConfig.setProperty(DeviceConfig.NAMESPACE_ROLLBACK, PackageWatchdog.PROPERTY_WATCHDOG_TRIGGER_FAILURE_COUNT, Integer.toString(3), /*makeDefault*/false); From 757da37e0bdfc5e097a64ec190a1dfdaf1c2665d Mon Sep 17 00:00:00 2001 From: JW Wang Date: Fri, 29 Jan 2021 17:51:41 +0800 Subject: [PATCH 3/5] updateConfigs() might throw during tests (3/n) The callback might be run in-between setUp() and tearDown() when the permission READ_DEVICE_CONFIG has been dropped and crash tests. Bug: 178675924 Test: atest PackageWatchdogTest Change-Id: Iefd31d08f71340706b64d84d44c41151b21193bf --- services/core/java/com/android/server/PackageWatchdog.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/services/core/java/com/android/server/PackageWatchdog.java b/services/core/java/com/android/server/PackageWatchdog.java index 8b506bac4a851..919d3d02ba78e 100644 --- a/services/core/java/com/android/server/PackageWatchdog.java +++ b/services/core/java/com/android/server/PackageWatchdog.java @@ -982,7 +982,11 @@ public class PackageWatchdog { if (!DeviceConfig.NAMESPACE_ROLLBACK.equals(properties.getNamespace())) { return; } - updateConfigs(); + try { + updateConfigs(); + } catch (Exception ignore) { + Slog.w(TAG, "Failed to reload device config changes"); + } }); } From f0541557979a429f93e2c4d057a76c79dac319d3 Mon Sep 17 00:00:00 2001 From: JW Wang Date: Fri, 29 Jan 2021 21:10:17 +0800 Subject: [PATCH 4/5] Fix a race in setExplicitHealthCheckEnabled() (4/n) Sometimes the property change callback is not called within the sleep timeout. Let's call updateConfigs() to apply device config changes immediately to eliminate the race condition. Bug: 178675924 Test: atest PackageWatchdogTest Change-Id: I2b3ce79eac36cfc5ef98a62750142bb6d936e043 --- .../java/com/android/server/PackageWatchdog.java | 3 ++- .../com/android/server/PackageWatchdogTest.java | 14 +++++++++----- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/services/core/java/com/android/server/PackageWatchdog.java b/services/core/java/com/android/server/PackageWatchdog.java index 919d3d02ba78e..16bc794dd36ad 100644 --- a/services/core/java/com/android/server/PackageWatchdog.java +++ b/services/core/java/com/android/server/PackageWatchdog.java @@ -994,7 +994,8 @@ public class PackageWatchdog { * Health check is enabled or disabled after reading the flags * from DeviceConfig. */ - private void updateConfigs() { + @VisibleForTesting + void updateConfigs() { synchronized (mLock) { mTriggerFailureCount = DeviceConfig.getInt( DeviceConfig.NAMESPACE_ROLLBACK, diff --git a/tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java b/tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java index 9d4b74d4253aa..5381009fdf2b9 100644 --- a/tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java +++ b/tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java @@ -98,6 +98,8 @@ public class PackageWatchdogTest { private final TestClock mTestClock = new TestClock(); private TestLooper mTestLooper; private Context mSpyContext; + // Keep track of all created watchdogs to apply device config changes + private List mAllocatedWatchdogs; @Mock private ConnectivityModuleConnector mConnectivityModuleConnector; @Mock @@ -166,12 +168,15 @@ public class PackageWatchdogTest { DeviceConfig.setProperty(DeviceConfig.NAMESPACE_ROLLBACK, PackageWatchdog.PROPERTY_WATCHDOG_TRIGGER_FAILURE_COUNT, Integer.toString(PackageWatchdog.DEFAULT_TRIGGER_FAILURE_COUNT), false); + + mAllocatedWatchdogs = new ArrayList<>(); } @After public void tearDown() throws Exception { dropShellPermissions(); mSession.finishMocking(); + mAllocatedWatchdogs.clear(); } @Test @@ -1295,11 +1300,9 @@ public class PackageWatchdogTest { DeviceConfig.setProperty(DeviceConfig.NAMESPACE_ROLLBACK, PackageWatchdog.PROPERTY_WATCHDOG_EXPLICIT_HEALTH_CHECK_ENABLED, Boolean.toString(enabled), /*makeDefault*/false); - //give time for DeviceConfig to broadcast the property value change - try { - Thread.sleep(SHORT_DURATION); - } catch (InterruptedException e) { - fail("Thread.sleep unexpectedly failed!"); + // Call updateConfigs() so device config changes take effect immediately + for (PackageWatchdog watchdog : mAllocatedWatchdogs) { + watchdog.updateConfigs(); } } @@ -1348,6 +1351,7 @@ public class PackageWatchdogTest { verify(mConnectivityModuleConnector).registerHealthListener( mConnectivityModuleCallbackCaptor.capture()); } + mAllocatedWatchdogs.add(watchdog); return watchdog; } From 0f9fc342ea8c9d3dc42f142323d682176d7a884d Mon Sep 17 00:00:00 2001 From: JW Wang Date: Fri, 29 Jan 2021 21:25:05 +0800 Subject: [PATCH 5/5] Register and unregister might be out of order (5/n) Calling unregisterHealthObserver() right after startObservingHealth() will fail to unregister observers correctly because startObservingHealth() do registration asynchronously. Let's do unregistration asynchronously as well to keep things in order. Bug: 178675924 Test: atest PackageWatchdogTest Change-Id: I7dfea957fb88f0346cdeea20e626a728ec1a1325 --- .../core/java/com/android/server/PackageWatchdog.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/services/core/java/com/android/server/PackageWatchdog.java b/services/core/java/com/android/server/PackageWatchdog.java index 16bc794dd36ad..41903fcd165f2 100644 --- a/services/core/java/com/android/server/PackageWatchdog.java +++ b/services/core/java/com/android/server/PackageWatchdog.java @@ -372,10 +372,12 @@ public class PackageWatchdog { * even from a previous boot. */ public void unregisterHealthObserver(PackageHealthObserver observer) { - synchronized (mLock) { - mAllObservers.remove(observer.getName()); - } - syncState("unregistering observer: " + observer.getName()); + mLongTaskHandler.post(() -> { + synchronized (mLock) { + mAllObservers.remove(observer.getName()); + } + syncState("unregistering observer: " + observer.getName()); + }); } /**