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

diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java
index 6b0a73b..3347b91 100644 (file)
--- 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<PendingTempWhitelist> 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 baa71d7..349180f 100644 (file)
--- 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 c697f28..a580d4b 100644 (file)
--- 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;