Prevent TWA location permission request callback run more than once

When checking TWA location permission with the client app, if the
TWAService did not returns the success message (returns failed),
we treat it as "denied" and returns the result via the
PermissionResponseCallback.

However if the TWAService did handle the extraCommand and later invoked
the TWACallback, the PermissionResponseCallback will be called again.
This causes the OnceCallback in native been called twice, and crashed.

This should happen with a correct TWAService, but it would be nice to
prevent that a TWAService handling extraCommand wrongly crashes Chrome.

This CL fix it by adding a boolean mCalled in PermissionCheckCallback
to mark whether the callback been invoked. so the callback would not
be run more than once.

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

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

@@ -22,7 +22,6 @@ import org.chromium.components.embedder_support.util.Origin;
  */
 public class InstalledWebappBridge {
     private static long sNativeInstalledWebappProvider;
-    private static long sNativePermissionResultCallback;
 
     /**
      * A POD class to store the combination of a permission setting and the origin the permission is

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

@@ -61,13 +61,18 @@ public class LocationPermissionUpdater {
     void checkPermission(Origin origin, long callback) {
         mTrustedWebActivityClient.checkLocationPermission(
                 origin, new TrustedWebActivityClient.PermissionCheckCallback() {
+                    private boolean mCalled;
                     @Override
                     public void onPermissionCheck(ComponentName answeringApp, boolean enabled) {
+                        if (mCalled) return;
+                        mCalled = true;
                         updatePermission(origin, callback, answeringApp, enabled);
                     }
 
                     @Override
                     public void onNoTwaFound() {
+                        if (mCalled) return;
+                        mCalled = true;
                         mPermissionManager.resetStoredPermission(origin, TYPE);
                         InstalledWebappBridge.onGetPermissionResult(callback, false);
                     }

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

@@ -29,6 +29,7 @@ import org.robolectric.shadows.ShadowPackageManager;
 
 import org.chromium.base.test.BaseRobolectricTestRunner;
 import org.chromium.base.test.util.Feature;
+import org.chromium.base.test.util.JniMocker;
 import org.chromium.chrome.browser.browserservices.TrustedWebActivityClient;
 import org.chromium.chrome.browser.browserservices.TrustedWebActivityUmaRecorder;
 import org.chromium.chrome.browser.flags.ChromeFeatureList;
@@ -47,10 +48,14 @@ public class LocationPermissionUpdaterTest {
     private static final Origin ORIGIN = Origin.create("https://www.website.com");
     private static final String PACKAGE_NAME = "com.package.name";
     private static final String OTHER_PACKAGE_NAME = "com.other.package.name";
+    private static final long CALLBACK = 12;
 
     @Rule
     public TestRule mProcessor = new Features.JUnitProcessor();
 
+    @Rule
+    public JniMocker mocker = new JniMocker();
+
     @Mock
     public TrustedWebActivityPermissionManager mPermissionManager;
     @Mock
@@ -58,6 +63,9 @@ public class LocationPermissionUpdaterTest {
     @Mock
     public TrustedWebActivityUmaRecorder mUmaRecorder;
 
+    @Mock
+    private InstalledWebappBridge.Natives mNativeMock;
+
     private LocationPermissionUpdater mLocationPermissionUpdater;
     private ShadowPackageManager mShadowPackageManager;
 
@@ -67,6 +75,8 @@ public class LocationPermissionUpdaterTest {
     public void setUp() {
         MockitoAnnotations.initMocks(this);
 
+        mocker.mock(InstalledWebappBridgeJni.TEST_HOOKS, mNativeMock);
+
         PackageManager pm = RuntimeEnvironment.application.getPackageManager();
         mShadowPackageManager = shadowOf(pm);
         mLocationPermissionUpdater = new LocationPermissionUpdater(
@@ -80,7 +90,7 @@ public class LocationPermissionUpdaterTest {
         installTrustedWebActivityService(ORIGIN, PACKAGE_NAME);
         setLocationEnabledForClient(false);
 
-        mLocationPermissionUpdater.checkPermission(ORIGIN, 0 /*callback*/);
+        mLocationPermissionUpdater.checkPermission(ORIGIN, CALLBACK);
 
         verifyPermissionUpdated(false);
     }
@@ -91,7 +101,7 @@ public class LocationPermissionUpdaterTest {
         installTrustedWebActivityService(ORIGIN, PACKAGE_NAME);
         setLocationEnabledForClient(true);
 
-        mLocationPermissionUpdater.checkPermission(ORIGIN, 0 /*callback*/);
+        mLocationPermissionUpdater.checkPermission(ORIGIN, CALLBACK);
 
         verifyPermissionUpdated(true);
     }
@@ -101,11 +111,11 @@ public class LocationPermissionUpdaterTest {
     public void updatesPermission_onSubsequentCalls() {
         installTrustedWebActivityService(ORIGIN, PACKAGE_NAME);
         setLocationEnabledForClient(true);
-        mLocationPermissionUpdater.checkPermission(ORIGIN, 0 /*callback*/);
+        mLocationPermissionUpdater.checkPermission(ORIGIN, CALLBACK);
         verifyPermissionUpdated(true);
 
         setLocationEnabledForClient(false);
-        mLocationPermissionUpdater.checkPermission(ORIGIN, 0 /*callback*/);
+        mLocationPermissionUpdater.checkPermission(ORIGIN, CALLBACK);
         verifyPermissionUpdated(false);
     }
 
@@ -114,13 +124,13 @@ public class LocationPermissionUpdaterTest {
     public void updatesPermission_onNewClient() {
         installTrustedWebActivityService(ORIGIN, PACKAGE_NAME);
         setLocationEnabledForClient(true);
-        mLocationPermissionUpdater.checkPermission(ORIGIN, 0 /*callback*/);
+        mLocationPermissionUpdater.checkPermission(ORIGIN, CALLBACK);
         verifyPermissionUpdated(true);
 
         installBrowsableIntentHandler(ORIGIN, OTHER_PACKAGE_NAME);
         installTrustedWebActivityService(ORIGIN, OTHER_PACKAGE_NAME);
         setLocationEnabledForClient(false);
-        mLocationPermissionUpdater.checkPermission(ORIGIN, 0 /*callback*/);
+        mLocationPermissionUpdater.checkPermission(ORIGIN, CALLBACK);
         verifyPermissionUpdated(OTHER_PACKAGE_NAME, false);
     }
 
@@ -130,7 +140,7 @@ public class LocationPermissionUpdaterTest {
         installTrustedWebActivityService(ORIGIN, PACKAGE_NAME);
         setLocationEnabledForClient(true);
 
-        mLocationPermissionUpdater.checkPermission(ORIGIN, 0 /*callback*/);
+        mLocationPermissionUpdater.checkPermission(ORIGIN, CALLBACK);
 
         uninstallTrustedWebActivityService(ORIGIN);
         mLocationPermissionUpdater.onClientAppUninstalled(ORIGIN);
@@ -182,6 +192,7 @@ public class LocationPermissionUpdaterTest {
         verify(mPermissionManager)
                 .updatePermission(eq(ORIGIN), eq(packageName), eq(ContentSettingsType.GEOLOCATION),
                         eq(enabled));
+        verify(mNativeMock).notifyPermissionResult(eq(CALLBACK), eq(enabled));
     }
 
     private void verifyPermissionReset() {
@@ -193,4 +204,21 @@ public class LocationPermissionUpdaterTest {
         verify(mPermissionManager, never())
                 .resetStoredPermission(eq(ORIGIN), eq(ContentSettingsType.GEOLOCATION));
     }
+
+    @Test
+    @Feature("TrustedWebActivity")
+    public void updatesPermissionOnlyOnce_incorrectReturnsFromTwaService() {
+        doAnswer(invocation -> {
+            TrustedWebActivityClient.PermissionCheckCallback callback = invocation.getArgument(1);
+            // PermissionCheckCallback is invoked twice with different result.
+            callback.onPermissionCheck(new ComponentName(PACKAGE_NAME, "FakeClass"), false);
+            callback.onPermissionCheck(new ComponentName(PACKAGE_NAME, "FakeClass"), true);
+            return true;
+        })
+                .when(mTrustedWebActivityClient)
+                .checkLocationPermission(eq(ORIGIN), any());
+
+        mLocationPermissionUpdater.checkPermission(ORIGIN, CALLBACK);
+        verifyPermissionUpdated(PACKAGE_NAME, false);
+    }
 }