From 66c8cb6b3d22046a421e6dc0443dd28b6f46f18b Mon Sep 17 00:00:00 2001
From: Riddle Hsu <riddlehsu@google.com>
Date: Mon, 2 Nov 2020 20:17:37 +0800
Subject: [PATCH] Update oom score reference state if activity state is changed

Previously, the update of reference state may be scheduled in
display thread. That causes a race for the case of activity stopped:
when ActivityTaskManagerService#activityStopped is called, it will
first set STOPPED state and then invoke trimApplications which will
trigger a full oom-adj calculation. But if the scheduled reference
state is updated later than the calculation, the adj and process
state of the stopped activity may stay at a stale perceptible state
temporally. That seems somewhat inconsistent between ATMS and AMS.

This change ensures that any activity states change applies to the
oom reference state immediately. So any direct or posted oom-adj
calculation can refer to the latest state.

Also correct the place of setting activity state flags that should
be outside of the loop.

Bug: 159104503
Bug: 170685888
Test: ActivityLifecycleTests#testRestoreFromKill
      WindowProcessControllerTests#testComputeOomAdjFromActivities
Change-Id: I42bd43f71315dbf8f5d6abc7ab2f742419f0b6da
---
 .../com/android/server/wm/ActivityRecord.java |  2 +-
 .../server/wm/RootWindowContainer.java        |  3 +-
 .../server/wm/WindowProcessController.java    | 98 ++++++-------------
 .../wm/WindowProcessControllerTests.java      |  6 --
 4 files changed, 34 insertions(+), 75 deletions(-)

diff --git a/services/core/java/com/android/server/wm/ActivityRecord.java b/services/core/java/com/android/server/wm/ActivityRecord.java
index 0a7f08bfbe8cb..f65a5024bdea9 100644
--- a/services/core/java/com/android/server/wm/ActivityRecord.java
+++ b/services/core/java/com/android/server/wm/ActivityRecord.java
@@ -4489,7 +4489,7 @@ final class ActivityRecord extends WindowToken implements WindowManagerService.A
             detachChildren();
         }
         if (app != null) {
-            app.invalidateOomScoreReferenceState(false /* computeNow */);
+            app.computeProcessActivityState();
         }
 
         switch (state) {
diff --git a/services/core/java/com/android/server/wm/RootWindowContainer.java b/services/core/java/com/android/server/wm/RootWindowContainer.java
index 04b303053d8f2..757c57f94a368 100644
--- a/services/core/java/com/android/server/wm/RootWindowContainer.java
+++ b/services/core/java/com/android/server/wm/RootWindowContainer.java
@@ -2728,8 +2728,7 @@ class RootWindowContainer extends WindowContainer<DisplayContent>
         }, true /* traverseTopToBottom */);
 
         for (int i = mTmpTaskLayerChangedProcs.size() - 1; i >= 0; i--) {
-            mTmpTaskLayerChangedProcs.valueAt(i).invalidateOomScoreReferenceState(
-                    true /* computeNow */);
+            mTmpTaskLayerChangedProcs.valueAt(i).computeProcessActivityState();
         }
         mTmpTaskLayerChangedProcs.clear();
     }
diff --git a/services/core/java/com/android/server/wm/WindowProcessController.java b/services/core/java/com/android/server/wm/WindowProcessController.java
index 2e7905c64049e..5bfa662ea4cb4 100644
--- a/services/core/java/com/android/server/wm/WindowProcessController.java
+++ b/services/core/java/com/android/server/wm/WindowProcessController.java
@@ -225,8 +225,19 @@ public class WindowProcessController extends ConfigurationContainer<Configuratio
     @Nullable
     private final BackgroundActivityStartCallback mBackgroundActivityStartCallback;
 
-    /** The state for oom-adjustment calculation. */
-    private final OomScoreReferenceState mOomRefState;
+    // The bits used for mActivityStateFlags.
+    private static final int ACTIVITY_STATE_FLAG_IS_VISIBLE = 0x10000000;
+    private static final int ACTIVITY_STATE_FLAG_IS_PAUSING_OR_PAUSED = 0x20000000;
+    private static final int ACTIVITY_STATE_FLAG_IS_STOPPING = 0x40000000;
+    private static final int ACTIVITY_STATE_FLAG_IS_STOPPING_FINISHING = 0x80000000;
+    private static final int ACTIVITY_STATE_FLAG_MASK_MIN_TASK_LAYER = 0x0000ffff;
+
+    /**
+     * The state for oom-adjustment calculation. The higher 16 bits are the activity states, and the
+     * lower 16 bits are the task layer rank (see {@link Task#mLayerRank}). This field is written by
+     * window manager and read by activity manager.
+     */
+    private volatile int mActivityStateFlags = ACTIVITY_STATE_FLAG_MASK_MIN_TASK_LAYER;
 
     public WindowProcessController(@NonNull ActivityTaskManagerService atm, ApplicationInfo info,
             String name, int uid, int userId, Object owner,
@@ -240,7 +251,6 @@ public class WindowProcessController extends ConfigurationContainer<Configuratio
         mAtm = atm;
         mDisplayId = INVALID_DISPLAY;
         mBackgroundActivityStartCallback = mAtm.getBackgroundActivityStartCallback();
-        mOomRefState = new OomScoreReferenceState(this);
 
         boolean isSysUiPackage = info.packageName.equals(
                 mAtm.getSysUiServiceComponentLocked().getPackageName());
@@ -707,7 +717,7 @@ public class WindowProcessController extends ConfigurationContainer<Configuratio
 
     @HotPath(caller = HotPath.OOM_ADJUSTMENT)
     public boolean hasVisibleActivities() {
-        return (mOomRefState.mActivityStateFlags & OomScoreReferenceState.FLAG_IS_VISIBLE) != 0;
+        return (mActivityStateFlags & ACTIVITY_STATE_FLAG_IS_VISIBLE) != 0;
     }
 
     @HotPath(caller = HotPath.LRU_UPDATE)
@@ -1000,34 +1010,6 @@ public class WindowProcessController extends ConfigurationContainer<Configuratio
         mHostActivities.remove(r);
     }
 
-    private static class OomScoreReferenceState extends RootWindowContainer.LockedScheduler {
-        private static final int FLAG_IS_VISIBLE = 0x10000000;
-        private static final int FLAG_IS_PAUSING = 0x20000000;
-        private static final int FLAG_IS_STOPPING = 0x40000000;
-        private static final int FLAG_IS_STOPPING_FINISHING = 0x80000000;
-        /** @see Task#mLayerRank */
-        private static final int MASK_MIN_TASK_LAYER = 0x0000ffff;
-
-        private final WindowProcessController mOwner;
-        boolean mChanged;
-
-        /**
-         * The higher 16 bits are the activity states, and the lower 16 bits are the task layer
-         * rank. This field is written by window manager and read by activity manager.
-         */
-        volatile int mActivityStateFlags = MASK_MIN_TASK_LAYER;
-
-        OomScoreReferenceState(WindowProcessController owner) {
-            super(owner.mAtm);
-            mOwner = owner;
-        }
-
-        @Override
-        public void execute() {
-            mOwner.computeOomScoreReferenceStateIfNeeded();
-        }
-    }
-
     public interface ComputeOomAdjCallback {
         void onVisibleActivity();
         void onPausedActivity();
@@ -1041,26 +1023,21 @@ public class WindowProcessController extends ConfigurationContainer<Configuratio
      */
     @HotPath(caller = HotPath.OOM_ADJUSTMENT)
     public int computeOomAdjFromActivities(ComputeOomAdjCallback callback) {
-        final int flags = mOomRefState.mActivityStateFlags;
-        if ((flags & OomScoreReferenceState.FLAG_IS_VISIBLE) != 0) {
+        final int flags = mActivityStateFlags;
+        if ((flags & ACTIVITY_STATE_FLAG_IS_VISIBLE) != 0) {
             callback.onVisibleActivity();
-        } else if ((flags & OomScoreReferenceState.FLAG_IS_PAUSING) != 0) {
+        } else if ((flags & ACTIVITY_STATE_FLAG_IS_PAUSING_OR_PAUSED) != 0) {
             callback.onPausedActivity();
-        } else if ((flags & OomScoreReferenceState.FLAG_IS_STOPPING) != 0) {
+        } else if ((flags & ACTIVITY_STATE_FLAG_IS_STOPPING) != 0) {
             callback.onStoppingActivity(
-                    (flags & OomScoreReferenceState.FLAG_IS_STOPPING_FINISHING) != 0);
+                    (flags & ACTIVITY_STATE_FLAG_IS_STOPPING_FINISHING) != 0);
         } else {
             callback.onOtherActivity();
         }
-        return flags & OomScoreReferenceState.MASK_MIN_TASK_LAYER;
+        return flags & ACTIVITY_STATE_FLAG_MASK_MIN_TASK_LAYER;
     }
 
-    void computeOomScoreReferenceStateIfNeeded() {
-        if (!mOomRefState.mChanged) {
-            return;
-        }
-        mOomRefState.mChanged = false;
-
+    void computeProcessActivityState() {
         // Since there could be more than one activities in a process record, we don't need to
         // compute the OomAdj with each of them, just need to find out the activity with the
         // "best" state, the order would be visible, pausing, stopping...
@@ -1101,36 +1078,25 @@ public class WindowProcessController extends ConfigurationContainer<Configuratio
                     finishing &= r.finishing;
                 }
             }
+        }
 
-            int stateFlags = minTaskLayer & OomScoreReferenceState.MASK_MIN_TASK_LAYER;
-            if (visible) {
-                stateFlags |= OomScoreReferenceState.FLAG_IS_VISIBLE;
-            } else if (best == PAUSING) {
-                stateFlags |= OomScoreReferenceState.FLAG_IS_PAUSING;
-            } else if (best == STOPPING) {
-                stateFlags |= OomScoreReferenceState.FLAG_IS_STOPPING;
-                if (finishing) {
-                    stateFlags |= OomScoreReferenceState.FLAG_IS_STOPPING_FINISHING;
-                }
+        int stateFlags = minTaskLayer & ACTIVITY_STATE_FLAG_MASK_MIN_TASK_LAYER;
+        if (visible) {
+            stateFlags |= ACTIVITY_STATE_FLAG_IS_VISIBLE;
+        } else if (best == PAUSING) {
+            stateFlags |= ACTIVITY_STATE_FLAG_IS_PAUSING_OR_PAUSED;
+        } else if (best == STOPPING) {
+            stateFlags |= ACTIVITY_STATE_FLAG_IS_STOPPING;
+            if (finishing) {
+                stateFlags |= ACTIVITY_STATE_FLAG_IS_STOPPING_FINISHING;
             }
-            mOomRefState.mActivityStateFlags = stateFlags;
         }
-    }
-
-    void invalidateOomScoreReferenceState(boolean computeNow) {
-        mOomRefState.mChanged = true;
-        if (computeNow) {
-            computeOomScoreReferenceStateIfNeeded();
-            return;
-        }
-        mOomRefState.scheduleIfNeeded();
+        mActivityStateFlags = stateFlags;
     }
 
     /** Called when the process has some oom related changes and it is going to update oom-adj. */
     private void prepareOomAdjustment() {
         mAtm.mRootWindowContainer.rankTaskLayersIfNeeded();
-        // The task layer may not change but the activity state in the same task may change.
-        computeOomScoreReferenceStateIfNeeded();
     }
 
     public int computeRelaunchReason() {
diff --git a/services/tests/wmtests/src/com/android/server/wm/WindowProcessControllerTests.java b/services/tests/wmtests/src/com/android/server/wm/WindowProcessControllerTests.java
index 94bbfca012b0c..21be6ef2e8973 100644
--- a/services/tests/wmtests/src/com/android/server/wm/WindowProcessControllerTests.java
+++ b/services/tests/wmtests/src/com/android/server/wm/WindowProcessControllerTests.java
@@ -291,25 +291,19 @@ public class WindowProcessControllerTests extends WindowTestsBase {
         assertEquals(1 /* minTaskLayer */, mWpc.computeOomAdjFromActivities(callback));
         assertEquals(visible, callbackResult[0]);
 
-        // The oom state will be updated in handler from activity state change.
         callbackResult[0] = 0;
         activity.mVisibleRequested = false;
         activity.setState(Task.ActivityState.PAUSED, "test");
-        waitHandlerIdle(mAtm.mH);
         mWpc.computeOomAdjFromActivities(callback);
         assertEquals(paused, callbackResult[0]);
 
-        // updateProcessInfo with updateOomAdj=true should refresh the state immediately.
         callbackResult[0] = 0;
         activity.setState(Task.ActivityState.STOPPING, "test");
-        mWpc.updateProcessInfo(false /* updateServiceConnectionActivities */,
-                true /* activityChange */, true /* updateOomAdj */, false /* addPendingTopUid */);
         mWpc.computeOomAdjFromActivities(callback);
         assertEquals(stopping, callbackResult[0]);
 
         callbackResult[0] = 0;
         activity.setState(Task.ActivityState.STOPPED, "test");
-        waitHandlerIdle(mAtm.mH);
         mWpc.computeOomAdjFromActivities(callback);
         assertEquals(other, callbackResult[0]);
     }