From e4d1a2e0daf8a3a6fa6b0f327523ebc381386cde Mon Sep 17 00:00:00 2001 From: Dianne Hackborn Date: Fri, 14 Apr 2017 17:57:33 -0700 Subject: [PATCH] Fix issue #36858643: Runtime restart on OPR1.170323.002 We had a layering problem between alarm manager, device idle controller, and activity manager. The layering should be, from bottom to top: activity manager alarm manager device idle controller. However in the path of activity manager's PendingIntent.send(), it would do a direct call to device idle controller. It was careful to do this without its lock held, but that isn't enough. In this case, alarm manager is doing send() with its lock held, expecting that to be safe, but it ends up causing it to call up in to device idle controller via the activity manager (in order to update the temporary whitelist). To fix this, activity manager now has an internal data structure representing pending temp whitelist requests, and all it does is add to that during the call in, scheduling a message to later dispatch those to device idle controller. But to make things correct, we need to use this data stucture to act like the uid is already on the temp whitelist even before we actually dispatch that message. (So we don't have races with things calling startService() for example.) So all the existing stuff looking at the temp whitelist that is handed down by device idle controller will also consult with this data structure of pending changes. Test: bit CtsAppTestCases:ActivityManagerProcessStateTest Change-Id: Id444b7ad3e694dc977c7f4fa236fbad855ce4066 --- .../server/am/ActivityManagerService.java | 156 +++++++++++++----- .../com/android/server/am/BroadcastQueue.java | 14 +- .../server/am/PendingIntentRecord.java | 24 ++- 3 files changed, 135 insertions(+), 59 deletions(-) diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java index 6b0a73b25e71e..3347b9193722b 100644 --- a/services/core/java/com/android/server/am/ActivityManagerService.java +++ b/services/core/java/com/android/server/am/ActivityManagerService.java @@ -1230,6 +1230,20 @@ public class ActivityManagerService extends IActivityManager.Stub */ int[] mDeviceIdleTempWhitelist = new int[0]; + static final class PendingTempWhitelist { + final int targetUid; + final long duration; + final String tag; + + PendingTempWhitelist(int _targetUid, long _duration, String _tag) { + targetUid = _targetUid; + duration = _duration; + tag = _tag; + } + } + + final SparseArray mPendingTempWhitelist = new SparseArray<>(); + /** * Information about and control over application operations */ @@ -1688,6 +1702,7 @@ public class ActivityManagerService extends IActivityManager.Stub static final int NOTIFY_VR_SLEEPING_MSG = 65; 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 START_USER_SWITCH_FG_MSG = 712; static final int FIRST_ACTIVITY_STACK_MSG = 100; @@ -1921,6 +1936,9 @@ public class ActivityManagerService extends IActivityManager.Stub case DISPATCH_UIDS_CHANGED_UI_MSG: { dispatchUidsChanged(); } break; + case PUSH_TEMP_WHITELIST_UI_MSG: { + pushTempWhitelist(); + } break; } } } @@ -6493,7 +6511,8 @@ public class ActivityManagerService extends IActivityManager.Stub // This is the first appearance of the uid, report it now! if (DEBUG_UID_OBSERVERS) Slog.i(TAG_UID_OBSERVERS, "Creating new process uid: " + uidRec); - if (Arrays.binarySearch(mDeviceIdleTempWhitelist, UserHandle.getAppId(proc.uid)) >= 0) { + if (Arrays.binarySearch(mDeviceIdleTempWhitelist, UserHandle.getAppId(proc.uid)) >= 0 + || mPendingTempWhitelist.indexOfKey(proc.uid) >= 0) { uidRec.setWhitelist = uidRec.curWhitelist = true; } uidRec.updateHasInternetPermission(); @@ -7487,43 +7506,6 @@ public class ActivityManagerService extends IActivityManager.Stub } } - /** - * Whitelists {@code targetUid} to temporarily bypass Power Save mode. - */ - void tempWhitelistAppForPowerSave(int callerPid, int callerUid, int targetUid, long duration) { - if (DEBUG_WHITELISTS) { - Slog.d(TAG, "tempWhitelistAppForPowerSave(" + callerPid + ", " + callerUid + ", " - + targetUid + ", " + duration + ")"); - } - - if (checkPermission(CHANGE_DEVICE_IDLE_TEMP_WHITELIST, callerPid, callerUid) - != PackageManager.PERMISSION_GRANTED) { - synchronized (mPidsSelfLocked) { - final ProcessRecord pr = mPidsSelfLocked.get(callerPid); - if (pr == null) { - Slog.w(TAG, "tempWhitelistAppForPowerSave() no ProcessRecord for pid " - + callerPid); - return; - } - if (!pr.whitelistManager) { - if (DEBUG_WHITELISTS) { - Slog.d(TAG, "tempWhitelistAppForPowerSave() for target " + targetUid - + ": pid " + callerPid + " is not allowed"); - } - return; - } - } - } - - final long token = Binder.clearCallingIdentity(); - try { - mLocalDeviceIdleController.addPowerSaveTempWhitelistAppDirect(targetUid, duration, - true, "pe from uid:" + callerUid); - } finally { - Binder.restoreCallingIdentity(token); - } - } - @Override public void cancelIntentSender(IIntentSender sender) { if (!(sender instanceof PendingIntentRecord)) { @@ -8412,7 +8394,8 @@ public class ActivityManagerService extends IActivityManager.Stub boolean isOnDeviceIdleWhitelistLocked(int uid) { final int appId = UserHandle.getAppId(uid); return Arrays.binarySearch(mDeviceIdleWhitelist, appId) >= 0 - || Arrays.binarySearch(mDeviceIdleTempWhitelist, appId) >= 0; + || Arrays.binarySearch(mDeviceIdleTempWhitelist, appId) >= 0 + || mPendingTempWhitelist.indexOfKey(uid) >= 0; } private ProviderInfo getProviderInfoLocked(String authority, int userHandle, int pmFlags) { @@ -15587,6 +15570,18 @@ public class ActivityManagerService extends IActivityManager.Stub } pw.println(" mDeviceIdleWhitelist=" + Arrays.toString(mDeviceIdleWhitelist)); pw.println(" mDeviceIdleTempWhitelist=" + Arrays.toString(mDeviceIdleTempWhitelist)); + if (mPendingTempWhitelist.size() > 0) { + pw.println(" mPendingTempWhitelist:"); + for (int i = 0; i < mPendingTempWhitelist.size(); i++) { + PendingTempWhitelist ptw = mPendingTempWhitelist.valueAt(i); + pw.print(" "); + UserHandle.formatUid(pw, ptw.targetUid); + pw.print(": "); + TimeUtils.formatDuration(ptw.duration, pw); + pw.print(" "); + pw.println(ptw.tag); + } + } } if (dumpPackage == null) { pw.println(" mWakefulness=" @@ -22699,6 +22694,80 @@ public class ActivityManagerService extends IActivityManager.Stub enqueueUidChangeLocked(uidRec, uid, UidRecord.CHANGE_IDLE); } + /** + * Whitelists {@code targetUid} to temporarily bypass Power Save mode. + */ + void tempWhitelistForPendingIntentLocked(int callerPid, int callerUid, int targetUid, + long duration, String tag) { + if (DEBUG_WHITELISTS) { + Slog.d(TAG, "tempWhitelistForPendingIntentLocked(" + callerPid + ", " + callerUid + ", " + + targetUid + ", " + duration + ")"); + } + + synchronized (mPidsSelfLocked) { + final ProcessRecord pr = mPidsSelfLocked.get(callerPid); + if (pr == null) { + Slog.w(TAG, "tempWhitelistForPendingIntentLocked() no ProcessRecord for pid " + + callerPid); + return; + } + if (!pr.whitelistManager) { + if (checkPermission(CHANGE_DEVICE_IDLE_TEMP_WHITELIST, callerPid, callerUid) + != PackageManager.PERMISSION_GRANTED) { + if (DEBUG_WHITELISTS) { + Slog.d(TAG, "tempWhitelistForPendingIntentLocked() for target " + targetUid + + ": pid " + callerPid + " is not allowed"); + } + return; + } + } + } + + tempWhitelistUidLocked(targetUid, duration, tag); + } + + /** + * Whitelists {@code targetUid} to temporarily bypass Power Save mode. + */ + void tempWhitelistUidLocked(int targetUid, long duration, String tag) { + mPendingTempWhitelist.put(targetUid, new PendingTempWhitelist(targetUid, duration, tag)); + setUidTempWhitelistStateLocked(targetUid, true); + mUiHandler.obtainMessage(PUSH_TEMP_WHITELIST_UI_MSG).sendToTarget(); + } + + void pushTempWhitelist() { + final int N; + final PendingTempWhitelist[] list; + + // First copy out the pending changes... we need to leave them in the map for now, + // in case someone needs to check what is coming up while we don't have the lock held. + synchronized(this) { + N = mPendingTempWhitelist.size(); + list = new PendingTempWhitelist[N]; + for (int i = 0; i < N; i++) { + list[i] = mPendingTempWhitelist.valueAt(i); + } + } + + // Now safely dispatch changes to device idle controller. + for (int i = 0; i < N; i++) { + PendingTempWhitelist ptw = list[i]; + mLocalDeviceIdleController.addPowerSaveTempWhitelistAppDirect(ptw.targetUid, + ptw.duration, true, ptw.tag); + } + + // And now we can safely remove them from the map. + synchronized(this) { + for (int i = 0; i < N; i++) { + PendingTempWhitelist ptw = list[i]; + int index = mPendingTempWhitelist.indexOfKey(ptw.targetUid); + if (index >= 0 && mPendingTempWhitelist.valueAt(index) == ptw) { + mPendingTempWhitelist.removeAt(index); + } + } + } + } + final void setAppIdTempWhitelistStateLocked(int appId, boolean onWhitelist) { boolean changed = false; for (int i=mActiveUids.size()-1; i>=0; i--) { @@ -22713,6 +22782,15 @@ public class ActivityManagerService extends IActivityManager.Stub } } + final void setUidTempWhitelistStateLocked(int uid, boolean onWhitelist) { + boolean changed = false; + final UidRecord uidRec = mActiveUids.get(uid); + if (uidRec != null && uidRec.curWhitelist != onWhitelist) { + uidRec.curWhitelist = onWhitelist; + updateOomAdjLocked(); + } + } + final void trimApplications() { synchronized (this) { int i; diff --git a/services/core/java/com/android/server/am/BroadcastQueue.java b/services/core/java/com/android/server/am/BroadcastQueue.java index baa71d708caad..349180fd28a96 100644 --- a/services/core/java/com/android/server/am/BroadcastQueue.java +++ b/services/core/java/com/android/server/am/BroadcastQueue.java @@ -155,8 +155,6 @@ public final class BroadcastQueue { static final int BROADCAST_INTENT_MSG = ActivityManagerService.FIRST_BROADCAST_QUEUE_MSG; static final int BROADCAST_TIMEOUT_MSG = ActivityManagerService.FIRST_BROADCAST_QUEUE_MSG + 1; - static final int SCHEDULE_TEMP_WHITELIST_MSG - = ActivityManagerService.FIRST_BROADCAST_QUEUE_MSG + 2; final BroadcastHandler mHandler; @@ -178,13 +176,6 @@ public final class BroadcastQueue { broadcastTimeoutLocked(true); } } break; - case SCHEDULE_TEMP_WHITELIST_MSG: { - DeviceIdleController.LocalService dic = mService.mLocalDeviceIdleController; - if (dic != null) { - dic.addPowerSaveTempWhitelistAppDirect(UserHandle.getAppId(msg.arg1), - msg.arg2, true, (String)msg.obj); - } - } break; } } } @@ -789,12 +780,11 @@ public final class BroadcastQueue { if (r.intent.getAction() != null) { b.append(r.intent.getAction()); } else if (r.intent.getComponent() != null) { - b.append(r.intent.getComponent().flattenToShortString()); + r.intent.getComponent().appendShortString(b); } else if (r.intent.getData() != null) { b.append(r.intent.getData()); } - mHandler.obtainMessage(SCHEDULE_TEMP_WHITELIST_MSG, uid, (int)duration, b.toString()) - .sendToTarget(); + mService.tempWhitelistUidLocked(uid, duration, b.toString()); } /** diff --git a/services/core/java/com/android/server/am/PendingIntentRecord.java b/services/core/java/com/android/server/am/PendingIntentRecord.java index c697f2876c653..a580d4bdde5e9 100644 --- a/services/core/java/com/android/server/am/PendingIntentRecord.java +++ b/services/core/java/com/android/server/am/PendingIntentRecord.java @@ -237,14 +237,6 @@ final class PendingIntentRecord extends IIntentSender.Stub { if (intent != null) intent.setDefusable(true); if (options != null) options.setDefusable(true); - if (whitelistDuration > 0 && !canceled) { - // Must call before acquiring the lock. It's possible the method return before sending - // the intent due to some validations inside the lock, in which case the UID shouldn't - // be whitelisted, but since the whitelist is temporary, that would be ok. - owner.tempWhitelistAppForPowerSave(Binder.getCallingPid(), Binder.getCallingUid(), uid, - whitelistDuration); - } - synchronized (owner) { final ActivityContainer activityContainer = (ActivityContainer)container; if (activityContainer != null && activityContainer.mParentActivity != null && @@ -279,6 +271,22 @@ final class PendingIntentRecord extends IIntentSender.Stub { resolvedType = key.requestResolvedType; } + if (whitelistDuration > 0) { + StringBuilder tag = new StringBuilder(64); + tag.append("pendingintent:"); + UserHandle.formatUid(tag, Binder.getCallingUid()); + tag.append(":"); + if (finalIntent.getAction() != null) { + tag.append(finalIntent.getAction()); + } else if (finalIntent.getComponent() != null) { + finalIntent.getComponent().appendShortString(tag); + } else if (finalIntent.getData() != null) { + tag.append(finalIntent.getData()); + } + owner.tempWhitelistForPendingIntentLocked(Binder.getCallingPid(), + Binder.getCallingUid(), uid, whitelistDuration, tag.toString()); + } + final long origId = Binder.clearCallingIdentity(); boolean sendFinish = finishedReceiver != null;