Move executeAppFunctionInternal off binder thread

so that the threading model is more clear. ExecuteAppFunction is running in the binder thread and ExecuteAppFunctionInternal is not.

Also, only close a session when we have finished using it.

And improve the latency of onUserUnlocked. Now it only takes 14ms

Flag: android.app.appfunctions.flags.enable_app_function_manager
Bug: 360864791
Fixes: 368153367
Test: atest CtsAppFunctionTestCases

Change-Id: Iabd87cab440fd0459acda30ece1ddd9ef576d6af

6 files changed, 124 insertions, 116 deletions

diff --git a/services/appfunctions/java/com/android/server/appfunctions/AppFunctionManagerServiceImpl.java b/services/appfunctions/java/com/android/server/appfunctions/AppFunctionManagerServiceImpl.java
index cf039df3bc96..469a1a2251e8 100644
--- a/services/appfunctions/java/com/android/server/appfunctions/AppFunctionManagerServiceImpl.java
+++ b/services/appfunctions/java/com/android/server/appfunctions/AppFunctionManagerServiceImpl.java
@@ -20,6 +20,7 @@ import static com.android.server.appfunctions.AppFunctionExecutors.THREAD_POOL_E
 
 import android.annotation.NonNull;
 import android.annotation.Nullable;
+import android.annotation.WorkerThread;
 import android.app.appfunctions.AppFunctionStaticMetadataHelper;
 import android.app.appfunctions.ExecuteAppFunctionAidlRequest;
 import android.app.appfunctions.ExecuteAppFunctionResponse;
@@ -45,7 +46,6 @@ import com.android.server.SystemService.TargetUser;
 import com.android.server.appfunctions.RemoteServiceCaller.RunServiceCallCallback;
 import com.android.server.appfunctions.RemoteServiceCaller.ServiceUsageCompleteListener;
 
-import java.io.IOException;
 import java.util.Objects;
 import java.util.concurrent.CompletionException;
 
@@ -83,23 +83,6 @@ public class AppFunctionManagerServiceImpl extends IAppFunctionManager.Stub {
         mServiceConfig = serviceConfig;
     }
 
-    @Override
-    public void executeAppFunction(
-            @NonNull ExecuteAppFunctionAidlRequest requestInternal,
-            @NonNull IExecuteAppFunctionCallback executeAppFunctionCallback) {
-        Objects.requireNonNull(requestInternal);
-        Objects.requireNonNull(executeAppFunctionCallback);
-
-        final SafeOneTimeExecuteAppFunctionCallback safeExecuteAppFunctionCallback =
-                new SafeOneTimeExecuteAppFunctionCallback(executeAppFunctionCallback);
-
-        try {
-            executeAppFunctionInternal(requestInternal, safeExecuteAppFunctionCallback);
-        } catch (Exception e) {
-            safeExecuteAppFunctionCallback.onResult(mapExceptionToExecuteAppFunctionResponse(e));
-        }
-    }
-
     /** Called when the user is unlocked. */
     public void onUserUnlocked(TargetUser user) {
         Objects.requireNonNull(user);
@@ -120,18 +103,22 @@ public class AppFunctionManagerServiceImpl extends IAppFunctionManager.Stub {
         }
     }
 
-    private void executeAppFunctionInternal(
-            ExecuteAppFunctionAidlRequest requestInternal,
-            SafeOneTimeExecuteAppFunctionCallback safeExecuteAppFunctionCallback) {
+    @Override
+    public void executeAppFunction(
+            @NonNull ExecuteAppFunctionAidlRequest requestInternal,
+            @NonNull IExecuteAppFunctionCallback executeAppFunctionCallback) {
+        Objects.requireNonNull(requestInternal);
+        Objects.requireNonNull(executeAppFunctionCallback);
+
+        final SafeOneTimeExecuteAppFunctionCallback safeExecuteAppFunctionCallback =
+                new SafeOneTimeExecuteAppFunctionCallback(executeAppFunctionCallback);
 
         String validatedCallingPackage;
-        UserHandle targetUser;
         try {
             validatedCallingPackage =
                     mCallerValidator.validateCallingPackage(requestInternal.getCallingPackage());
-            targetUser =
-                    mCallerValidator.verifyTargetUserHandle(
-                            requestInternal.getUserHandle(), validatedCallingPackage);
+            mCallerValidator.verifyTargetUserHandle(
+                    requestInternal.getUserHandle(), validatedCallingPackage);
         } catch (SecurityException exception) {
             safeExecuteAppFunctionCallback.onResult(
                     ExecuteAppFunctionResponse.newFailure(
@@ -141,6 +128,30 @@ public class AppFunctionManagerServiceImpl extends IAppFunctionManager.Stub {
             return;
         }
 
+        int callingUid = Binder.getCallingUid();
+        int callingPid = Binder.getCallingUid();
+        THREAD_POOL_EXECUTOR.execute(
+                () -> {
+                    try {
+                        executeAppFunctionInternal(
+                                requestInternal,
+                                callingUid,
+                                callingPid,
+                                safeExecuteAppFunctionCallback);
+                    } catch (Exception e) {
+                        safeExecuteAppFunctionCallback.onResult(
+                                mapExceptionToExecuteAppFunctionResponse(e));
+                    }
+                });
+    }
+
+    @WorkerThread
+    private void executeAppFunctionInternal(
+            ExecuteAppFunctionAidlRequest requestInternal,
+            int callingUid,
+            int callingPid,
+            SafeOneTimeExecuteAppFunctionCallback safeExecuteAppFunctionCallback) {
+        UserHandle targetUser = requestInternal.getUserHandle();
         // TODO(b/354956319): Add and honor the new enterprise policies.
         if (mCallerValidator.isUserOrganizationManaged(targetUser)) {
             safeExecuteAppFunctionCallback.onResult(
@@ -165,7 +176,9 @@ public class AppFunctionManagerServiceImpl extends IAppFunctionManager.Stub {
         var unused =
                 mCallerValidator
                         .verifyCallerCanExecuteAppFunction(
-                                validatedCallingPackage,
+                                callingUid,
+                                callingPid,
+                                requestInternal.getCallingPackage(),
                                 targetPackageName,
                                 requestInternal.getClientRequest().getFunctionIdentifier())
                         .thenAccept(
@@ -191,19 +204,14 @@ public class AppFunctionManagerServiceImpl extends IAppFunctionManager.Stub {
                                                         /* extras= */ null));
                                         return;
                                     }
-                                    final long token = Binder.clearCallingIdentity();
-                                    try {
-                                        bindAppFunctionServiceUnchecked(
-                                                requestInternal,
-                                                serviceIntent,
-                                                targetUser,
-                                                safeExecuteAppFunctionCallback,
-                                                /* bindFlags= */ Context.BIND_AUTO_CREATE,
-                                                /* timeoutInMillis= */ mServiceConfig
-                                                        .getExecuteAppFunctionTimeoutMillis());
-                                    } finally {
-                                        Binder.restoreCallingIdentity(token);
-                                    }
+                                    bindAppFunctionServiceUnchecked(
+                                            requestInternal,
+                                            serviceIntent,
+                                            targetUser,
+                                            safeExecuteAppFunctionCallback,
+                                            /* bindFlags= */ Context.BIND_AUTO_CREATE,
+                                            /* timeoutInMillis= */ mServiceConfig
+                                                    .getExecuteAppFunctionTimeoutMillis());
                                 })
                         .exceptionally(
                                 ex -> {
@@ -332,30 +340,27 @@ public class AppFunctionManagerServiceImpl extends IAppFunctionManager.Stub {
             Slog.d(TAG, "AppSearch Manager not found for user: " + user.getUserIdentifier());
             return;
         }
-        try (FutureGlobalSearchSession futureGlobalSearchSession =
+        FutureGlobalSearchSession futureGlobalSearchSession =
                 new FutureGlobalSearchSession(
-                        perUserAppSearchManager, AppFunctionExecutors.THREAD_POOL_EXECUTOR)) {
-            AppFunctionMetadataObserver appFunctionMetadataObserver =
-                    new AppFunctionMetadataObserver(
-                            user.getUserHandle(),
-                            mContext.createContextAsUser(user.getUserHandle(), /* flags= */ 0));
-            var unused =
-                    futureGlobalSearchSession
-                            .registerObserverCallbackAsync(
-                                    "android",
-                                    new ObserverSpec.Builder().build(),
-                                    THREAD_POOL_EXECUTOR,
-                                    appFunctionMetadataObserver)
-                            .whenComplete(
-                                    (voidResult, ex) -> {
-                                        if (ex != null) {
-                                            Slog.e(TAG, "Failed to register observer: ", ex);
-                                        }
-                                    });
-
-        } catch (IOException ex) {
-            Slog.e(TAG, "Failed to close observer session: ", ex);
-        }
+                        perUserAppSearchManager, AppFunctionExecutors.THREAD_POOL_EXECUTOR);
+        AppFunctionMetadataObserver appFunctionMetadataObserver =
+                new AppFunctionMetadataObserver(
+                        user.getUserHandle(),
+                        mContext.createContextAsUser(user.getUserHandle(), /* flags= */ 0));
+        var unused =
+                futureGlobalSearchSession
+                        .registerObserverCallbackAsync(
+                                "android",
+                                new ObserverSpec.Builder().build(),
+                                THREAD_POOL_EXECUTOR,
+                                appFunctionMetadataObserver)
+                        .whenComplete(
+                                (voidResult, ex) -> {
+                                    if (ex != null) {
+                                        Slog.e(TAG, "Failed to register observer: ", ex);
+                                    }
+                                    futureGlobalSearchSession.close();
+                                });
     }
 
     private void trySyncRuntimeMetadata(@NonNull TargetUser user) {
diff --git a/services/appfunctions/java/com/android/server/appfunctions/CallerValidator.java b/services/appfunctions/java/com/android/server/appfunctions/CallerValidator.java
index e7a861e0a0dc..3592ed587ab0 100644
--- a/services/appfunctions/java/com/android/server/appfunctions/CallerValidator.java
+++ b/services/appfunctions/java/com/android/server/appfunctions/CallerValidator.java
@@ -60,9 +60,9 @@ public interface CallerValidator {
      * Validates that the caller can execute the specified app function.
      *
      * <p>The caller can execute if the app function's package name is the same as the caller's
-     * package or the caller has either {@link Manifest.permission.EXECUTE_APP_FUNCTIONS_TRUSTED} or
-     * {@link Manifest.permission.EXECUTE_APP_FUNCTIONS} granted. In some cases, app functions can
-     * still opt-out of caller having {@link Manifest.permission.EXECUTE_APP_FUNCTIONS}.
+     * package or the caller has either {@link Manifest.permission#EXECUTE_APP_FUNCTIONS_TRUSTED} or
+     * {@link Manifest.permission#EXECUTE_APP_FUNCTIONS} granted. In some cases, app functions can
+     * still opt-out of caller having {@link Manifest.permission#EXECUTE_APP_FUNCTIONS}.
      *
      * @param callerPackageName The calling package (as previously validated).
      * @param targetPackageName The package that owns the app function to execute.
@@ -70,6 +70,8 @@ public interface CallerValidator {
      * @return Whether the caller can execute the specified app function.
      */
     AndroidFuture<Boolean> verifyCallerCanExecuteAppFunction(
+            int callingUid,
+            int callingPid,
             @NonNull String callerPackageName,
             @NonNull String targetPackageName,
             @NonNull String functionId);
diff --git a/services/appfunctions/java/com/android/server/appfunctions/CallerValidatorImpl.java b/services/appfunctions/java/com/android/server/appfunctions/CallerValidatorImpl.java
index 94a63b43dd17..8b6251a59e3a 100644
--- a/services/appfunctions/java/com/android/server/appfunctions/CallerValidatorImpl.java
+++ b/services/appfunctions/java/com/android/server/appfunctions/CallerValidatorImpl.java
@@ -20,6 +20,7 @@ import static android.app.appfunctions.AppFunctionStaticMetadataHelper.APP_FUNCT
 import static android.app.appfunctions.AppFunctionStaticMetadataHelper.APP_FUNCTION_STATIC_NAMESPACE;
 import static android.app.appfunctions.AppFunctionStaticMetadataHelper.STATIC_PROPERTY_RESTRICT_CALLERS_WITH_EXECUTE_APP_FUNCTIONS;
 import static android.app.appfunctions.AppFunctionStaticMetadataHelper.getDocumentIdForAppFunction;
+
 import static com.android.server.appfunctions.AppFunctionExecutors.THREAD_POOL_EXECUTOR;
 
 import android.Manifest;
@@ -41,6 +42,7 @@ import android.os.UserHandle;
 import android.os.UserManager;
 
 import com.android.internal.infra.AndroidFuture;
+
 import java.util.Objects;
 
 /* Validates that caller has the correct privilege to call an AppFunctionManager Api. */
@@ -82,7 +84,6 @@ class CallerValidatorImpl implements CallerValidator {
     }
 
     @Override
-    @BinderThread
     @RequiresPermission(
             anyOf = {
                 Manifest.permission.EXECUTE_APP_FUNCTIONS_TRUSTED,
@@ -90,6 +91,8 @@ class CallerValidatorImpl implements CallerValidator {
             },
             conditional = true)
     public AndroidFuture<Boolean> verifyCallerCanExecuteAppFunction(
+            int callingUid,
+            int callingPid,
             @NonNull String callerPackageName,
             @NonNull String targetPackageName,
             @NonNull String functionId) {
@@ -97,11 +100,11 @@ class CallerValidatorImpl implements CallerValidator {
             return AndroidFuture.completedFuture(true);
         }
 
-        int pid = Binder.getCallingPid();
-        int uid = Binder.getCallingUid();
         boolean hasTrustedExecutionPermission =
                 mContext.checkPermission(
-                                Manifest.permission.EXECUTE_APP_FUNCTIONS_TRUSTED, pid, uid)
+                                Manifest.permission.EXECUTE_APP_FUNCTIONS_TRUSTED,
+                                callingPid,
+                                callingUid)
                         == PackageManager.PERMISSION_GRANTED;
 
         if (hasTrustedExecutionPermission) {
@@ -109,40 +112,34 @@ class CallerValidatorImpl implements CallerValidator {
         }
 
         boolean hasExecutionPermission =
-                mContext.checkPermission(Manifest.permission.EXECUTE_APP_FUNCTIONS, pid, uid)
+                mContext.checkPermission(
+                                Manifest.permission.EXECUTE_APP_FUNCTIONS, callingPid, callingUid)
                         == PackageManager.PERMISSION_GRANTED;
 
         if (!hasExecutionPermission) {
             return AndroidFuture.completedFuture(false);
         }
 
-        final long token = Binder.clearCallingIdentity();
-        try {
-            FutureAppSearchSession futureAppSearchSession =
-                    new FutureAppSearchSessionImpl(
-                            mContext.getSystemService(AppSearchManager.class),
-                            THREAD_POOL_EXECUTOR,
-                            new SearchContext.Builder(APP_FUNCTION_STATIC_METADATA_DB).build());
+        FutureAppSearchSession futureAppSearchSession =
+                new FutureAppSearchSessionImpl(
+                        mContext.getSystemService(AppSearchManager.class),
+                        THREAD_POOL_EXECUTOR,
+                        new SearchContext.Builder(APP_FUNCTION_STATIC_METADATA_DB).build());
 
-            String documentId = getDocumentIdForAppFunction(targetPackageName, functionId);
+        String documentId = getDocumentIdForAppFunction(targetPackageName, functionId);
 
-            return futureAppSearchSession
-                    .getByDocumentId(
-                            new GetByDocumentIdRequest.Builder(APP_FUNCTION_STATIC_NAMESPACE)
-                                    .addIds(documentId)
-                                    .build())
-                    .thenApply(
-                            batchResult ->
-                                    getGenericDocumentFromBatchResult(batchResult, documentId))
-                    .thenApply(
-                            CallerValidatorImpl::getRestrictCallersWithExecuteAppFunctionsProperty)
-                    .thenApply(
-                            restrictCallersWithExecuteAppFunctions ->
-                                    !restrictCallersWithExecuteAppFunctions
-                                            && hasExecutionPermission);
-        } finally {
-            Binder.restoreCallingIdentity(token);
-        }
+        return futureAppSearchSession
+                .getByDocumentId(
+                        new GetByDocumentIdRequest.Builder(APP_FUNCTION_STATIC_NAMESPACE)
+                                .addIds(documentId)
+                                .build())
+                .thenApply(
+                        batchResult -> getGenericDocumentFromBatchResult(batchResult, documentId))
+                .thenApply(document -> !getRestrictCallersWithExecuteAppFunctionsProperty(document))
+                .whenComplete(
+                        (result, throwable) -> {
+                            futureAppSearchSession.close();
+                        });
     }
 
     private static GenericDocument getGenericDocumentFromBatchResult(
@@ -167,19 +164,13 @@ class CallerValidatorImpl implements CallerValidator {
     }
 
     @Override
-    @BinderThread
     public boolean isUserOrganizationManaged(@NonNull UserHandle targetUser) {
-        final long callingIdentityToken = Binder.clearCallingIdentity();
-        try {
-            if (Objects.requireNonNull(mContext.getSystemService(DevicePolicyManager.class))
-                    .isDeviceManaged()) {
-                return true;
-            }
-            return Objects.requireNonNull(mContext.getSystemService(UserManager.class))
-                    .isManagedProfile(targetUser.getIdentifier());
-        } finally {
-            Binder.restoreCallingIdentity(callingIdentityToken);
+        if (Objects.requireNonNull(mContext.getSystemService(DevicePolicyManager.class))
+                .isDeviceManaged()) {
+            return true;
         }
+        return Objects.requireNonNull(mContext.getSystemService(UserManager.class))
+                .isManagedProfile(targetUser.getIdentifier());
     }
 
     /**
diff --git a/services/appfunctions/java/com/android/server/appfunctions/FutureAppSearchSession.java b/services/appfunctions/java/com/android/server/appfunctions/FutureAppSearchSession.java
index 0044b4b958cb..de2034b5be2f 100644
--- a/services/appfunctions/java/com/android/server/appfunctions/FutureAppSearchSession.java
+++ b/services/appfunctions/java/com/android/server/appfunctions/FutureAppSearchSession.java
@@ -84,6 +84,9 @@ public interface FutureAppSearchSession extends Closeable {
     AndroidFuture<FutureSearchResults> search(
             @NonNull String queryExpression, @NonNull SearchSpec searchSpec);
 
+    @Override
+    void close();
+
     /** A future API wrapper of {@link android.app.appsearch.SearchResults}. */
     interface FutureSearchResults {
 
diff --git a/services/appfunctions/java/com/android/server/appfunctions/FutureAppSearchSessionImpl.java b/services/appfunctions/java/com/android/server/appfunctions/FutureAppSearchSessionImpl.java
index 3079d9f51bf3..d24bb871c393 100644
--- a/services/appfunctions/java/com/android/server/appfunctions/FutureAppSearchSessionImpl.java
+++ b/services/appfunctions/java/com/android/server/appfunctions/FutureAppSearchSessionImpl.java
@@ -38,7 +38,6 @@ import android.app.appsearch.SetSchemaResponse;
 
 import com.android.internal.infra.AndroidFuture;
 
-import java.io.IOException;
 import java.util.List;
 import java.util.Objects;
 import java.util.concurrent.Executor;
@@ -183,7 +182,15 @@ public class FutureAppSearchSessionImpl implements FutureAppSearchSession {
     }
 
     @Override
-    public void close() throws IOException {}
+    public void close() {
+        getSessionAsync()
+                .whenComplete(
+                        (appSearchSession, throwable) -> {
+                            if (appSearchSession != null) {
+                                appSearchSession.close();
+                            }
+                        });
+    }
 
     private static final class FutureSearchResultsImpl implements FutureSearchResults {
         private final SearchResults mSearchResults;
diff --git a/services/appfunctions/java/com/android/server/appfunctions/FutureGlobalSearchSession.java b/services/appfunctions/java/com/android/server/appfunctions/FutureGlobalSearchSession.java
index 0c2262456032..874c5daa1662 100644
--- a/services/appfunctions/java/com/android/server/appfunctions/FutureGlobalSearchSession.java
+++ b/services/appfunctions/java/com/android/server/appfunctions/FutureGlobalSearchSession.java
@@ -23,12 +23,10 @@ import android.app.appsearch.GlobalSearchSession;
 import android.app.appsearch.exceptions.AppSearchException;
 import android.app.appsearch.observer.ObserverCallback;
 import android.app.appsearch.observer.ObserverSpec;
-import android.util.Slog;
 
 import com.android.internal.infra.AndroidFuture;
 
 import java.io.Closeable;
-import java.io.IOException;
 import java.util.concurrent.Executor;
 
 /** A wrapper around {@link GlobalSearchSession} that provides a future-based API. */
@@ -84,11 +82,13 @@ public class FutureGlobalSearchSession implements Closeable {
     }
 
     @Override
-    public void close() throws IOException {
-        try {
-            getSessionAsync().get().close();
-        } catch (Exception ex) {
-            Slog.e(TAG, "Failed to close global search session", ex);
-        }
+    public void close() {
+        getSessionAsync()
+                .whenComplete(
+                        (appSearchSession, throwable) -> {
+                            if (appSearchSession != null) {
+                                appSearchSession.close();
+                            }
+                        });
     }
 }