From 20b5e08717b8428a7cbb8b3400c97eff7f181594 Mon Sep 17 00:00:00 2001 From: Jeongsik Mun Date: Mon, 3 May 2021 15:56:16 +0900 Subject: [PATCH] Introduce a new thread in system_server for the PermisisonController FgThread could be blocked by lock contention for a while especially during a new user creation. During this time, PermisisonPolicyService synchronizes permissions and app ops in FgThread. Meanwhile, PermissionController does async things for initialization. Both use PackageManager a lot to get package info, which could result in lock contention and blocking FgThread for a very short duration for each call to PackageManager. It could delay the user creation or switching. This CL adds a new thread for PermisisonPolicyService and PermisisonController to avoid blocking FgThread by lock contention. This is a roll forward of ag/15373851 with a minor fix for the benchmark. synchronizePackagePermissionsAndAppOpsForUser kept on FgThread. It is a heavy operation. Dispatched on a PermissionThread, it interferes with user switch. FgThread is busy and schedules it after most of the switch is done. Bug: 165030092 Test: cts-tradefed run cts -m CtsDevicePolicyManagerTestCases -t \ com.android.cts.devicepolicy.MixedManagedProfileOwnerTest#testPermissionGrant Test: atest UserLifecycleTests#switchUser --iterations Change-Id: Ia035e4ad8f302af7bde5c1c7fd5ac3aaa6642e96 --- .../com/android/server/PermissionThread.java | 94 +++++++++++++++++++ .../OneTimePermissionUserManager.java | 4 +- .../PermissionManagerServiceImpl.java | 11 ++- .../policy/PermissionPolicyService.java | 19 ++-- 4 files changed, 116 insertions(+), 12 deletions(-) create mode 100644 services/core/java/com/android/server/PermissionThread.java diff --git a/services/core/java/com/android/server/PermissionThread.java b/services/core/java/com/android/server/PermissionThread.java new file mode 100644 index 0000000000000..3f747a8cf008c --- /dev/null +++ b/services/core/java/com/android/server/PermissionThread.java @@ -0,0 +1,94 @@ +/* + * Copyright (C) 2021 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.server; + +import android.os.Handler; +import android.os.HandlerExecutor; +import android.os.Looper; +import android.os.Trace; + +import com.android.internal.annotations.GuardedBy; + +import java.util.concurrent.Executor; + +/** + * Shared singleton thread for the system. This is a thread for handling + * calls to and from the PermissionController and handling synchronization + * between permissions and appops states. + */ +public final class PermissionThread extends ServiceThread { + private static final long SLOW_DISPATCH_THRESHOLD_MS = 100; + private static final long SLOW_DELIVERY_THRESHOLD_MS = 200; + + private static final Object sLock = new Object(); + + @GuardedBy("sLock") + private static PermissionThread sInstance; + private static Handler sHandler; + private static HandlerExecutor sHandlerExecutor; + + private PermissionThread() { + super("android.perm", android.os.Process.THREAD_PRIORITY_DEFAULT, /* allowIo= */ true); + } + + @GuardedBy("sLock") + private static void ensureThreadLocked() { + if (sInstance != null) { + return; + } + + sInstance = new PermissionThread(); + sInstance.start(); + final Looper looper = sInstance.getLooper(); + looper.setTraceTag(Trace.TRACE_TAG_SYSTEM_SERVER); + looper.setSlowLogThresholdMs( + SLOW_DISPATCH_THRESHOLD_MS, SLOW_DELIVERY_THRESHOLD_MS); + sHandler = new Handler(sInstance.getLooper()); + sHandlerExecutor = new HandlerExecutor(sHandler); + } + + /** + * Obtain a singleton instance of the PermissionThread. + */ + public static PermissionThread get() { + synchronized (sLock) { + ensureThreadLocked(); + return sInstance; + } + } + + /** + * Obtain a singleton instance of a handler executing in the PermissionThread. + */ + public static Handler getHandler() { + synchronized (sLock) { + ensureThreadLocked(); + return sHandler; + } + } + + + /** + * Obtain a singleton instance of an executor of the PermissionThread. + */ + public static Executor getExecutor() { + synchronized (sLock) { + ensureThreadLocked(); + return sHandlerExecutor; + } + } +} diff --git a/services/core/java/com/android/server/pm/permission/OneTimePermissionUserManager.java b/services/core/java/com/android/server/pm/permission/OneTimePermissionUserManager.java index 881f8707fdd86..661161f05d23a 100644 --- a/services/core/java/com/android/server/pm/permission/OneTimePermissionUserManager.java +++ b/services/core/java/com/android/server/pm/permission/OneTimePermissionUserManager.java @@ -33,6 +33,7 @@ import android.util.Log; import android.util.SparseArray; import com.android.internal.annotations.GuardedBy; +import com.android.server.PermissionThread; /** * Class that handles one-time permissions for a user @@ -79,7 +80,8 @@ public class OneTimePermissionUserManager { mContext = context; mActivityManager = context.getSystemService(ActivityManager.class); mAlarmManager = context.getSystemService(AlarmManager.class); - mPermissionControllerManager = context.getSystemService(PermissionControllerManager.class); + mPermissionControllerManager = new PermissionControllerManager( + mContext, PermissionThread.getHandler()); mHandler = context.getMainThreadHandler(); } diff --git a/services/core/java/com/android/server/pm/permission/PermissionManagerServiceImpl.java b/services/core/java/com/android/server/pm/permission/PermissionManagerServiceImpl.java index ccd906646d604..7d4cfdff2bfa7 100644 --- a/services/core/java/com/android/server/pm/permission/PermissionManagerServiceImpl.java +++ b/services/core/java/com/android/server/pm/permission/PermissionManagerServiceImpl.java @@ -122,6 +122,7 @@ import com.android.internal.util.Preconditions; import com.android.internal.util.function.pooled.PooledLambda; import com.android.server.FgThread; import com.android.server.LocalServices; +import com.android.server.PermissionThread; import com.android.server.ServiceThread; import com.android.server.SystemConfig; import com.android.server.Watchdog; @@ -2004,7 +2005,7 @@ public class PermissionManagerServiceImpl implements PermissionManagerServiceInt Preconditions.checkArgumentNonNegative(userId, "userId"); CompletableFuture backup = new CompletableFuture<>(); mPermissionControllerManager.getRuntimePermissionBackup(UserHandle.of(userId), - mContext.getMainExecutor(), backup::complete); + PermissionThread.getExecutor(), backup::complete); try { return backup.get(BACKUP_TIMEOUT_MILLIS, TimeUnit.MILLISECONDS); @@ -2055,7 +2056,7 @@ public class PermissionManagerServiceImpl implements PermissionManagerServiceInt } } mPermissionControllerManager.applyStagedRuntimePermissionBackup(packageName, - UserHandle.of(userId), mContext.getMainExecutor(), (hasMoreBackup) -> { + UserHandle.of(userId), PermissionThread.getExecutor(), (hasMoreBackup) -> { if (hasMoreBackup) { return; } @@ -4443,9 +4444,9 @@ public class PermissionManagerServiceImpl implements PermissionManagerServiceInt } } - mPermissionControllerManager = mContext.getSystemService(PermissionControllerManager.class); - mPermissionPolicyInternal = LocalServices.getService(PermissionPolicyInternal.class); - } + mPermissionControllerManager = new PermissionControllerManager( + mContext, PermissionThread.getHandler()); + mPermissionPolicyInternal = LocalServices.getService(PermissionPolicyInternal.class); } private static String getVolumeUuidForPackage(AndroidPackage pkg) { if (pkg == null) { diff --git a/services/core/java/com/android/server/policy/PermissionPolicyService.java b/services/core/java/com/android/server/policy/PermissionPolicyService.java index b56e1120f16a0..9c9576984820b 100644 --- a/services/core/java/com/android/server/policy/PermissionPolicyService.java +++ b/services/core/java/com/android/server/policy/PermissionPolicyService.java @@ -92,6 +92,7 @@ import com.android.internal.util.IntPair; import com.android.internal.util.function.pooled.PooledLambda; import com.android.server.FgThread; import com.android.server.LocalServices; +import com.android.server.PermissionThread; import com.android.server.SystemService; import com.android.server.notification.NotificationManagerInternal; import com.android.server.pm.UserManagerInternal; @@ -335,7 +336,7 @@ public final class PermissionPolicyService extends SystemService { PermissionControllerManager manager = mPermControllerManagers.get(user); if (manager == null) { manager = new PermissionControllerManager( - getUserContext(getContext(), user), FgThread.getHandler()); + getUserContext(getContext(), user), PermissionThread.getHandler()); mPermControllerManagers.put(user, manager); } manager.updateUserSensitiveForApp(uid); @@ -343,8 +344,9 @@ public final class PermissionPolicyService extends SystemService { }, UserHandle.ALL, intentFilter, null, null); PermissionControllerManager manager = new PermissionControllerManager( - getUserContext(getContext(), Process.myUserHandle()), FgThread.getHandler()); - FgThread.getHandler().postDelayed(manager::updateUserSensitive, + getUserContext(getContext(), Process.myUserHandle()), + PermissionThread.getHandler()); + PermissionThread.getHandler().postDelayed(manager::updateUserSensitive, USER_SENSITIVE_UPDATE_DELAY_MS); } @@ -371,6 +373,11 @@ public final class PermissionPolicyService extends SystemService { if (isStarted(changedUserId)) { synchronized (mLock) { if (mIsPackageSyncsScheduled.add(new Pair<>(packageName, changedUserId))) { + // TODO(b/165030092): migrate this to PermissionThread.getHandler(). + // synchronizePackagePermissionsAndAppOpsForUser is a heavy operation. + // Dispatched on a PermissionThread, it interferes with user switch. + // FgThread is busy and schedules it after most of the switch is done. + // A possible solution is to delay the callback. FgThread.getHandler().sendMessage(PooledLambda.obtainMessage( PermissionPolicyService ::synchronizePackagePermissionsAndAppOpsForUser, @@ -584,9 +591,9 @@ public final class PermissionPolicyService extends SystemService { final PermissionControllerManager permissionControllerManager = new PermissionControllerManager( getUserContext(getContext(), UserHandle.of(userId)), - FgThread.getHandler()); + PermissionThread.getHandler()); permissionControllerManager.grantOrUpgradeDefaultRuntimePermissions( - FgThread.getExecutor(), successful -> { + PermissionThread.getExecutor(), successful -> { if (successful) { future.complete(null); } else { @@ -690,7 +697,7 @@ public final class PermissionPolicyService extends SystemService { synchronized (mLock) { if (!mIsUidSyncScheduled.get(uid)) { mIsUidSyncScheduled.put(uid, true); - FgThread.getHandler().sendMessage(PooledLambda.obtainMessage( + PermissionThread.getHandler().sendMessage(PooledLambda.obtainMessage( PermissionPolicyService::resetAppOpPermissionsIfNotRequestedForUid, this, uid)); }