Added a CriteriaHelper to ensure to allow content settings time to be…

… set

Based on logs added in previous CL it appears that there is an async step
in between the setting being set and it being persisted. This becomes
apparent when seeing that the value being retrieved is "BLOCK" even though
we just set it to "ALLOW" previously.

Log sample:
```
08-03 18:53:15.541 26119 26119 I cr_GeolocationHeader: [crbug/1338183] uri: https://www.google.com/search?q=potatoes
08-03 18:53:15.541 26119 26119 I cr_GeolocationHeader: [crbug/1338183] profile.OTR: false
08-03 18:53:15.541 26119 26119 I cr_GeolocationHeader: [crbug/1338183] getBrowserProfileTypeFromProfile: 0
08-03 18:53:15.541 26119 26119 I cr_GeolocationHeader: [crbug/1338183] profile.getNativeBrowserContextPointer: 1494027648
08-03 18:53:15.542 26119 26119 I cr_GeolocationHeader: [crbug/1338183] isDSEOrigin: true
08-03 18:53:15.570 26119 26119 I cr_GeolocationHeader: [crbug/1338183] settingValue: 2
08-03 18:53:15.570 26119 26119 I cr_GeolocationHeader: [crbug/1338183] Site permission is missing
```

Bug: 1338183, 1333038, 1344427, 1344431, 1342630
Change-Id: I90eac50aba1fab50d6bb93ec6e4a90439d8dfd7c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3810489
Reviewed-by: Tomasz Wiszkowski <ender@google.com>
Commit-Queue: Andy Paicu <andypaicu@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1032527}

chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderTest.java

@@ -17,8 +17,8 @@
 import org.junit.Test;
 import org.junit.runner.RunWith;
 
-import org.chromium.base.Log;
 import org.chromium.base.test.util.CommandLineFlags;
+import org.chromium.base.test.util.CriteriaHelper;
 import org.chromium.base.test.util.Feature;
 import org.chromium.chrome.browser.flags.ChromeFeatureList;
 import org.chromium.chrome.browser.flags.ChromeSwitches;
@@ -309,31 +309,14 @@ private void assertHeaderEquals(long locationTime, String header) {
     }
 
     private void setPermission(final @ContentSettingValues int setting) {
+        PermissionInfo infoHttps =
+                new PermissionInfo(ContentSettingsType.GEOLOCATION, SEARCH_URL_1, null, false);
+
         TestThreadUtils.runOnUiThreadBlocking(() -> {
-            Profile profile = Profile.getLastUsedRegularProfile();
-            PermissionInfo infoHttps =
-                    new PermissionInfo(ContentSettingsType.GEOLOCATION, SEARCH_URL_1, null, false);
-            Log.i(TAG,
-                    "[crbug/1338183] Before Settting|isNativeInitialized: "
-                            + profile.isNativeInitialized());
-            Log.i(TAG,
-                    "[crbug/1338183] Before Settting|getBrowserProfileTypeFromProfile: "
-                            + Profile.getBrowserProfileTypeFromProfile(profile));
-            Log.i(TAG,
-                    "[crbug/1338183] Before Settting|getNativeBrowserContextPointer: "
-                            + profile.getNativeBrowserContextPointer());
-
-            infoHttps.setContentSetting(profile, setting);
-
-            Log.i(TAG,
-                    "[crbug/1338183] After Settting|isNativeInitialized: "
-                            + profile.isNativeInitialized());
-            Log.i(TAG,
-                    "[crbug/1338183] Before Settting|getBrowserProfileTypeFromProfile: "
-                            + Profile.getBrowserProfileTypeFromProfile(profile));
-            Log.i(TAG,
-                    "[crbug/1338183] Before Settting|getNativeBrowserContextPointer: "
-                            + profile.getNativeBrowserContextPointer());
+            infoHttps.setContentSetting(Profile.getLastUsedRegularProfile(), setting);
+        });
+        CriteriaHelper.pollUiThread(() -> {
+            return infoHttps.getContentSetting(Profile.getLastUsedRegularProfile()) == setting;
         });
     }
 }

chrome/android/javatests/src/org/chromium/chrome/browser/site_settings/PermissionInfoTest.java

@@ -7,7 +7,6 @@
 import androidx.test.filters.SmallTest;
 
 import org.junit.After;
-import org.junit.Assert;
 import org.junit.ClassRule;
 import org.junit.Rule;
 import org.junit.Test;
@@ -16,6 +15,7 @@
 import org.chromium.base.test.util.Batch;
 import org.chromium.base.test.util.CallbackHelper;
 import org.chromium.base.test.util.CommandLineFlags;
+import org.chromium.base.test.util.CriteriaHelper;
 import org.chromium.base.test.util.Feature;
 import org.chromium.base.test.util.RequiresRestart;
 import org.chromium.chrome.browser.browsing_data.BrowsingDataBridge;
@@ -35,7 +35,6 @@
 import org.chromium.content_public.common.ContentSwitches;
 
 import java.util.concurrent.Callable;
-import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeoutException;
 
 /** Tests for the PermissionInfoTest. */
@@ -86,35 +85,15 @@ private static Profile getPrimaryOTRProfile() {
                                 /*createIfNeeded=*/true));
     }
 
-    private void setGeolocation(
-            String origin, String embedder, @ContentSettingValues int setting, Profile profile) {
-        PermissionInfo info = new PermissionInfo(ContentSettingsType.GEOLOCATION, origin, embedder);
-        TestThreadUtils.runOnUiThreadBlocking(() -> info.setContentSetting(profile, setting));
-    }
+    private void setSettingAndExpectValue(@ContentSettingsType int type, String origin,
+            String embedder, @ContentSettingValues int setting, Profile profile,
+            @ContentSettingValues int expectedSetting) {
+        PermissionInfo info = new PermissionInfo(type, origin, embedder);
 
-    private @ContentSettingValues int getGeolocation(
-            String origin, String embedder, Profile profile) throws ExecutionException {
-        return TestThreadUtils.runOnUiThreadBlocking(() -> {
-            PermissionInfo info =
-                    new PermissionInfo(ContentSettingsType.GEOLOCATION, origin, embedder);
-            return info.getContentSetting(profile);
-        });
-    }
-
-    private void setNotifications(
-            String origin, String embedder, @ContentSettingValues int setting, Profile profile) {
-        PermissionInfo info =
-                new PermissionInfo(ContentSettingsType.NOTIFICATIONS, origin, embedder);
         TestThreadUtils.runOnUiThreadBlocking(() -> info.setContentSetting(profile, setting));
-    }
 
-    private @ContentSettingValues int getNotifications(
-            String origin, String embedder, Profile profile) throws ExecutionException {
-        return TestThreadUtils.runOnUiThreadBlocking(() -> {
-            PermissionInfo info =
-                    new PermissionInfo(ContentSettingsType.NOTIFICATIONS, origin, embedder);
-            return info.getContentSetting(profile);
-        });
+        CriteriaHelper.pollUiThread(
+                () -> { return info.getContentSetting(profile) == expectedSetting; });
     }
 
     private void resetNotificationsSettingsForTest() {
@@ -130,12 +109,10 @@ private void resetNotificationsSettingsForTest() {
     public void testResetDSEGeolocation_InPrimaryOTRProfile_DefaultsToAskFromBlock()
             throws Throwable {
         Profile primaryOTRProfile = getPrimaryOTRProfile();
-        setGeolocation(DSE_ORIGIN, null, ContentSettingValues.BLOCK, primaryOTRProfile);
-        Assert.assertEquals(
-                ContentSettingValues.BLOCK, getGeolocation(DSE_ORIGIN, null, primaryOTRProfile));
-        setGeolocation(DSE_ORIGIN, null, ContentSettingValues.DEFAULT, primaryOTRProfile);
-        Assert.assertEquals(
-                ContentSettingValues.ASK, getGeolocation(DSE_ORIGIN, null, primaryOTRProfile));
+        setSettingAndExpectValue(ContentSettingsType.GEOLOCATION, DSE_ORIGIN, null,
+                ContentSettingValues.BLOCK, primaryOTRProfile, ContentSettingValues.BLOCK);
+        setSettingAndExpectValue(ContentSettingsType.GEOLOCATION, DSE_ORIGIN, null,
+                ContentSettingValues.DEFAULT, primaryOTRProfile, ContentSettingValues.ASK);
     }
 
     @Test
@@ -144,12 +121,10 @@ public void testResetDSEGeolocation_InPrimaryOTRProfile_DefaultsToAskFromBlock()
     public void testResetDSEGeolocation_InNonPrimaryOTRProfile_DefaultsToAskFromBlock()
             throws Throwable {
         Profile nonPrimaryOTRProfile = getNonPrimaryOTRProfile();
-        setGeolocation(DSE_ORIGIN, null, ContentSettingValues.BLOCK, nonPrimaryOTRProfile);
-        Assert.assertEquals(
-                ContentSettingValues.BLOCK, getGeolocation(DSE_ORIGIN, null, nonPrimaryOTRProfile));
-        setGeolocation(DSE_ORIGIN, null, ContentSettingValues.DEFAULT, nonPrimaryOTRProfile);
-        Assert.assertEquals(
-                ContentSettingValues.ASK, getGeolocation(DSE_ORIGIN, null, nonPrimaryOTRProfile));
+        setSettingAndExpectValue(ContentSettingsType.GEOLOCATION, DSE_ORIGIN, null,
+                ContentSettingValues.BLOCK, nonPrimaryOTRProfile, ContentSettingValues.BLOCK);
+        setSettingAndExpectValue(ContentSettingsType.GEOLOCATION, DSE_ORIGIN, null,
+                ContentSettingValues.DEFAULT, nonPrimaryOTRProfile, ContentSettingValues.ASK);
     }
 
     @Test
@@ -158,12 +133,10 @@ public void testResetDSEGeolocation_InNonPrimaryOTRProfile_DefaultsToAskFromBloc
     @RequiresRestart
     public void testResetDSEGeolocation_RegularProfile_DefaultsToAskFromBlock() throws Throwable {
         Profile regularProfile = getRegularProfile();
-        setGeolocation(DSE_ORIGIN, null, ContentSettingValues.BLOCK, regularProfile);
-        Assert.assertEquals(
-                ContentSettingValues.BLOCK, getGeolocation(DSE_ORIGIN, null, regularProfile));
-        setGeolocation(DSE_ORIGIN, null, ContentSettingValues.DEFAULT, regularProfile);
-        Assert.assertEquals(
-                ContentSettingValues.ASK, getGeolocation(DSE_ORIGIN, null, regularProfile));
+        setSettingAndExpectValue(ContentSettingsType.GEOLOCATION, DSE_ORIGIN, null,
+                ContentSettingValues.BLOCK, regularProfile, ContentSettingValues.BLOCK);
+        setSettingAndExpectValue(ContentSettingsType.GEOLOCATION, DSE_ORIGIN, null,
+                ContentSettingValues.DEFAULT, regularProfile, ContentSettingValues.ASK);
     }
 
     @Test
@@ -175,12 +148,10 @@ public void testResetDSENotification_InPrimaryOTRProfile_DefaultsToAskFromBlock(
 
         // Resetting in incognito should not have the same behavior.
         resetNotificationsSettingsForTest();
-        setNotifications(DSE_ORIGIN, null, ContentSettingValues.BLOCK, primaryOTRProfile);
-        Assert.assertEquals(
-                ContentSettingValues.BLOCK, getNotifications(DSE_ORIGIN, null, primaryOTRProfile));
-        setNotifications(DSE_ORIGIN, null, ContentSettingValues.DEFAULT, primaryOTRProfile);
-        Assert.assertEquals(
-                ContentSettingValues.ASK, getNotifications(DSE_ORIGIN, null, primaryOTRProfile));
+        setSettingAndExpectValue(ContentSettingsType.NOTIFICATIONS, DSE_ORIGIN, null,
+                ContentSettingValues.BLOCK, primaryOTRProfile, ContentSettingValues.BLOCK);
+        setSettingAndExpectValue(ContentSettingsType.NOTIFICATIONS, DSE_ORIGIN, null,
+                ContentSettingValues.DEFAULT, primaryOTRProfile, ContentSettingValues.ASK);
     }
 
     @Test
@@ -192,12 +163,10 @@ public void testResetDSENotification_InNonPrimaryOTRProfile_DefaultsToAskFromBlo
 
         // Resetting in incognito should not have the same behavior.
         resetNotificationsSettingsForTest();
-        setNotifications(DSE_ORIGIN, null, ContentSettingValues.BLOCK, nonPrimaryOTRProfile);
-        Assert.assertEquals(ContentSettingValues.BLOCK,
-                getNotifications(DSE_ORIGIN, null, nonPrimaryOTRProfile));
-        setNotifications(DSE_ORIGIN, null, ContentSettingValues.DEFAULT, nonPrimaryOTRProfile);
-        Assert.assertEquals(
-                ContentSettingValues.ASK, getNotifications(DSE_ORIGIN, null, nonPrimaryOTRProfile));
+        setSettingAndExpectValue(ContentSettingsType.NOTIFICATIONS, DSE_ORIGIN, null,
+                ContentSettingValues.BLOCK, nonPrimaryOTRProfile, ContentSettingValues.BLOCK);
+        setSettingAndExpectValue(ContentSettingsType.NOTIFICATIONS, DSE_ORIGIN, null,
+                ContentSettingValues.DEFAULT, nonPrimaryOTRProfile, ContentSettingValues.ASK);
     }
 
     @Test
@@ -207,11 +176,9 @@ public void testResetDSENotification_InNonPrimaryOTRProfile_DefaultsToAskFromBlo
     public void testResetDSENotification_RegularProfile_DefaultsToAskFromBlock() throws Throwable {
         Profile regularProfile = getRegularProfile();
         resetNotificationsSettingsForTest();
-        setNotifications(DSE_ORIGIN, null, ContentSettingValues.BLOCK, regularProfile);
-        Assert.assertEquals(
-                ContentSettingValues.BLOCK, getNotifications(DSE_ORIGIN, null, regularProfile));
-        setNotifications(DSE_ORIGIN, null, ContentSettingValues.DEFAULT, regularProfile);
-        Assert.assertEquals(
-                ContentSettingValues.ASK, getNotifications(DSE_ORIGIN, null, regularProfile));
+        setSettingAndExpectValue(ContentSettingsType.NOTIFICATIONS, DSE_ORIGIN, null,
+                ContentSettingValues.BLOCK, regularProfile, ContentSettingValues.BLOCK);
+        setSettingAndExpectValue(ContentSettingsType.NOTIFICATIONS, DSE_ORIGIN, null,
+                ContentSettingValues.DEFAULT, regularProfile, ContentSettingValues.ASK);
     }
 }

chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java

@@ -277,7 +277,6 @@ private static int geoHeaderStateForUrl(Profile profile, String url, boolean rec
             // Only send X-Geo header if the user hasn't disabled geolocation for url.
             if (isLocationDisabledForUrl(profile, uri)) {
                 if (recordUma) recordHistogram(UMA_LOCATION_DISABLED_FOR_GOOGLE_DOMAIN);
-                Log.i(TAG, "[crbug/1338183] Site permission is missing");
                 return HeaderState.LOCATION_PERMISSION_BLOCKED;
             }
 
@@ -461,23 +460,12 @@ static int getGeolocationPermission(Tab tab) {
      * geolocation infobar).
      */
     static boolean isLocationDisabledForUrl(Profile profile, Uri uri) {
-        Log.i(TAG, "[crbug/1338183] uri: " + uri);
-        Log.i(TAG, "[crbug/1338183] profile.OTR: " + profile.isOffTheRecord());
-        Log.i(TAG,
-                "[crbug/1338183] getBrowserProfileTypeFromProfile: "
-                        + Profile.getBrowserProfileTypeFromProfile(profile));
-        Log.i(TAG,
-                "[crbug/1338183] profile.getNativeBrowserContextPointer: "
-                        + profile.getNativeBrowserContextPointer());
-
         // TODO(raymes): The call to isDSEOrigin is only needed if this could be called for
         // an origin that isn't the default search engine. Otherwise remove this line.
         boolean isDSEOrigin = WebsitePreferenceBridge.isDSEOrigin(profile, uri.toString());
-        Log.i(TAG, "[crbug/1338183] isDSEOrigin: " + isDSEOrigin);
         @ContentSettingValues
         @Nullable
         Integer settingValue = locationContentSettingForUrl(profile, uri);
-        Log.i(TAG, "[crbug/1338183] settingValue: " + settingValue);
 
         boolean enabled = isDSEOrigin && settingValue == ContentSettingValues.ALLOW;
         return !enabled;