1129576 - Wrong default search activity engine set with device under US IP

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Search Activity, defect)

Description

[Aaron Train [:aaronmt]]

This is a simulation of the question of what happens to a en-US user who launches the search activity under a US IP with a search engine set under a US IP.

Currently the default is Google, whereas the engine in the browser is set to Yahoo

Install a VPN off Google Play and connect to a US IP
Launch Firefox, see Yahoo as default
Launch the search activity, see Google as default

Comment 1

[Aaron Train [:aaronmt]]

Need confirmation from actual device in US if search activity engine matches in-browser default (Yahoo)

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

This is still broken. We need to add code to the Search Activity to pull the country code and use it.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: search-activity-geoip v0.1

This patch adds support into the Java SearchEngineManager for fetching and caching the country code used to geo-fence the search engine list.

Once we have the region code, we attempt to find the default search engine for that region. If the region can't be found, we fallback to the non-region default engine.

I did not add any background thread for the networking because we are already on a background thread via the distribution code.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: rename-mozilla-api v0.1

The URL for the location services uses the MOZ_MOZILLA_API_KEY just as Stumbler does. This patch always adds the API key to AppConstants and renames the constant to MOZ_MOZILLA_API_KEY, which is the proper name.

I see this in the processed AppConstants.java:

    public static final String MOZ_MOZILLA_API_KEY = "no-mozilla-api-key";
    public static final boolean MOZ_STUMBLER_BUILD_TIME_ENABLED = ("1" == "1");

Comment 5

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8559987 [details] [diff] [review]
rename-mozilla-api v0.1

Review of attachment 8559987 [details] [diff] [review]:
-----------------------------------------------------------------

One small nit.

::: mobile/android/base/AppConstants.java.in
@@ +98,5 @@
>      // add additional quotes we end up with ""x.y"", which is a syntax error.
>      public static final String MOZILLA_VERSION = @MOZILLA_VERSION@;
>  
> +    public static final String MOZ_MOZILLA_API_KEY = "@MOZ_MOZILLA_API_KEY@";
> +    public static final boolean MOZ_STUMBLER_BUILD_TIME_ENABLED = ("@MOZ_ANDROID_MLS_STUMBLER@" == "1");

This works but isn't "build standard".  Let's do the

//#ifdef MOZ_ANDROID_MLS_STUMBLER
MOZ_STUMBLER_BUILD_TIME_ENABLED = true;
...

block to keep the if defined rather than specific value semantics we use other places.

Comment 6

[:Margaret Leibovic]

Comment on attachment 8559976 [details] [diff] [review]
search-activity-geoip v0.1

Review of attachment 8559976 [details] [diff] [review]:
-----------------------------------------------------------------

Let's get a follow-up bug on file to add logic to the JS side to store a shared preference (and potentially read from this shared preference) so that we only do this country code fetch once.

::: mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.java
@@ +282,5 @@
> +    }
> +
> +    /**
> +     * Gets the country code based on the current IP, using the Mozilla Location Service.
> +     * We cache the country code in a sharead preference, so we only fetch from the network

Typo: shared.

@@ +287,5 @@
> +     * once.
> +     *
> +     * @return String containing the country code
> +     */
> +    private String fetchCountryCode() {

So now we have 3 implementations of country code fetching :/

We should fix bug 1123880.

@@ +317,5 @@
> +                out.write(message.getBytes());
> +                out.close();
> +
> +                in = new BufferedInputStream(urlConnection.getInputStream());
> +                json = convertStreamToString(in);

I'll let rnewman comment on stream processing best practices. He always seems to have an opinion.

@@ +346,5 @@
> +            } else {
> +                Log.e(LOG_TAG, "Country code fetch failed");
> +            }
> +        } catch (Exception e) {
> +            Log.e(LOG_TAG, "Error", e);

More descriptive error message?

As a follow-up, should we add telemetry similar to what the search service has?
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js#580

@@ +362,5 @@
>          try {
>              final JSONObject browsersearch = new JSONObject(RawResource.getAsString(context, R.raw.browsersearch));
> +
> +            // Get the region used to fence search engines.
> +            String region = fetchCountryCode();

So we just block on this? I guess that makes sense, but first run might be kinda slow if the first thing the user ever does is launch the search activity.

@@ +364,5 @@
> +
> +            // Get the region used to fence search engines.
> +            String region = fetchCountryCode();
> +            if (region != null) {
> +                if (browsersearch.has("regions")) {

Wouldn't it be cheaper to do this check first?

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 8559976 [details] [diff] [review]
search-activity-geoip v0.1

Review of attachment 8559976 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.java
@@ +53,5 @@
> +    // Key for shared preference that stores search region.
> +    private static final String PREF_REGION_KEY = "search.region";
> +
> +    // URL for the geo-ip location service. Keep in sync with "browser.search.geoip.url" perference in Gecko.
> +    private static final String GEOIP_LOCATION_URL = "https://location.services.mozilla.com/v1/country?key=" + AppConstants.MOZ_STUMBLER_API_KEY;

IIRC MOZ_STUMBLER_APK_KEY isn't set if we're not built with stumbler enabled. You probably want

  private static final String GEOIP_LOCATION_URL = AppConstants.MOZ_STUMBLER_API_KEY == null ? null : "https://location.services.mozilla.com/v1/country?key=" + AppConstants.MOZ_STUMBLER_API_KEY;

then to null check it later.

@@ +58,5 @@
> +
> +    // This should go through GeckoInterface to get the UA, but the search activity
> +    // doesn't use a GeckoView yet. Until it does, get the UA directly.
> +    private static final String USER_AGENT = HardwareUtils.isTablet() ?
> +        AppConstants.USER_AGENT_FENNEC_TABLET : AppConstants.USER_AGENT_FENNEC_MOBILE;

We've replicated this pattern in a bunch of places now.  :/

@@ +277,5 @@
> +        try {
> +            return new java.util.Scanner(is).useDelimiter("\\A").next();
> +        } catch (java.util.NoSuchElementException e) {
> +            return "";
> +        }

I suggest moving the .close() from the caller into this method. Less likelihood of missing a stream, safer to put the null check in one place, and also means we close the stream before disconnecting the connection, which I think is correct.

@@ +289,5 @@
> +     * @return String containing the country code
> +     */
> +    private String fetchCountryCode() {
> +        // First, we look to see if we have a cached code.
> +        String region = GeckoSharedPrefs.forApp(context).getString(PREF_REGION_KEY, null);

Make this final and use a new variable on line 338.

@@ +293,5 @@
> +        String region = GeckoSharedPrefs.forApp(context).getString(PREF_REGION_KEY, null);
> +        if (region != null) {
> +            return region;
> +        }
> +

Right here:

  if (GEOIP_LOCATION_URL == null) {
      // No stumbler API key.
      return null;
  }

@@ +303,5 @@
> +            HttpURLConnection urlConnection = null;
> +            InputStream in = null;
> +            try {
> +                // POST an empty JSON object.
> +                String message = new JSONObject().toString();

= "{}";

@@ +305,5 @@
> +            try {
> +                // POST an empty JSON object.
> +                String message = new JSONObject().toString();
> +
> +                urlConnection = (HttpURLConnection) url.openConnection();

There's no catch block for this, so prefer this pattern:

  final HttpURLConnection urlConnection = (HttpURLConnection) url.openConnection();
  try {
      urlConnection.setDoOutput(true);
      …
      // This closes the stream for you.
      json = convertStreamToString(new BufferedInputStream(…));
  } finally {
      // This is never null.
      urlConnection.disconnect();
  }

@@ +306,5 @@
> +                // POST an empty JSON object.
> +                String message = new JSONObject().toString();
> +
> +                urlConnection = (HttpURLConnection) url.openConnection();
> +                urlConnection.setDoOutput(true);

setConnectTimeout. setReadTimeout. Make them short; maybe 10000 each.

@@ +312,5 @@
> +                urlConnection.setRequestProperty("User-Agent", USER_AGENT);
> +                urlConnection.setRequestProperty("Content-Type", "application/json");
> +                urlConnection.setFixedLengthStreamingMode(message.getBytes().length);
> +
> +                OutputStream out = urlConnection.getOutputStream();

final

@@ +314,5 @@
> +                urlConnection.setFixedLengthStreamingMode(message.getBytes().length);
> +
> +                OutputStream out = urlConnection.getOutputStream();
> +                out.write(message.getBytes());
> +                out.close();

This should be in a small finally block.

@@ +323,5 @@
> +                if (urlConnection != null) {
> +                    urlConnection.disconnect();
> +                }
> +                if (in != null) {
> +                    try {

Both of these clauses can be replaced by my comment above.

@@ +331,5 @@
> +                    }
> +                }
> +            }
> +
> +            if (json != null) {

if (json == null) {
    Log.e(…);
    return null;
}

final JSONObject response = …;

@@ +335,5 @@
> +            if (json != null) {
> +                // Extract the country code and save it for later in a cache.
> +                JSONObject response = new JSONObject(json);
> +                region = response.optString("country_code", null);
> +                if (region != null) {

If we fail to fetch, we won't save anything. Are we sure that we won't try more than once?

@@ +362,5 @@
>          try {
>              final JSONObject browsersearch = new JSONObject(RawResource.getAsString(context, R.raw.browsersearch));
> +
> +            // Get the region used to fence search engines.
> +            String region = fetchCountryCode();

See my timeout comment above.

@@ +364,5 @@
> +
> +            // Get the region used to fence search engines.
> +            String region = fetchCountryCode();
> +            if (region != null) {
> +                if (browsersearch.has("regions")) {

Yup.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Richard Newman [:rnewman] from comment #7)

> :::
> mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.
> java
> @@ +53,5 @@
> > +    // Key for shared preference that stores search region.
> > +    private static final String PREF_REGION_KEY = "search.region";
> > +
> > +    // URL for the geo-ip location service. Keep in sync with "browser.search.geoip.url" perference in Gecko.
> > +    private static final String GEOIP_LOCATION_URL = "https://location.services.mozilla.com/v1/country?key=" + AppConstants.MOZ_STUMBLER_API_KEY;
> 
> IIRC MOZ_STUMBLER_APK_KEY isn't set if we're not built with stumbler
> enabled. You probably want
> 
>   private static final String GEOIP_LOCATION_URL =
> AppConstants.MOZ_STUMBLER_API_KEY == null ? null :
> "https://location.services.mozilla.com/v1/country?key=" +
> AppConstants.MOZ_STUMBLER_API_KEY;
> 
> then to null check it later.

The next patch deals with the API key by making it always available.

> > +        try {
> > +            return new java.util.Scanner(is).useDelimiter("\\A").next();
> > +        } catch (java.util.NoSuchElementException e) {
> > +            return "";
> > +        }
> 
> I suggest moving the .close() from the caller into this method. Less
> likelihood of missing a stream, safer to put the null check in one place,
> and also means we close the stream before disconnecting the connection,
> which I think is correct.

I'm not a big fan of util functions taking ownership of arg lifecycle though. I will move the stream close to happen before the connection disconnect.

This code was cribbed from SuggestClient.java
I decided to just pass in the HttpURLConnection and rename the method to getHttpResponse. Now it's all self-contained.


> @@ +335,5 @@
> > +            if (json != null) {
> > +                // Extract the country code and save it for later in a cache.
> > +                JSONObject response = new JSONObject(json);
> > +                region = response.optString("country_code", null);
> > +                if (region != null) {
> 
> If we fail to fetch, we won't save anything. Are we sure that we won't try
> more than once?

I am changing this to save an empty string if the fetch fails. Desktop saves an empty region on failure too.

> > +            // Get the region used to fence search engines.
> > +            String region = fetchCountryCode();
> > +            if (region != null) {
> > +                if (browsersearch.has("regions")) {
> 
> Yup.

I don't follow what you two are suggesting here, but we must always fetch a country code / region, even if the JSON does not have any regions. Am I missing something?

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Made almost all the tweaks, except where indicated in comment 8:
https://hg.mozilla.org/integration/fx-team/rev/0fe69a9f2bd4
https://hg.mozilla.org/integration/fx-team/rev/c343219070e2

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0fe69a9f2bd4
https://hg.mozilla.org/mozilla-central/rev/c343219070e2

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8559976 [details] [diff] [review]
search-activity-geoip v0.1

Approval Request Comment
[Feature/regressing bug #]: bug 1117186
[User impact if declined]: Search Activity won't use geoip search list
[Describe test coverage new/current, TreeHerder]: Working on Nightly
[Risks and why]: Low. Search Activity only
[String/UUID change made/needed]: None

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4ab14d90ae41
https://hg.mozilla.org/releases/mozilla-aurora/rev/776044d1654a

https://hg.mozilla.org/releases/mozilla-beta/rev/8180eb5904de
https://hg.mozilla.org/releases/mozilla-beta/rev/8600a7b2e3a6

Comment 13

[Teodora Vermesan (:TeoVermesan)]

I have installed VPN1Click from play store and connected to a server in the USA, so:

On latest Nightly and Aurora: (2015-02-10), I launch Firefox, Yahoo is the default search engine and also Yahoo is default in search activity.
On Firefox 36 Beta 8: I launch Firefox, Google is the default search engine and Yahoo is default in search activity.

Comment 14

[Hanno Schlichting]

For future work using MLS, I'd appreciate a CC on the bug or f? on the patch. This looks ok and I don't need to do a super review for the MLS module here. But a heads up about Fennec now using MLS would have been appreciated.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Hanno Schlichting [:hannosch] from comment #14)
> For future work using MLS, I'd appreciate a CC on the bug or f? on the
> patch. This looks ok and I don't need to do a super review for the MLS
> module here. But a heads up about Fennec now using MLS would have been
> appreciated.

We are just using the same URL as Desktop (and Fennec because it's using Gecko). I didn't see you in the Desktop bugs that added use of the geoip endpoint, so I didn't pull you into this one either.

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Teodora Vermesan (:TeoVermesan) from comment #13)
> I have installed VPN1Click from play store and connected to a server in the
> USA, so:
> 
> On latest Nightly and Aurora: (2015-02-10), I launch Firefox, Yahoo is the
> default search engine and also Yahoo is default in search activity.
> On Firefox 36 Beta 8: I launch Firefox, Google is the default search engine
> and Yahoo is default in search activity.

What you are describing is exactly the mistake I fixed here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1117186#c29

The fix was landed and the next Beta should have this fixed.

Comment 17

[Teodora Vermesan (:TeoVermesan)]

Tested with:
Device: Samsung S4 (Android 4.4)
Build: Firefox for Android 36 Beta 9

I have installed VPN1Click from play store and connected to a server in the USA and Yahoo is the default search engine and also Yahoo is default in search activity.

For non US, Google is the default search engine and also the default in search activity.

Comment 18

[Marcia Knous [:marcia]]

I just updated to the latest Beta using my Nexus 6, and I still see the bug.

Google is my default browser in Settings, but any searches I perform are using Yahoo.

Comment 19

[Teodora Vermesan (:TeoVermesan)]

This is the issue you are dealing with:  Bug 1132089 - Regression: Default search engine overrode on browser upgrade. And also take a look at this: Bug 1131087.

But, if you clear data and have a clean profile, you can see that Yahoo is default in Settings. I've checked this using a Nexus 4 (Android 5.0.1).