Removed unnecessarily call to unlock system user.

When CarServiceHelper boots, it needs to unlock the user 0 (system), which it currently does by
making 2 calls to IActivityManager: startUserInBackground(0, false), then unlockUser(0, null, null, null)
(the nulls in the second call mean no credentials).

But the second call is redundant, as UserController's startUserInternal() already tries to unlock
the user (using null credentials) at the end - although this second call is harmless right now (other than
making an unnecessary IPC call), it makes it harder to refactor the user boot workflow.

This change also refactored the logic to unlock the system user in a new method, which not only is
clear (no nested if statements), but makes it easier to trace the underlying calls.

Bug: 133242016
Bug: 128904478
Bug: 132111956

Test: manual verification # it boots!
Test: atest CarServiceTest

Merged-In: I52f1303471ad34ea1ab3b7d44a3be42907f7350a
Change-Id: I52f1303471ad34ea1ab3b7d44a3be42907f7350a
(cherry picked from commit 33bc18fb39e0a54884198339977ad3fb6aa0030b)

1 files changed, 30 insertions, 24 deletions

diff --git a/src/com/android/internal/car/CarServiceHelperService.java b/src/com/android/internal/car/CarServiceHelperService.java
index f8842d4..160d248 100644
--- a/src/com/android/internal/car/CarServiceHelperService.java
+++ b/src/com/android/internal/car/CarServiceHelperService.java
@@ -16,6 +16,7 @@
 
 package com.android.internal.car;
 
+import android.annotation.NonNull;
 import android.app.ActivityManager;
 import android.app.IActivityManager;
 import android.app.admin.DevicePolicyManager;
@@ -275,45 +276,50 @@ public class CarServiceHelperService extends SystemService {
             Slog.wtf(TAG, "cannot get ActivityManagerService");
             return;
         }
-        TimingsTraceLog traceLog = new TimingsTraceLog("SystemServerTiming",
-                Trace.TRACE_TAG_SYSTEM_SERVER);
-        traceLog.traceBegin("User0Unlock");
+        TimingsTraceLog t = new TimingsTraceLog(TAG, Trace.TRACE_TAG_SYSTEM_SERVER);
+        unlockSystemUser(t, am);
+
+        t.traceBegin("ForegroundUserStart" + targetUserId);
         try {
-            // This is for force changing state into RUNNING_LOCKED. Otherwise unlock does not
-            // update the state and user 0 unlock happens twice.
-            if (!am.startUserInBackground(UserHandle.USER_SYSTEM)) {
-                // cannot start user
-                Slog.w(TAG, "cannot start system user");
-            } else if (!am.unlockUser(UserHandle.USER_SYSTEM, null, null, null)) {
-                // unlocking system user failed. But still continue for other setup.
-                Slog.w(TAG, "cannot unlock system user");
+            if (!am.startUserInForegroundWithListener(targetUserId, null)) {
+                Slog.e(TAG, "cannot start foreground user:" + targetUserId);
             } else {
-                // user 0 started and unlocked
-                handleUserLockStatusChange(UserHandle.USER_SYSTEM, true);
+                mCarUserManagerHelper.setLastActiveUser(targetUserId);
             }
         } catch (RemoteException e) {
             // should not happen for local call.
             Slog.wtf("RemoteException from AMS", e);
         }
-        traceLog.traceEnd();
-        // Do not unlock here to allow other stuffs done. Unlock will happen
-        // when system completes the boot.
-        // TODO(b/124460424) Unlock earlier?
-        traceLog.traceBegin("ForegroundUserStart");
+        t.traceEnd();
+    }
+
+    private void unlockSystemUser(@NonNull TimingsTraceLog t, @NonNull IActivityManager am) {
+        t.traceBegin("UnlockSystemUser");
         try {
-            if (!am.startUserInForegroundWithListener(targetUserId, null)) {
-                Slog.e(TAG, "cannot start foreground user:" + targetUserId);
-            } else {
-                mCarUserManagerHelper.setLastActiveUser(targetUserId);
+            // This is for force changing state into RUNNING_LOCKED. Otherwise unlock does not
+            // update the state and user 0 unlock happens twice.
+            boolean started = am.startUserInBackground(UserHandle.USER_SYSTEM);
+            if (!started) {
+                Slog.w(TAG, "could not restart system user in foreground; trying unlock instead");
+                t.traceBegin("forceUnlockSystemUser");
+                boolean unlocked = am.unlockUser(UserHandle.USER_SYSTEM,
+                        /* token= */ null, /* secret= */ null, /* listner= */ null);
+                t.traceEnd();
+                if (!unlocked) {
+                    Slog.w(TAG, "could not unlock system user neither");
+                    return;
+                }
             }
+            // System user started and unlocked
+            handleUserLockStatusChange(UserHandle.USER_SYSTEM, true);
         } catch (RemoteException e) {
             // should not happen for local call.
             Slog.wtf("RemoteException from AMS", e);
+        } finally {
+            t.traceEnd();
         }
-        traceLog.traceEnd();
     }
 
-
     private void notifyAllUnlockedUsers() {
         // only care about unlocked users
         LinkedList<Integer> users = new LinkedList<>();