Fix privacy indicators leak (i.e., active op leak)
This change fixes an intermittent bug where privacy indicator icons will
sometimes stay on, even after the last app actively using the appop
(e.g., camera, microphone, or location) terminates.
Background:
We begin tracking that an app is actively using one of these appops as
soon as startOp (or another startOp-like method) is called. We stop
tracking whenever one of the following triggers occur (whichever
happens first):
1. finishOp (or another finishOp-like method) is explicity called)
(invoking AppOpsService::finishOperation)
2. The IBinder token, a required arg to startOp, dies
(invoking AppOpsService::onClientDeath)
Both triggers end up calling the following chain of functions, which
results in the privacy indicator visibly disappearing:
AttributedOp::finished
-> AttributedOp::finishOrPaused,
-> AppOpsService:scheduleOpActiveChangedIfNeededLocked
-> ...
-> (indicator visibly disappears)
The problem:
The problem occurs inside another function,
AttributedOp::onUidStateChanged (triggered whenever a currently-
active-op's uid changes uid state) performs the unique behavior of
"restarting" an active op, which involves finishing the active op
(AttributedOp::finished) and then starting it again
(AttributedOp::startedOrPaused).
In order for onUidStateChanged to perform this restart functionality
without causing chaos, when it finishes an active-op, it passes a flag
to indicate that active watchers should not be notified of this
operation (triggerCallbackIfNeeded=false).
The problem is that active ops are keyed by live IBinder sessions.
And, a binder session can die at any time. So, if the binder dies during
the brief moment in AttributedOp::onUidStateChanged between the
finished() and startedOrPaused() calls, then, startedOrPaused() will
throw a DeadObjectException, because it needs to interact with the
binder (to register an binder.onClientDeath callback). In this case, the
active op will be left finished, but it never actually notified any
watchers. To the user, the state of the indicator appears to have
leaked.
The solution:
When a DeadObjectException is thrown from startedOrPaused(),
notify the watchers that the active op is finished. Also, rename the
triggerCallbackIfNeeded argument to reduce the risk of future users
repeating this mistake.
Fix: 290091092
Test: cd packages/modules/Permission && atest :alltests
(cherry picked from https://googleplex-android-review.googlesource.com/q/commit:5d7e9f78bbaa02f98ac5edb36da10404de23f305)
Merged-In: I48f5577f7c611eccdd803a2a9c32790362a329ea
Change-Id: I48f5577f7c611eccdd803a2a9c32790362a329ea
diff --git a/services/core/java/com/android/server/appop/AttributedOp.java b/services/core/java/com/android/server/appop/AttributedOp.java
index dcc36bc..0ded75a 100644
--- a/services/core/java/com/android/server/appop/AttributedOp.java
+++ b/services/core/java/com/android/server/appop/AttributedOp.java
@@ -199,28 +199,18 @@
@AppOpsManager.UidState int uidState, @AppOpsManager.OpFlags int flags,
@AppOpsManager.AttributionFlags
int attributionFlags, int attributionChainId) throws RemoteException {
- started(clientId, proxyUid, proxyPackageName, proxyAttributionTag,
- uidState, flags, /*triggerCallbackIfNeeded*/ true, attributionFlags,
- attributionChainId);
- }
-
- private void started(@NonNull IBinder clientId, int proxyUid,
- @Nullable String proxyPackageName, @Nullable String proxyAttributionTag,
- @AppOpsManager.UidState int uidState, @AppOpsManager.OpFlags int flags,
- boolean triggerCallbackIfNeeded, @AppOpsManager.AttributionFlags int attributionFlags,
- int attributionChainId) throws RemoteException {
startedOrPaused(clientId, proxyUid, proxyPackageName,
- proxyAttributionTag, uidState, flags, triggerCallbackIfNeeded,
- /*triggerCallbackIfNeeded*/ true, attributionFlags, attributionChainId);
+ proxyAttributionTag, uidState, flags, /* triggeredByUidStateChange */ false,
+ /* isStarted */ true, attributionFlags, attributionChainId);
}
@SuppressWarnings("GuardedBy") // Lock is held on mAppOpsService
private void startedOrPaused(@NonNull IBinder clientId, int proxyUid,
@Nullable String proxyPackageName, @Nullable String proxyAttributionTag,
@AppOpsManager.UidState int uidState, @AppOpsManager.OpFlags int flags,
- boolean triggerCallbackIfNeeded, boolean isStarted, @AppOpsManager.AttributionFlags
+ boolean triggeredByUidStateChange, boolean isStarted, @AppOpsManager.AttributionFlags
int attributionFlags, int attributionChainId) throws RemoteException {
- if (triggerCallbackIfNeeded && !parent.isRunning() && isStarted) {
+ if (!triggeredByUidStateChange && !parent.isRunning() && isStarted) {
mAppOpsService.scheduleOpActiveChangedIfNeededLocked(parent.op, parent.uid,
parent.packageName, tag, true, attributionFlags, attributionChainId);
}
@@ -263,19 +253,27 @@
* @param clientId Id of the finishOp caller
*/
public void finished(@NonNull IBinder clientId) {
- finished(clientId, true);
+ finished(clientId, false);
}
- private void finished(@NonNull IBinder clientId, boolean triggerCallbackIfNeeded) {
- finishOrPause(clientId, triggerCallbackIfNeeded, false);
+ private void finished(@NonNull IBinder clientId, boolean triggeredByUidStateChange) {
+ finishOrPause(clientId, triggeredByUidStateChange, false);
}
/**
* Update state when paused or finished is called. If pausing, it records the op as
* stopping in the HistoricalRegistry, but does not delete it.
+ *
+ * @param triggeredByUidStateChange If {@code true}, then this method operates as usual, except
+ * that {@link AppOpsService#mActiveWatchers} will not be notified. This is currently only
+ * used in {@link #onUidStateChanged(int)}, for the purpose of restarting (i.e.,
+ * finishing then immediately starting again in the new uid state) the AttributedOp. In this
+ * case, the caller is responsible for guaranteeing that either the AttributedOp is started
+ * again or all {@link AppOpsService#mActiveWatchers} are notified that the AttributedOp is
+ * finished.
*/
@SuppressWarnings("GuardedBy") // Lock is held on mAppOpsService
- private void finishOrPause(@NonNull IBinder clientId, boolean triggerCallbackIfNeeded,
+ private void finishOrPause(@NonNull IBinder clientId, boolean triggeredByUidStateChange,
boolean isPausing) {
int indexOfToken = isRunning() ? mInProgressEvents.indexOfKey(clientId) : -1;
if (indexOfToken < 0) {
@@ -320,7 +318,7 @@
mInProgressEvents = null;
// TODO ntmyren: Also callback for single attribution tag activity changes
- if (triggerCallbackIfNeeded && !parent.isRunning()) {
+ if (!triggeredByUidStateChange && !parent.isRunning()) {
mAppOpsService.scheduleOpActiveChangedIfNeededLocked(parent.op,
parent.uid, parent.packageName, tag, false,
event.getAttributionFlags(), event.getAttributionChainId());
@@ -368,7 +366,7 @@
@AppOpsManager.AttributionFlags
int attributionFlags, int attributionChainId) throws RemoteException {
startedOrPaused(clientId, proxyUid, proxyPackageName, proxyAttributionTag,
- uidState, flags, true, false, attributionFlags, attributionChainId);
+ uidState, flags, false, false, attributionFlags, attributionChainId);
}
/**
@@ -386,7 +384,7 @@
for (int i = 0; i < mInProgressEvents.size(); i++) {
InProgressStartOpEvent event = mInProgressEvents.valueAt(i);
mPausedInProgressEvents.put(event.getClientId(), event);
- finishOrPause(event.getClientId(), true, true);
+ finishOrPause(event.getClientId(), false, true);
mAppOpsService.scheduleOpActiveChangedIfNeededLocked(parent.op, parent.uid,
parent.packageName, tag, false,
@@ -475,6 +473,8 @@
InProgressStartOpEvent event = events.get(binders.get(i));
if (event != null && event.getUidState() != newState) {
+ int eventAttributionFlags = event.getAttributionFlags();
+ int eventAttributionChainId = event.getAttributionChainId();
try {
// Remove all but one unfinished start count and then call finished() to
// remove start event object
@@ -482,18 +482,18 @@
event.mNumUnfinishedStarts = 1;
AppOpsManager.OpEventProxyInfo proxy = event.getProxy();
- finished(event.getClientId(), false);
+ finished(event.getClientId(), true);
// Call started() to add a new start event object and then add the
// previously removed unfinished start counts back
if (proxy != null) {
startedOrPaused(event.getClientId(), proxy.getUid(),
proxy.getPackageName(), proxy.getAttributionTag(), newState,
- event.getFlags(), false, isRunning,
+ event.getFlags(), true, isRunning,
event.getAttributionFlags(), event.getAttributionChainId());
} else {
startedOrPaused(event.getClientId(), Process.INVALID_UID, null, null,
- newState, event.getFlags(), false, isRunning,
+ newState, event.getFlags(), true, isRunning,
event.getAttributionFlags(), event.getAttributionChainId());
}
@@ -507,6 +507,9 @@
Slog.e(AppOpsService.TAG,
"Cannot switch to new uidState " + newState);
}
+ mAppOpsService.scheduleOpActiveChangedIfNeededLocked(parent.op,
+ parent.uid, parent.packageName, tag, false,
+ eventAttributionFlags, eventAttributionChainId);
}
}
}