From beef3189eaa76c7675b89ba5dc50bb8176603a72 Mon Sep 17 00:00:00 2001
From: Zim <zezeozue@google.com>
Date: Tue, 1 Aug 2023 19:13:43 +0100
Subject: [PATCH] Defer service requests if the app hasn't finished startup

If an app is still in the 'pending finish attach' state, we should
not schedule any service requests to the app because:
1. Pending requests will be dispatched as soon as its startup completes
2. Even if we dispatch a request, it doesn't actually expedite the request
because it will be blocked on the apps main thread which is currently
busy starting up.
3. Lastly and most importantly, this could lead to multiple onCreate requests
to the same service when the app completes startup if there were multiple bind
requests prior to its startup.

This is easy to repro with a test app that sleeps in the Application#onCreate
while a bindService is sent multiple times during the sleep.

Test: Manual (CTS pending)
Bug: 289688173
Change-Id: Iedaa62354418428a98b612067dd6439daf9dd530
---
 .../java/com/android/server/am/ActiveServices.java | 14 +++++++-------
 .../java/com/android/server/am/ProcessRecord.java  |  5 +++++
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/services/core/java/com/android/server/am/ActiveServices.java b/services/core/java/com/android/server/am/ActiveServices.java
index d959de33d3e9e..26f2373c44aa5 100644
--- a/services/core/java/com/android/server/am/ActiveServices.java
+++ b/services/core/java/com/android/server/am/ActiveServices.java
@@ -5067,7 +5067,7 @@ public final class ActiveServices {
             boolean whileRestarting, boolean permissionsReviewRequired, boolean packageFrozen,
             boolean enqueueOomAdj)
             throws TransactionTooLargeException {
-        if (r.app != null && r.app.getThread() != null) {
+        if (r.app != null && r.app.isThreadReady()) {
             sendServiceArgsLocked(r, execInFg, false);
             return null;
         }
@@ -5139,7 +5139,7 @@ public final class ActiveServices {
                 final IApplicationThread thread = app.getThread();
                 final int pid = app.getPid();
                 final UidRecord uidRecord = app.getUidRecord();
-                if (thread != null) {
+                if (app.isThreadReady()) {
                     try {
                         if (Trace.isTagEnabled(Trace.TRACE_TAG_ACTIVITY_MANAGER)) {
                             Trace.traceBegin(Trace.TRACE_TAG_ACTIVITY_MANAGER,
@@ -5171,7 +5171,7 @@ public final class ActiveServices {
                     final int pid = app.getPid();
                     final UidRecord uidRecord = app.getUidRecord();
                     r.isolationHostProc = app;
-                    if (thread != null) {
+                    if (app.isThreadReady()) {
                         try {
                             if (Trace.isTagEnabled(Trace.TRACE_TAG_ACTIVITY_MANAGER)) {
                                 Trace.traceBegin(Trace.TRACE_TAG_ACTIVITY_MANAGER,
@@ -5571,7 +5571,7 @@ public final class ActiveServices {
 
         boolean oomAdjusted = false;
         // Tell the service that it has been unbound.
-        if (r.app != null && r.app.getThread() != null) {
+        if (r.app != null && r.app.isThreadReady()) {
             for (int i = r.bindings.size() - 1; i >= 0; i--) {
                 IntentBindRecord ibr = r.bindings.valueAt(i);
                 if (DEBUG_SERVICE) Slog.v(TAG_SERVICE, "Bringing down binding " + ibr
@@ -5713,7 +5713,7 @@ public final class ActiveServices {
             mAm.mBatteryStatsService.noteServiceStopLaunch(r.appInfo.uid, r.name.getPackageName(),
                     r.name.getClassName());
             stopServiceAndUpdateAllowlistManagerLocked(r);
-            if (r.app.getThread() != null) {
+            if (r.app.isThreadReady()) {
                 // Bump the process to the top of LRU list
                 mAm.updateLruProcessLocked(r.app, false, null);
                 updateServiceForegroundLocked(r.app.mServices, false);
@@ -5877,7 +5877,7 @@ public final class ActiveServices {
         if (!c.serviceDead) {
             if (DEBUG_SERVICE) Slog.v(TAG_SERVICE, "Disconnecting binding " + b.intent
                     + ": shouldUnbind=" + b.intent.hasBound);
-            if (s.app != null && s.app.getThread() != null && b.intent.apps.size() == 0
+            if (s.app != null && s.app.isThreadReady() && b.intent.apps.size() == 0
                     && b.intent.hasBound) {
                 try {
                     bumpServiceExecutingLocked(s, false, "unbind", OOM_ADJ_REASON_UNBIND_SERVICE);
@@ -6379,7 +6379,7 @@ public final class ActiveServices {
                     sr.pendingStarts.add(new ServiceRecord.StartItem(sr, true,
                             sr.getLastStartId(), baseIntent, null, 0, null, null,
                             ActivityManager.PROCESS_STATE_UNKNOWN));
-                    if (sr.app != null && sr.app.getThread() != null) {
+                    if (sr.app != null && sr.app.isThreadReady()) {
                         // We always run in the foreground, since this is called as
                         // part of the "remove task" UI operation.
                         try {
diff --git a/services/core/java/com/android/server/am/ProcessRecord.java b/services/core/java/com/android/server/am/ProcessRecord.java
index f532122c10d93..f6acc41e4df35 100644
--- a/services/core/java/com/android/server/am/ProcessRecord.java
+++ b/services/core/java/com/android/server/am/ProcessRecord.java
@@ -769,6 +769,11 @@ class ProcessRecord implements WindowProcessListener {
         return mPendingFinishAttach;
     }
 
+    @GuardedBy("mService")
+    boolean isThreadReady() {
+        return mThread != null && !mPendingFinishAttach;
+    }
+
     @GuardedBy("mService")
     long getStartSeq() {
         return mStartSeq;