From b08b641a334b08c2fffd7af84724700c96c30483 Mon Sep 17 00:00:00 2001 From: Jing Ji Date: Thu, 5 Aug 2021 18:02:21 -0700 Subject: [PATCH] Apply proper locks onto the service/association state tracking As they should be locked with the ProcessStatsService lock. Bug: 194991626 Test: Manual - build & boot Test: atest ProcStatsValidationTests Test: atest ProcessStatsDumpsysTest Test: atest CtsIncidentHostTestCases:ProcStatsProtoTest Test: atest CtsAppTestCases Change-Id: I18ccd3f4509ba0ee00434012ed47abf81da9c907 --- .../com/android/server/am/ActiveServices.java | 162 +++++++++++------- .../android/server/am/ConnectionRecord.java | 19 +- .../server/am/ContentProviderConnection.java | 19 +- .../server/am/ContentProviderRecord.java | 15 +- .../com/android/server/am/OomAdjuster.java | 6 +- 5 files changed, 139 insertions(+), 82 deletions(-) diff --git a/services/core/java/com/android/server/am/ActiveServices.java b/services/core/java/com/android/server/am/ActiveServices.java index 07847f1df053a..ad1d9b16e7e7d 100644 --- a/services/core/java/com/android/server/am/ActiveServices.java +++ b/services/core/java/com/android/server/am/ActiveServices.java @@ -805,10 +805,12 @@ public final class ActiveServices { if (fgRequired) { // We are now effectively running a foreground service. - ServiceState stracker = r.getTracker(); - if (stracker != null) { - stracker.setForeground(true, mAm.mProcessStats.getMemFactorLocked(), - r.lastActivity); + synchronized (mAm.mProcessStats.mLock) { + final ServiceState stracker = r.getTracker(); + if (stracker != null) { + stracker.setForeground(true, mAm.mProcessStats.getMemFactorLocked(), + r.lastActivity); + } } mAm.mAppOpsService.startOperation(AppOpsManager.getToken(mAm.mAppOpsService), AppOpsManager.OP_START_FOREGROUND, r.appInfo.uid, r.packageName, null, @@ -1080,9 +1082,11 @@ public final class ActiveServices { ComponentName startServiceInnerLocked(ServiceMap smap, Intent service, ServiceRecord r, boolean callerFg, boolean addToStarting) throws TransactionTooLargeException { - ServiceState stracker = r.getTracker(); - if (stracker != null) { - stracker.setStarted(true, mAm.mProcessStats.getMemFactorLocked(), r.lastActivity); + synchronized (mAm.mProcessStats.mLock) { + final ServiceState stracker = r.getTracker(); + if (stracker != null) { + stracker.setStarted(true, mAm.mProcessStats.getMemFactorLocked(), r.lastActivity); + } } r.callStart = false; @@ -1142,8 +1146,10 @@ public final class ActiveServices { mAm.mBatteryStatsService.noteServiceStopRunning(uid, packageName, serviceName); service.startRequested = false; if (service.tracker != null) { - service.tracker.setStarted(false, mAm.mProcessStats.getMemFactorLocked(), - SystemClock.uptimeMillis()); + synchronized (mAm.mProcessStats.mLock) { + service.tracker.setStarted(false, mAm.mProcessStats.getMemFactorLocked(), + SystemClock.uptimeMillis()); + } } service.callStart = false; @@ -1318,8 +1324,10 @@ public final class ActiveServices { mAm.mBatteryStatsService.noteServiceStopRunning(uid, packageName, serviceName); r.startRequested = false; if (r.tracker != null) { - r.tracker.setStarted(false, mAm.mProcessStats.getMemFactorLocked(), - SystemClock.uptimeMillis()); + synchronized (mAm.mProcessStats.mLock) { + r.tracker.setStarted(false, mAm.mProcessStats.getMemFactorLocked(), + SystemClock.uptimeMillis()); + } } r.callStart = false; final long origId = Binder.clearCallingIdentity(); @@ -1875,10 +1883,12 @@ public final class ActiveServices { r.mStartForegroundCount++; r.mFgsEnterTime = SystemClock.uptimeMillis(); if (!stopProcStatsOp) { - ServiceState stracker = r.getTracker(); - if (stracker != null) { - stracker.setForeground(true, - mAm.mProcessStats.getMemFactorLocked(), r.lastActivity); + synchronized (mAm.mProcessStats.mLock) { + final ServiceState stracker = r.getTracker(); + if (stracker != null) { + stracker.setForeground(true, + mAm.mProcessStats.getMemFactorLocked(), r.lastActivity); + } } } else { stopProcStatsOp = false; @@ -1913,10 +1923,12 @@ public final class ActiveServices { if (stopProcStatsOp) { // We got through to this point with it actively being started foreground, // and never decided we wanted to keep it like that, so drop it. - ServiceState stracker = r.getTracker(); - if (stracker != null) { - stracker.setForeground(false, mAm.mProcessStats.getMemFactorLocked(), - SystemClock.uptimeMillis()); + synchronized (mAm.mProcessStats.mLock) { + final ServiceState stracker = r.getTracker(); + if (stracker != null) { + stracker.setForeground(false, mAm.mProcessStats.getMemFactorLocked(), + SystemClock.uptimeMillis()); + } } } if (alreadyStartedOp) { @@ -1958,10 +1970,12 @@ public final class ActiveServices { r.isForeground = false; r.mFgsExitTime = SystemClock.uptimeMillis(); - ServiceState stracker = r.getTracker(); - if (stracker != null) { - stracker.setForeground(false, mAm.mProcessStats.getMemFactorLocked(), - SystemClock.uptimeMillis()); + synchronized (mAm.mProcessStats.mLock) { + final ServiceState stracker = r.getTracker(); + if (stracker != null) { + stracker.setForeground(false, mAm.mProcessStats.getMemFactorLocked(), + SystemClock.uptimeMillis()); + } } mAm.mAppOpsService.finishOperation( AppOpsManager.getToken(mAm.mAppOpsService), @@ -2736,10 +2750,12 @@ public final class ActiveServices { s.lastActivity = SystemClock.uptimeMillis(); if (!s.hasAutoCreateConnections()) { // This is the first binding, let the tracker know. - ServiceState stracker = s.getTracker(); - if (stracker != null) { - stracker.setBound(true, mAm.mProcessStats.getMemFactorLocked(), - s.lastActivity); + synchronized (mAm.mProcessStats.mLock) { + final ServiceState stracker = s.getTracker(); + if (stracker != null) { + stracker.setBound(true, mAm.mProcessStats.getMemFactorLocked(), + s.lastActivity); + } } } } @@ -3376,9 +3392,11 @@ public final class ActiveServices { ProcessServiceRecord psr; if (r.executeNesting == 0) { r.executeFg = fg; - ServiceState stracker = r.getTracker(); - if (stracker != null) { - stracker.setExecuting(true, mAm.mProcessStats.getMemFactorLocked(), now); + synchronized (mAm.mProcessStats.mLock) { + final ServiceState stracker = r.getTracker(); + if (stracker != null) { + stracker.setExecuting(true, mAm.mProcessStats.getMemFactorLocked(), now); + } } if (r.app != null) { psr = r.app.mServices; @@ -3573,7 +3591,9 @@ public final class ActiveServices { if (!mRestartingServices.contains(r)) { r.createdFromFg = false; mRestartingServices.add(r); - r.makeRestarting(mAm.mProcessStats.getMemFactorLocked(), now); + synchronized (mAm.mProcessStats.mLock) { + r.makeRestarting(mAm.mProcessStats.getMemFactorLocked(), now); + } } cancelForegroundNotificationLocked(r); @@ -3651,8 +3671,10 @@ public final class ActiveServices { } } if (!stillTracking) { - r.restartTracker.setRestarting(false, mAm.mProcessStats.getMemFactorLocked(), - SystemClock.uptimeMillis()); + synchronized (mAm.mProcessStats.mLock) { + r.restartTracker.setRestarting(false, mAm.mProcessStats.getMemFactorLocked(), + SystemClock.uptimeMillis()); + } r.restartTracker = null; } } @@ -4172,9 +4194,11 @@ public final class ActiveServices { + r); r.fgRequired = false; r.fgWaiting = false; - ServiceState stracker = r.getTracker(); - if (stracker != null) { - stracker.setForeground(false, mAm.mProcessStats.getMemFactorLocked(), now); + synchronized (mAm.mProcessStats.mLock) { + ServiceState stracker = r.getTracker(); + if (stracker != null) { + stracker.setForeground(false, mAm.mProcessStats.getMemFactorLocked(), now); + } } mAm.mAppOpsService.finishOperation(AppOpsManager.getToken(mAm.mAppOpsService), AppOpsManager.OP_START_FOREGROUND, r.appInfo.uid, r.packageName, null); @@ -4231,9 +4255,11 @@ public final class ActiveServices { cancelForegroundNotificationLocked(r); if (r.isForeground) { decActiveForegroundAppLocked(smap, r); - ServiceState stracker = r.getTracker(); - if (stracker != null) { - stracker.setForeground(false, mAm.mProcessStats.getMemFactorLocked(), now); + synchronized (mAm.mProcessStats.mLock) { + ServiceState stracker = r.getTracker(); + if (stracker != null) { + stracker.setForeground(false, mAm.mProcessStats.getMemFactorLocked(), now); + } } mAm.mAppOpsService.finishOperation( AppOpsManager.getToken(mAm.mAppOpsService), @@ -4301,13 +4327,15 @@ public final class ActiveServices { ((ServiceRestarter)r.restarter).setService(null); } - int memFactor = mAm.mProcessStats.getMemFactorLocked(); - if (r.tracker != null) { - r.tracker.setStarted(false, memFactor, now); - r.tracker.setBound(false, memFactor, now); - if (r.executeNesting == 0) { - r.tracker.clearCurrentOwner(r, false); - r.tracker = null; + synchronized (mAm.mProcessStats.mLock) { + final int memFactor = mAm.mProcessStats.getMemFactorLocked(); + if (r.tracker != null) { + r.tracker.setStarted(false, memFactor, now); + r.tracker.setBound(false, memFactor, now); + if (r.executeNesting == 0) { + r.tracker.clearCurrentOwner(r, false); + r.tracker = null; + } } } @@ -4438,8 +4466,10 @@ public final class ActiveServices { boolean hasAutoCreate = s.hasAutoCreateConnections(); if (!hasAutoCreate) { if (s.tracker != null) { - s.tracker.setBound(false, mAm.mProcessStats.getMemFactorLocked(), - SystemClock.uptimeMillis()); + synchronized (mAm.mProcessStats.mLock) { + s.tracker.setBound(false, mAm.mProcessStats.getMemFactorLocked(), + SystemClock.uptimeMillis()); + } } } bringDownServiceIfNeededLocked(s, true, hasAutoCreate, enqueueOomAdj); @@ -4531,12 +4561,14 @@ public final class ActiveServices { private void serviceProcessGoneLocked(ServiceRecord r, boolean enqueueOomAdj) { if (r.tracker != null) { - int memFactor = mAm.mProcessStats.getMemFactorLocked(); - long now = SystemClock.uptimeMillis(); - r.tracker.setExecuting(false, memFactor, now); - r.tracker.setForeground(false, memFactor, now); - r.tracker.setBound(false, memFactor, now); - r.tracker.setStarted(false, memFactor, now); + synchronized (mAm.mProcessStats.mLock) { + final int memFactor = mAm.mProcessStats.getMemFactorLocked(); + final long now = SystemClock.uptimeMillis(); + r.tracker.setExecuting(false, memFactor, now); + r.tracker.setForeground(false, memFactor, now); + r.tracker.setBound(false, memFactor, now); + r.tracker.setStarted(false, memFactor, now); + } } serviceDoneExecutingLocked(r, true, true, enqueueOomAdj); } @@ -4583,13 +4615,15 @@ public final class ActiveServices { } r.executeFg = false; if (r.tracker != null) { - final int memFactor = mAm.mProcessStats.getMemFactorLocked(); - final long now = SystemClock.uptimeMillis(); - r.tracker.setExecuting(false, memFactor, now); - r.tracker.setForeground(false, memFactor, now); - if (finishing) { - r.tracker.clearCurrentOwner(r, false); - r.tracker = null; + synchronized (mAm.mProcessStats.mLock) { + final int memFactor = mAm.mProcessStats.getMemFactorLocked(); + final long now = SystemClock.uptimeMillis(); + r.tracker.setExecuting(false, memFactor, now); + r.tracker.setForeground(false, memFactor, now); + if (finishing) { + r.tracker.clearCurrentOwner(r, false); + r.tracker = null; + } } } if (finishing) { @@ -4974,8 +5008,10 @@ public final class ActiveServices { // down it. sr.startRequested = false; if (sr.tracker != null) { - sr.tracker.setStarted(false, mAm.mProcessStats.getMemFactorLocked(), - SystemClock.uptimeMillis()); + synchronized (mAm.mProcessStats.mLock) { + sr.tracker.setStarted(false, mAm.mProcessStats.getMemFactorLocked(), + SystemClock.uptimeMillis()); + } } } } diff --git a/services/core/java/com/android/server/am/ConnectionRecord.java b/services/core/java/com/android/server/am/ConnectionRecord.java index e6cd509f50dcd..916127126117b 100644 --- a/services/core/java/com/android/server/am/ConnectionRecord.java +++ b/services/core/java/com/android/server/am/ConnectionRecord.java @@ -47,6 +47,7 @@ final class ConnectionRecord { public AssociationState.SourceState association; // Association tracking String stringName; // Caching of toString. boolean serviceDead; // Well is it? + private Object mProcStatsLock; // Internal lock for accessing AssociationState // Please keep the following two enum list synced. private static final int[] BIND_ORIG_ENUMS = new int[] { @@ -137,23 +138,29 @@ final class ConnectionRecord { Slog.wtf(TAG_AM, "Inactive holder in referenced service " + binding.service.shortInstanceName + ": proc=" + binding.service.app); } else { - association = holder.pkg.getAssociationStateLocked(holder.state, - binding.service.instanceName.getClassName()).startSource(clientUid, - clientProcessName, clientPackageName); - + mProcStatsLock = binding.service.app.mService.mProcessStats.mLock; + synchronized (mProcStatsLock) { + association = holder.pkg.getAssociationStateLocked(holder.state, + binding.service.instanceName.getClassName()).startSource(clientUid, + clientProcessName, clientPackageName); + } } } } public void trackProcState(int procState, int seq, long now) { if (association != null) { - association.trackProcState(procState, seq, now); + synchronized (mProcStatsLock) { + association.trackProcState(procState, seq, now); + } } } public void stopAssociation() { if (association != null) { - association.stop(); + synchronized (mProcStatsLock) { + association.stop(); + } association = null; } } diff --git a/services/core/java/com/android/server/am/ContentProviderConnection.java b/services/core/java/com/android/server/am/ContentProviderConnection.java index 3bc4fcf089212..3b9d47d509ad9 100644 --- a/services/core/java/com/android/server/am/ContentProviderConnection.java +++ b/services/core/java/com/android/server/am/ContentProviderConnection.java @@ -38,6 +38,7 @@ public final class ContentProviderConnection extends Binder { public final String clientPackage; public AssociationState.SourceState association; public final long createTime; + private Object mProcStatsLock; // Internal lock for accessing AssociationState /** * Internal lock that guards access to the two counters. @@ -87,23 +88,29 @@ public final class ContentProviderConnection extends Binder { Slog.wtf(TAG_AM, "Inactive holder in referenced provider " + provider.name.toShortString() + ": proc=" + provider.proc); } else { - association = holder.pkg.getAssociationStateLocked(holder.state, - provider.name.getClassName()).startSource(client.uid, client.processName, - clientPackage); - + mProcStatsLock = provider.proc.mService.mProcessStats.mLock; + synchronized (mProcStatsLock) { + association = holder.pkg.getAssociationStateLocked(holder.state, + provider.name.getClassName()).startSource(client.uid, + client.processName, clientPackage); + } } } } public void trackProcState(int procState, int seq, long now) { if (association != null) { - association.trackProcState(procState, seq, now); + synchronized (mProcStatsLock) { + association.trackProcState(procState, seq, now); + } } } public void stopAssociation() { if (association != null) { - association.stop(); + synchronized (mProcStatsLock) { + association.stop(); + } association = null; } } diff --git a/services/core/java/com/android/server/am/ContentProviderRecord.java b/services/core/java/com/android/server/am/ContentProviderRecord.java index 5fd15db79d660..75f31c0ac29f6 100644 --- a/services/core/java/com/android/server/am/ContentProviderRecord.java +++ b/services/core/java/com/android/server/am/ContentProviderRecord.java @@ -321,6 +321,7 @@ final class ContentProviderRecord implements ComponentName.WithComponentName { final String mOwningProcessName; int mAcquisitionCount; AssociationState.SourceState mAssociation; + private Object mProcStatsLock; // Internal lock for accessing AssociationState public ExternalProcessHandle(IBinder token, int owningUid, String owningProcessName) { mToken = token; @@ -353,17 +354,21 @@ final class ContentProviderRecord implements ComponentName.WithComponentName { Slog.wtf(TAG_AM, "Inactive holder in referenced provider " + provider.name.toShortString() + ": proc=" + provider.proc); } else { - mAssociation = holder.pkg.getAssociationStateLocked(holder.state, - provider.name.getClassName()).startSource(mOwningUid, - mOwningProcessName, null); - + mProcStatsLock = provider.proc.mService.mProcessStats.mLock; + synchronized (mProcStatsLock) { + mAssociation = holder.pkg.getAssociationStateLocked(holder.state, + provider.name.getClassName()).startSource(mOwningUid, + mOwningProcessName, null); + } } } } public void stopAssociation() { if (mAssociation != null) { - mAssociation.stop(); + synchronized (mProcStatsLock) { + mAssociation.stop(); + } mAssociation = null; } } diff --git a/services/core/java/com/android/server/am/OomAdjuster.java b/services/core/java/com/android/server/am/OomAdjuster.java index d6bf8dbe0a6ae..5c19ceb7067aa 100644 --- a/services/core/java/com/android/server/am/OomAdjuster.java +++ b/services/core/java/com/android/server/am/OomAdjuster.java @@ -2792,8 +2792,10 @@ public class OomAdjuster { state.setNotCachedSinceIdle(false); } if (!doingAll) { - mService.setProcessTrackerStateLOSP(app, - mService.mProcessStats.getMemFactorLocked(), now); + synchronized (mService.mProcessStats.mLock) { + mService.setProcessTrackerStateLOSP(app, + mService.mProcessStats.getMemFactorLocked(), now); + } } else { state.setProcStateChanged(true); }