From f82236ef0ae4ca818a639a5e9deb07bc95feeb28 Mon Sep 17 00:00:00 2001 From: Dianne Hackborn Date: Wed, 19 Apr 2017 10:26:01 -0700 Subject: [PATCH] Fix issue #37306208: CTS: ServiceTest#testForegroundService... ..._removeNotificationOnStop failure We weren't removing the foreground timeout when a service is brought down and still waiting for the app to make it foreground. Instead, turn this in to an immediate crash of the app when we detect it while bringing down the service. Also make various paths more robust against trying to do things with a ServiceRecord that isn't currently in ActiveServices. Test: bit CtsAppTestCases:ServiceTest Change-Id: I06f4ac48fb983483569677ab1ac6e5b0b681c4ae --- .../com/android/server/am/ActiveServices.java | 58 ++++++++++++++++--- .../server/am/ActivityManagerService.java | 4 ++ 2 files changed, 53 insertions(+), 9 deletions(-) diff --git a/services/core/java/com/android/server/am/ActiveServices.java b/services/core/java/com/android/server/am/ActiveServices.java index c77820b4de3ad..a3c691cc9a3de 100644 --- a/services/core/java/com/android/server/am/ActiveServices.java +++ b/services/core/java/com/android/server/am/ActiveServices.java @@ -761,7 +761,7 @@ public final class ActiveServices { } } if (r.fgRequired) { - if (DEBUG_BACKGROUND_CHECK) { + if (DEBUG_SERVICE || DEBUG_BACKGROUND_CHECK) { Slog.i(TAG, "Service called startForeground() as required: " + r); } r.fgRequired = false; @@ -1339,16 +1339,19 @@ public final class ActiveServices { final ComponentName comp = service.getComponent(); if (comp != null) { r = smap.mServicesByName.get(comp); + if (DEBUG_SERVICE && r != null) Slog.v(TAG_SERVICE, "Retrieved by component: " + r); } if (r == null && !isBindExternal) { Intent.FilterComparison filter = new Intent.FilterComparison(service); r = smap.mServicesByIntent.get(filter); + if (DEBUG_SERVICE && r != null) Slog.v(TAG_SERVICE, "Retrieved by intent: " + r); } if (r != null && (r.serviceInfo.flags & ServiceInfo.FLAG_EXTERNAL_SERVICE) != 0 && !callingPackage.equals(r.packageName)) { // If an external service is running within its own package, other packages // should not bind to that instance. r = null; + if (DEBUG_SERVICE) Slog.v(TAG_SERVICE, "Whoops, can't use existing external service"); } if (r == null) { try { @@ -1408,12 +1411,14 @@ public final class ActiveServices { sInfo.applicationInfo = mAm.getAppInfoForUser(sInfo.applicationInfo, userId); } r = smap.mServicesByName.get(name); + if (DEBUG_SERVICE && r != null) Slog.v(TAG_SERVICE, + "Retrieved via pm by intent: " + r); if (r == null && createIfNeeded) { - Intent.FilterComparison filter + final Intent.FilterComparison filter = new Intent.FilterComparison(service.cloneFilter()); - ServiceRestarter res = new ServiceRestarter(); - BatteryStatsImpl.Uid.Pkg.Serv ss = null; - BatteryStatsImpl stats = mAm.mBatteryStatsService.getActiveStatistics(); + final ServiceRestarter res = new ServiceRestarter(); + final BatteryStatsImpl.Uid.Pkg.Serv ss; + final BatteryStatsImpl stats = mAm.mBatteryStatsService.getActiveStatistics(); synchronized (stats) { ss = stats.getServiceStatsLocked( sInfo.applicationInfo.uid, sInfo.packageName, @@ -1426,12 +1431,14 @@ public final class ActiveServices { // Make sure this component isn't in the pending list. for (int i=mPendingServices.size()-1; i>=0; i--) { - ServiceRecord pr = mPendingServices.get(i); + final ServiceRecord pr = mPendingServices.get(i); if (pr.serviceInfo.applicationInfo.uid == sInfo.applicationInfo.uid && pr.name.equals(name)) { + if (DEBUG_SERVICE) Slog.v(TAG_SERVICE, "Remove pending: " + pr); mPendingServices.remove(i); } } + if (DEBUG_SERVICE) Slog.v(TAG_SERVICE, "Retrieve created new service: " + r); } } catch (RemoteException ex) { // pm is in same process, this will never happen. @@ -2119,7 +2126,28 @@ public final class ActiveServices { } } - if (DEBUG_SERVICE) Slog.v(TAG_SERVICE, "Bringing down " + r + " " + r.intent); + // Check to see if the service had been started as foreground, but being + // brought down before actually showing a notification. That is not allowed. + if (r.fgRequired) { + Slog.w(TAG_SERVICE, "Bringing down service while still waiting for start foreground: " + + r); + r.fgRequired = false; + r.fgWaiting = false; + mAm.mHandler.removeMessages( + ActivityManagerService.SERVICE_FOREGROUND_TIMEOUT_MSG, r); + if (r.app != null) { + Message msg = mAm.mHandler.obtainMessage( + ActivityManagerService.SERVICE_FOREGROUND_CRASH_MSG); + msg.obj = r.app; + mAm.mHandler.sendMessage(msg); + } + } + + if (DEBUG_SERVICE) { + RuntimeException here = new RuntimeException(); + here.fillInStackTrace(); + Slog.v(TAG_SERVICE, "Bringing down " + r + " " + r.intent, here); + } r.destroyTime = SystemClock.uptimeMillis(); if (LOG_SERVICE_START_STOP) { EventLogTags.writeAmDestroyService( @@ -2127,7 +2155,14 @@ public final class ActiveServices { } final ServiceMap smap = getServiceMapLocked(r.userId); - smap.mServicesByName.remove(r.name); + ServiceRecord found = smap.mServicesByName.remove(r.name); + if (found != r) { + // This is not actually the service we think is running... this should not happen, + // but if it does, fail hard. + smap.mServicesByName.put(r.name, found); + throw new IllegalStateException("Bringing down " + r + " but actually running " + + found); + } smap.mServicesByIntent.remove(r.intent); r.totalRestartCount = 0; unscheduleServiceRestartLocked(r, 0, true); @@ -2967,7 +3002,7 @@ public final class ActiveServices { void serviceForegroundTimeout(ServiceRecord r) { ProcessRecord app; synchronized (mAm) { - if (!r.fgRequired) { + if (!r.fgRequired || r.destroying) { return; } @@ -2985,6 +3020,11 @@ public final class ActiveServices { } } + void serviceForegroundCrash(ProcessRecord app) { + mAm.crashApplication(app.uid, app.pid, app.info.packageName, app.userId, + "Context.startForegroundService() did not then call Service.startForeground()"); + } + void scheduleServiceTimeoutLocked(ProcessRecord proc) { if (proc.executingServices.size() == 0 || proc.thread == null) { return; diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java index fa936c209099c..be30e4ba47861 100644 --- a/services/core/java/com/android/server/am/ActivityManagerService.java +++ b/services/core/java/com/android/server/am/ActivityManagerService.java @@ -1703,6 +1703,7 @@ public class ActivityManagerService extends IActivityManager.Stub static final int SERVICE_FOREGROUND_TIMEOUT_MSG = 66; static final int DISPATCH_PENDING_INTENT_CANCEL_MSG = 67; static final int PUSH_TEMP_WHITELIST_UI_MSG = 68; + static final int SERVICE_FOREGROUND_CRASH_MSG = 69; static final int START_USER_SWITCH_FG_MSG = 712; static final int FIRST_ACTIVITY_STACK_MSG = 100; @@ -1974,6 +1975,9 @@ public class ActivityManagerService extends IActivityManager.Stub case SERVICE_FOREGROUND_TIMEOUT_MSG: { mServices.serviceForegroundTimeout((ServiceRecord)msg.obj); } break; + case SERVICE_FOREGROUND_CRASH_MSG: { + mServices.serviceForegroundCrash((ProcessRecord)msg.obj); + } break; case DISPATCH_PENDING_INTENT_CANCEL_MSG: { RemoteCallbackList callbacks = (RemoteCallbackList)msg.obj;