🎫 Correctly notice when no TWA is found for a scope.

When updating our code to AndroidX.Browser's API changes[1] this bug
crept in. Previously `execute` would return a boolean that would be
false if we couldn't find a TWA for the given scope. The new version,
returns a ListenableFuture.

While moving the code over, this requirement got skipped and now when
a TWA doesn't exist, we still return `true`. This CL fixes that.

[1]: https://chromium-review.googlesource.com/c/chromium/src/+/1940329

Bug: 1082718
Change-Id: I78aef4695d1467748590a62b3192d8a76cfcbfc1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2226816Reviewed-by: Ella Ge <eirage@chromium.org>
Commit-Queue: Peter Conn <peconn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#774537}

chrome/android/java/src/org/chromium/chrome/browser/browserservices/TrustedWebActivityClient.java

@@ -81,6 +81,18 @@ public class TrustedWebActivityClient {
          * #checkLocationPermission}.
          */
         void onPermissionCheck(ComponentName answeringApp, boolean enabled);
+
+        /**
+         * Called when {@link #checkNotificationPermission} or {@link #checkLocationPermission}
+         * can't find a TWA to connect to.
+         */
+        default void onNoTwaFound() {}
+    }
+
+    private interface ExecutionCallback {
+        void onConnected(Origin origin, TrustedWebActivityServiceConnection service)
+                throws RemoteException;
+        default void onNoTwaFound() {}
     }
 
     /**
@@ -116,14 +128,24 @@ public class TrustedWebActivityClient {
      *         Ensure that the app has been added to the {@link TrustedWebActivityPermissionManager}
      *         before calling this.
      */
-    public boolean checkNotificationPermission(Origin origin, PermissionCheckCallback callback) {
+    public void checkNotificationPermission(Origin origin, PermissionCheckCallback callback) {
         Resources res = ContextUtils.getApplicationContext().getResources();
         String channelDisplayName = res.getString(R.string.notification_category_group_general);
 
-        return connectAndExecute(origin.uri(),
-                (originCopy, service)
-                        -> callback.onPermissionCheck(service.getComponentName(),
-                                service.areNotificationsEnabled(channelDisplayName)));
+        connectAndExecute(origin.uri(),
+                new ExecutionCallback() {
+                    @Override
+                    public void onConnected(Origin originCopy,
+                            TrustedWebActivityServiceConnection service) throws RemoteException {
+                        callback.onPermissionCheck(service.getComponentName(),
+                                service.areNotificationsEnabled(channelDisplayName));
+                    }
+
+                    @Override
+                    public void onNoTwaFound() {
+                        callback.onNoTwaFound();
+                    }
+                });
     }
 
     /**
@@ -133,25 +155,35 @@ public class TrustedWebActivityClient {
      *         Ensure that the app has been added to the {@link TrustedWebActivityPermissionManager}
      *         before calling this.
      */
-    public boolean checkLocationPermission(Origin origin, PermissionCheckCallback callback) {
-        return connectAndExecute(origin.uri(), (originCopy, service) -> {
-            TrustedWebActivityCallback resultCallback = new TrustedWebActivityCallback() {
-                @Override
-                public void onExtraCallback(String callbackName, @Nullable Bundle bundle) {
-                    boolean granted = false;
-                    if (TextUtils.equals(callbackName, CHECK_LOCATION_PERMISSION_COMMAND_NAME)
-                            && bundle != null) {
-                        granted = bundle.getBoolean(EXTRA_COMMAND_EXECUTION_RESULT);
+    public void checkLocationPermission(Origin origin, PermissionCheckCallback callback) {
+        connectAndExecute(origin.uri(), new ExecutionCallback() {
+            @Override
+            public void onConnected(Origin origin, TrustedWebActivityServiceConnection service)
+                    throws RemoteException {
+                TrustedWebActivityCallback resultCallback = new TrustedWebActivityCallback() {
+                    @Override
+                    public void onExtraCallback(String callbackName, @Nullable Bundle bundle) {
+                        boolean granted = false;
+                        if (TextUtils.equals(callbackName, CHECK_LOCATION_PERMISSION_COMMAND_NAME)
+                                && bundle != null) {
+                            granted = bundle.getBoolean(EXTRA_COMMAND_EXECUTION_RESULT);
+                        }
+                        callback.onPermissionCheck(service.getComponentName(), granted);
                     }
-                    callback.onPermissionCheck(service.getComponentName(), granted);
+                };
+
+                Bundle executionResult = service.extraCommand(
+                        CHECK_LOCATION_PERMISSION_COMMAND_NAME, Bundle.EMPTY, resultCallback);
+                // Set permission to false if the service does not know how to handle the
+                // extraCommand.
+                if (executionResult == null) {
+                    callback.onPermissionCheck(service.getComponentName(), false);
                 }
-            };
+            }
 
-            Bundle executionResult = service.extraCommand(
-                    CHECK_LOCATION_PERMISSION_COMMAND_NAME, Bundle.EMPTY, resultCallback);
-            // Set permission to false if the service does not know how to handle the extraCommand.
-            if (executionResult == null) {
-                callback.onPermissionCheck(service.getComponentName(), false);
+            @Override
+            public void onNoTwaFound() {
+                callback.onNoTwaFound();
             }
         });
     }
@@ -237,30 +269,37 @@ public class TrustedWebActivityClient {
         connectAndExecute(scope, (origin, service) -> service.cancel(platformTag, platformId));
     }
 
-    private interface ExecutionCallback {
-        void onConnected(Origin origin, TrustedWebActivityServiceConnection service)
-                throws RemoteException;
-    }
-
-    private boolean connectAndExecute(Uri scope, ExecutionCallback callback) {
+    private void connectAndExecute(Uri scope, ExecutionCallback callback) {
         Origin origin = Origin.create(scope);
-        if (origin == null) return false;
+        if (origin == null) {
+            callback.onNoTwaFound();
+            return;
+        }
 
         Set<Token> possiblePackages = mDelegatesManager.getAllDelegateApps(origin);
-        if (possiblePackages == null || possiblePackages.isEmpty()) return false;
+        if (possiblePackages == null || possiblePackages.isEmpty()) {
+            callback.onNoTwaFound();
+            return;
+        }
 
         ListenableFuture<TrustedWebActivityServiceConnection> connection =
                 mConnection.connect(scope, possiblePackages, AsyncTask.THREAD_POOL_EXECUTOR);
         connection.addListener(() -> {
             try {
                 callback.onConnected(origin, connection.get());
-            } catch (RemoteException | ExecutionException | InterruptedException
-                    | SecurityException e) {
+            } catch (RemoteException | InterruptedException e) {
+                // These failures could be transient - a RemoteException indicating that the TWA
+                // got killed as it was answering and an InterruptedException to do with threading
+                // on our side. In this case, there's not anything necessarily wrong with the TWA.
                 Log.w(TAG, "Failed to execute TWA command.", e);
+            } catch (ExecutionException | SecurityException e) {
+                // An ExecutionException means that we could not find a TWA to connect to and a
+                // SecurityException means that the TWA doesn't trust this app. In either cases we
+                // consider that there is no TWA for the scope.
+                Log.w(TAG, "Failed to connect to TWA to execute command", e);
+                callback.onNoTwaFound();
             }
         }, UI_THREAD_EXECUTOR);
-
-        return true;
     }
 
     /**

chrome/android/java/src/org/chromium/chrome/browser/browserservices/permissiondelegation/LocationPermissionUpdater.java

@@ -59,12 +59,19 @@ public class LocationPermissionUpdater {
      * TrustedWebActivityService is found, and the TWAService supports location permission.
      */
     void checkPermission(Origin origin, long callback) {
-        boolean couldConnect = mTrustedWebActivityClient.checkLocationPermission(
-                origin, (app, enabled) -> updatePermission(origin, callback, app, enabled));
-        if (!couldConnect) {
-            mPermissionManager.resetStoredPermission(origin, TYPE);
-            InstalledWebappBridge.onGetPermissionResult(callback, false);
-        }
+        mTrustedWebActivityClient.checkLocationPermission(
+                origin, new TrustedWebActivityClient.PermissionCheckCallback() {
+                    @Override
+                    public void onPermissionCheck(ComponentName answeringApp, boolean enabled) {
+                        updatePermission(origin, callback, answeringApp, enabled);
+                    }
+
+                    @Override
+                    public void onNoTwaFound() {
+                        mPermissionManager.resetStoredPermission(origin, TYPE);
+                        InstalledWebappBridge.onGetPermissionResult(callback, false);
+                    }
+                });
     }
 
     @WorkerThread

chrome/android/java/src/org/chromium/chrome/browser/browserservices/permissiondelegation/NotificationPermissionUpdater.java

@@ -68,13 +68,18 @@ public class NotificationPermissionUpdater {
     public void onClientAppUninstalled(Origin origin) {
         // See if there is any other app installed that could handle the notifications (and update
         // to that apps notification permission if it exists).
-        boolean couldConnect = mTrustedWebActivityClient.checkNotificationPermission(origin,
-                (app, enabled) -> updatePermission(origin, app, enabled));
+        mTrustedWebActivityClient.checkNotificationPermission(origin,
+                new TrustedWebActivityClient.PermissionCheckCallback() {
+                    @Override
+                    public void onPermissionCheck(ComponentName answeringApp, boolean enabled) {
+                        updatePermission(origin, answeringApp, enabled);
+                    }
 
-        // If not, we return notification state to what it was before installation.
-        if (!couldConnect) {
-            mPermissionManager.unregister(origin);
-        }
+                    @Override
+                    public void onNoTwaFound() {
+                        mPermissionManager.unregister(origin);
+                    }
+                });
     }
 
     @WorkerThread

chrome/android/javatests/src/org/chromium/chrome/browser/browserservices/TrustedWebActivityClientTest.java

@@ -200,6 +200,34 @@ public class TrustedWebActivityClientTest {
         Assert.assertEquals(mResponseHandler.mNotificationId, NOTIFICATION_ID);
     }
 
+    /**
+     * Tests that the appropriate callback is called when we try to connect to a TWA that doesn't
+     * exist.
+     */
+    @Test
+    @SmallTest
+    public void testNoClientFound() throws TimeoutException {
+        Origin scope = Origin.createOrThrow("https://www.websitewithouttwa.com/");
+
+        CallbackHelper noTwaFound = new CallbackHelper();
+
+        TrustedWebActivityClient.PermissionCheckCallback callback =
+                new TrustedWebActivityClient.PermissionCheckCallback() {
+            @Override
+            public void onPermissionCheck(ComponentName answeringApp, boolean enabled) { }
+
+            @Override
+            public void onNoTwaFound() {
+                noTwaFound.notifyCalled();
+            }
+        };
+
+        PostTask.runOrPostTask(UiThreadTaskTraits.DEFAULT,
+                () -> mClient.checkNotificationPermission(scope, callback));
+
+        noTwaFound.waitForFirst();
+    }
+
     /**
      * Tests {@link TrustedWebActivityClient#createLaunchIntentForTwa}.
      */

chrome/android/junit/src/org/chromium/chrome/browser/browserservices/permissiondelegation/LocationPermissionUpdaterTest.java

@@ -6,9 +6,9 @@ package org.chromium.chrome.browser.browserservices.permissiondelegation;
 
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
-import static org.mockito.Mockito.when;
 import static org.robolectric.Shadows.shadowOf;
 
 import android.content.ComponentName;
@@ -152,20 +152,22 @@ public class LocationPermissionUpdaterTest {
     /** "Installs" a Trusted Web Activity Service for the origin. */
     @SuppressWarnings("unchecked")
     private void installTrustedWebActivityService(Origin origin, String packageName) {
-        when(mTrustedWebActivityClient.checkLocationPermission(eq(origin), any()))
-                .thenAnswer(invocation -> {
-                    TrustedWebActivityClient.PermissionCheckCallback callback =
-                            ((TrustedWebActivityClient.PermissionCheckCallback)
-                                            invocation.getArgument(1));
-                    callback.onPermissionCheck(
-                            new ComponentName(packageName, "FakeClass"), mLocationEnabled);
-                    return true;
-                });
+        doAnswer(invocation -> {
+            TrustedWebActivityClient.PermissionCheckCallback callback =
+                    invocation.getArgument(1);
+            callback.onPermissionCheck(
+                    new ComponentName(packageName, "FakeClass"), mLocationEnabled);
+            return true;
+        }).when(mTrustedWebActivityClient).checkLocationPermission(eq(origin), any());
     }
 
     private void uninstallTrustedWebActivityService(Origin origin) {
-        when(mTrustedWebActivityClient.checkLocationPermission(eq(origin), any()))
-                .thenReturn(false);
+        doAnswer(invocation -> {
+            TrustedWebActivityClient.PermissionCheckCallback callback =
+                    invocation.getArgument(1);
+            callback.onNoTwaFound();
+            return true;
+        }).when(mTrustedWebActivityClient).checkLocationPermission(eq(origin), any());
     }
 
     private void setLocationEnabledForClient(boolean enabled) {

chrome/android/junit/src/org/chromium/chrome/browser/browserservices/permissiondelegation/NotificationPermissionUpdaterTest.java

@@ -9,9 +9,9 @@ import static org.mockito.ArgumentMatchers.anyBoolean;
 import static org.mockito.ArgumentMatchers.anyInt;
 import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
-import static org.mockito.Mockito.when;
 import static org.robolectric.Shadows.shadowOf;
 
 import android.content.ComponentName;
@@ -182,16 +182,14 @@ public class NotificationPermissionUpdaterTest {
     /** "Installs" a Trusted Web Activity Service for the origin. */
     @SuppressWarnings("unchecked")
     private void installTrustedWebActivityService(Origin origin, String packageName) {
-        when(mTrustedWebActivityClient.checkNotificationPermission(eq(origin), any()))
-                .thenAnswer(invocation -> {
-                    TrustedWebActivityClient.PermissionCheckCallback callback =
-                            ((TrustedWebActivityClient.PermissionCheckCallback)
-                                            invocation.getArgument(1));
-                    callback.onPermissionCheck(
-                            new ComponentName(packageName, "FakeClass"),
-                            mNotificationsEnabled);
-                    return true;
-                });
+        doAnswer(invocation -> {
+            TrustedWebActivityClient.PermissionCheckCallback callback =
+                    invocation.getArgument(1);
+            callback.onPermissionCheck(
+                    new ComponentName(packageName, "FakeClass"),
+                    mNotificationsEnabled);
+            return true;
+        }).when(mTrustedWebActivityClient).checkNotificationPermission(eq(origin), any());
     }
 
     private void setNotificationsEnabledForClient(boolean enabled) {
@@ -199,8 +197,12 @@ public class NotificationPermissionUpdaterTest {
     }
 
     private void uninstallTrustedWebActivityService(Origin origin) {
-        when(mTrustedWebActivityClient.checkNotificationPermission(eq(origin), any()))
-                .thenReturn(false);
+        doAnswer(invocation -> {
+            TrustedWebActivityClient.PermissionCheckCallback callback =
+                    invocation.getArgument(1);
+            callback.onNoTwaFound();
+            return true;
+        }).when(mTrustedWebActivityClient).checkNotificationPermission(eq(origin), any());
     }
 
     private void verifyPermissionNotUpdated() {