1123974 - geoIP request should not override existing isUS cached values

Status: VERIFIED FIXED

(Firefox :: Search, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

We don't want the new GeoIP check introduced in bug 1109120 to override the previous timezone-based check added in bug 1102416.

I think that just means making storeCountryCode (https://mxr.mozilla.org/mozilla-beta/source/toolkit/components/search/nsSearchService.js#481) not set the browser.search.isUS pref if it is already set.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-1123974-geoip-result-should-not-override-an-exis.patch

I've no context for the reason we want this, but I've a few concerns:

* If a user hits the 2s timeout or gets a non-JSON response from geoip, we are likely to fallback to the timezone check.  Once this has been set once, it's going to be used forever.

* Given the other bugs around geoip have been uplifted, this pref has *already* been overridden for most users.  So best I can tell it will only impact new users/profiles who see that timeout/error on the first geoip lookup.  It seems worse to have these users "forever" be locked with an incorrect geo-specific default than to have a correct default should it work on the next run (or even when the geoip request eventually completes after that 2s limit)

But as I said, I've no context, so under the assumption it really is what we want to do, I think this patch achieves it :)

[Only semi-related: should a "refresh profile" carry these prefs over?  If not, the search provider may change on refresh - which could be both good and bad depending on your perspective ;]

Comment 2

[:Gavin Sharp [email: [e-mail]]]

The primary motivator is not changing the default yet again for users of 34.0.5/35 who are in Canada/Mexico/South America and who upgrade to Firefox 36 (i.e. avoid default engine flip-flopping).

It would be ideal for this fix to not impact new 36 users in any way (i.e. maintain existing behavior in error conditions for new users who haven't used 34.0.5/35), but I guess that is a bit more complicated to implement.

(In reply to Mark Hammond [:markh] from comment #1)
> * If a user hits the 2s timeout or gets a non-JSON response from geoip, we
> are likely to fallback to the timezone check.  Once this has been set once,
> it's going to be used forever.

We should avoid this for new 36 (or later) users, ideally.

> * Given the other bugs around geoip have been uplifted, this pref has
> *already* been overridden for most users.

Not release channel users (yet).

> [Only semi-related: should a "refresh profile" carry these prefs over?  If
> not, the search provider may change on refresh - which could be both good
> and bad depending on your perspective ;]

I think it's fine for "refresh firefox" to reset these prefs.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8552228 [details] [diff] [review]
0001-Bug-1123974-geoip-result-should-not-override-an-exis.patch

I think we can probably get the desired behavior by having the timezone fallback set a separate pref (i.e. not isUS) that is only used when countryCode is not set? That might require reworking some of this logic though...

Comment 4

[Mark Hammond [:markh] [:mhammond]]

I'm still a little confused here :(

* release users already have isUS set and we want to keep that stable for them.
* soon they'll get the geoip check that will set countryCode and override isUS - and we don't want to override isUS.
* this implies the countryCode and isUS will be contradictory, but we probably never want to believe countryCode (as if we did, we'd flip-flop their default engine)

So this sounds to me like we can simply skip the geoip code completely if isUS is already set.  Those users will never get a countryCode pref, but that sounds OK - we are never going to believe that over isUS - and it still leaves the option in the future to reinstate that geoip check if that pref isn't set once the desired semantics in that scenario are clearer.

Gavin, what am I missing?

Comment 5

[:Gavin Sharp [email: [e-mail]]]

(In reply to Mark Hammond [:markh] from comment #4)
> * release users already have isUS set and we want to keep that stable for
> them.
> * soon they'll get the geoip check that will set countryCode and override
> isUS - and we don't want to override isUS.

Yes.

> * this implies the countryCode and isUS will be contradictory, but we
> probably never want to believe countryCode (as if we did, we'd flip-flop
> their default engine)

We might want to use countryCode for something else, or we might eventually change our decision to not obey it here, so I don't think we should just skip the countryCode request.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-1123974-geoip-result-should-not-override-users-a.patch

This is roughly what Gavin and I discussed in IRC.

* The new "canonical" preference is browser.search.region (I decided against "locale" as the expectation there would likely be a string like "en-US" rather than just the country part; I'm obviously open to a different name)  If this pref is set, it is compared against "US" to determine the defaults.

* There's a "migration" step: If we startup and find isUS is true but countryCode is not set, we set the new "region" pref to "US".  If isUS doesn't exist or is false, we don't set the new "region" pref.  In neither case do we touch countryCode.

* As part of normal init, we still check if countryCode isn't set, and start a geoip request.  On completion we set the countryCode pref, and set the region pref only if it doesn't already have a value.

* If we need to know isUS() but don't have the region pref, we fallback to a timezone check - but note the result of this timezone check is never cached in a pref (and indeed, the isUS pref is *never* set after this patch).  This means that in edge-cases (eg, where geoip requests never give a country-code), the timezone check will end up being made every time, but this seems relatively inexpensive so shouldn't be a problem - but we could cache the result in a global if that is a problem.

The end result is that countryCode should always reflect the result of the geoip.  The region value should *usually* be the same as countryCode, with the exception being when region is set as part of the migration step - it might be forced to "US" there, while the subsequent geoip request may return something other than "US" - but as described above, region is canonical, so such users stick with US defaults (ie, this is the key point that meets the bug requirements)

Also worth making explicit is that some existing users may have isUS false and no countryCode.  These users are on google.  Once this code runs, they *may* end up with region=="US", so may have their engine switched to yahoo - but these are users who should have been switched to Yahoo earlier but weren't due to a false-negative in the timezone check.

I think the patch is complete, although there are a couple of "XXX" lines in the tests where I need to check the isUSTimezone() fallback was made. The only reasonable way I can see to do this is with an nsIConsoleListener and to look for the log message generated in that case - I'll add that if the patch gets f+.

Comment 7

[Mark Hammond [:markh] [:mhammond]]

Oh - there's also a second migration step: if countryCode is set and region is not, just copy the countryCode to region.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8555640 [details] [diff] [review]
0001-Bug-1123974-geoip-result-should-not-override-users-a.patch

Thanks for the write up, that sounds good. Haven't looked at the code closely or run through the whole testing matrix in my head, but looks sane on a quick pass.

Seems like we should probably call migrateRegionPrefs() from _syncInit too?

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-1123974-geoip-result-should-not-override-users-a.patch

This patch adds the call to migrateRegionPrefs() to the sync init flow and also adds a console listener to check we fell back to a TZ check when necessary.

Try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=002b6d650202

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8556257 [details] [diff] [review]
0001-Bug-1123974-geoip-result-should-not-override-users-a.patch

Hoping Felipe or Florian can get to this before I do.

Comment 11

[Florian Quèze [:florian]]

Comment on attachment 8556257 [details] [diff] [review]
0001-Bug-1123974-geoip-result-should-not-override-users-a.patch

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

When the synchronous initialization code path is triggered while the geoip XHR is pending, it seems we let the XHR finish, save the prefs and then store in telemetry that the init was already done. Should we also re-init the search service to take into account the new pref values? If the answer is yes, I guess it could be a separate bug, so not a reason for r-, and all the other comments I left inline are very trivial nits.

::: toolkit/components/search/nsSearchService.js
@@ +427,5 @@
> +//   result of that geoip request.
> +// * A "region" pref, once set, is the region actually used for search.  In
> +//   most cases it will be identical to the countryCode pref.
> +// * The value of "region" and "countryCode" will only not agree in one edge
> +//   case - 34/35 users who previously been configured to use US defaults

"who previously been" seems like a word is missing here.

@@ +433,5 @@
> +//   regardless of what countryCode geoip returns.
> +// * We may want to know if we are in the US before we have *either*
> +//   countryCode or region - in which case we fallback to a timezone check,
> +//   but we don't persist that value anywhere in the expectation we will
> +//   eventually get a countryCode/region

nit: missing '.' at the end of the sentence.

@@ +451,3 @@
>    try {
> +    let isUS = Services.prefs.getBoolPref("browser.search.isUS");
> +    if (isUS && !Services.prefs.prefHasUserValue("browser.search.countryCode")) {

nit: the isUS variable is used only once and doesn't really help readability so you could as well do:
    if (Services.prefs.getBoolPref("browser.search.isUS") &&

@@ +479,3 @@
>      return false;
>    }
> +  // If we've got a region pref, trust it.

nit: I would put an empty line before this comment.

@@ +479,4 @@
>      return false;
>    }
> +  // If we've got a region pref, trust it.
> +  const cachePref = "browser.search.region";

nit: This const is used only once and could be inlined (you inlined it in the rest of the code).

@@ +498,5 @@
>      return basepref + ".US";
>    return basepref;
>  }
>  
> +// A method that tries to determine if this user is in a US geography

nit: '.' at the end of the comment.

::: toolkit/components/search/tests/xpcshell/test_location_malformed_json.js
@@ +7,5 @@
> +    let consoleService = Components.classes["@mozilla.org/consoleservice;1"]
> +                                   .getService(Components.interfaces.nsIConsoleService);
> +
> +    let listener = {
> +      QueryInterface : function(iid) {

QueryInterface: XPCOMUtils.generateQI([Ci.nsIConsoleListener])

(XPCOMUtils.jsm is already imported by head_search.js)

@@ +15,5 @@
> +        }
> +        return this;
> +      },
> +      observe : function (msg) {
> +        if (msg.message.indexOf("getIsUS() fell back to a timezone check with the result=") == 0) {

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/startsWith

@@ +21,5 @@
> +          resolve(msg);
> +        }
> +      }
> +    };
> +    consoleService.registerListener(listener);

Services.console.

::: toolkit/components/search/tests/xpcshell/test_location_migrate_countrycode_isUS.js
@@ +10,5 @@
> +  do_check_false(Services.search.isInitialized);
> +
> +  let engineDummyFile = gProfD.clone();
> +  engineDummyFile.append("searchplugins");
> +  engineDummyFile.append("test-search-engine.xml");

Is this line actually used?

::: toolkit/components/search/tests/xpcshell/test_location_migrate_no_countrycode_isUS.js
@@ +10,5 @@
> +  do_check_false(Services.search.isInitialized);
> +
> +  let engineDummyFile = gProfD.clone();
> +  engineDummyFile.append("searchplugins");
> +  engineDummyFile.append("test-search-engine.xml");

Same here.

@@ +36,5 @@
> +    for (let hid of ["SEARCH_SERVICE_COUNTRY_TIMEOUT",
> +                     "SEARCH_SERVICE_COUNTRY_FETCH_CAUSED_SYNC_INIT"]) {
> +      let histogram = Services.telemetry.getHistogramById(hid);
> +      let snapshot = histogram.snapshot();
> +      deepEqual(snapshot.counts, [1,0,0]); // boolean probe so 3 buckets, expect 1 result for |0|.

I don't really understand the meaning of [1,0,0] here, but I'll assume this line tests what you want.

::: toolkit/components/search/tests/xpcshell/test_location_migrate_no_countrycode_notUS.js
@@ +10,5 @@
> +  do_check_false(Services.search.isInitialized);
> +
> +  let engineDummyFile = gProfD.clone();
> +  engineDummyFile.append("searchplugins");
> +  engineDummyFile.append("test-search-engine.xml");

Same comment here too. Should this duplicated initialization code be moved to head_search.js?

::: toolkit/components/search/tests/xpcshell/test_location_sync.js
@@ +17,5 @@
>    }
>  }
>  
> +// A console listener so we can listen for a log message from nsSearchService.
> +function promiseTimezoneMessage() {

See my comments in the test_location_malformed_json.js file.
Should this function be moved to head_search.js?

Comment 12

[Mark Hammond [:markh] [:mhammond]]

Thanks for the thorough review:

(In reply to Florian Quèze [:florian] [:flo] from comment #11)

> When the synchronous initialization code path is triggered while the geoip
> XHR is pending, it seems we let the XHR finish, save the prefs and then
> store in telemetry that the init was already done. Should we also re-init
> the search service to take into account the new pref values?

Yeah, this behaviour existed before the patch - Gavin decided that wasn't necessary - IIRC, it was because having the default engine change mid-session is probably even more confusing than the current behaviour where it may change next session start.  So I haven't opened a followup for this.

<snip lots of nits I addressed>

> :::
> toolkit/components/search/tests/xpcshell/
> test_location_migrate_countrycode_isUS.js
> @@ +10,5 @@
> > +  do_check_false(Services.search.isInitialized);
> > +
> > +  let engineDummyFile = gProfD.clone();
> > +  engineDummyFile.append("searchplugins");
> > +  engineDummyFile.append("test-search-engine.xml");
> 
> Is this line actually used?

I believe so - the file ends up with profd/searchplugins/test-search-engine.xml

> Same comment here too. Should this duplicated initialization code be moved
> to head_search.js?

Yeah :)  I did that.

> > +// A console listener so we can listen for a log message from nsSearchService.
> > +function promiseTimezoneMessage() {
> 
> See my comments in the test_location_malformed_json.js file.
> Should this function be moved to head_search.js?

It's short enough now and only in 2 places, so IMO the benefits of having the function local and obvious outweighs the benefits of sharing the code and causing ppl to do an mxr search to locate it - so I left them in-place.

https://hg.mozilla.org/integration/fx-team/rev/0dd9e1ca3c34

Comment 13

[Florian Quèze [:florian]]

(In reply to Mark Hammond [:markh] from comment #12)

> > toolkit/components/search/tests/xpcshell/
> > test_location_migrate_countrycode_isUS.js
> > @@ +10,5 @@
> > > +  do_check_false(Services.search.isInitialized);
> > > +
> > > +  let engineDummyFile = gProfD.clone();
> > > +  engineDummyFile.append("searchplugins");
> > > +  engineDummyFile.append("test-search-engine.xml");
> > 
> > Is this line actually used?
> 
> I believe so - the file ends up with
> profd/searchplugins/test-search-engine.xml

I was under the impression that
  let engineDummyFile = gProfD.clone();
  engineDummyFile.append("searchplugins");
  engineDummyFile.append("test-search-engine.xml");
  let engineDir = engineDummyFile.parent;

could be simplified to:
  let engineDir = gProfD.clone();
  engineDir.append("searchplugins");

as in the following code engineDummyFile is never used.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0dd9e1ca3c34

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Mark, do you have plan for 36 for this patch? Thanks

Comment 16

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8556257 [details] [diff] [review]
0001-Bug-1123974-geoip-result-should-not-override-users-a.patch

Approval Request Comment
[Feature/regressing bug #]: Locale specific search defaults.
[User impact if declined]: Users who already had their default search engine switched to Yahoo may have it switched back to Google on upgrade
[Describe test coverage new/current, TreeHerder]: Tests
[Risks and why]: Should be low risk and is limited to said default switching
[String/UUID change made/needed]: None

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for uplift.

Comment 18

[Mark Hammond [:markh] [:mhammond]]

Attachment: 36/37 version

This patch had non-trivial conflicts in nsSearchService.js (eg, when we check locale="US" has changed and geoSpecificDefaultsEnabled() logic remains inline) due to bug 1117186 not being uplifted, so I'm requesting re-review as a sanity check.  There were no test conflicts and they still pass.

Try on aurora: https://treeherder.mozilla.org/#/jobs?repo=try&revision=159047075b4d
Try on beta: https://treeherder.mozilla.org/#/jobs?repo=try&revision=174a869d46f8 (commit message is wrong in that push, but otherwise the same)

Comment 19

[Florian Quèze [:florian]]

(In reply to Mark Hammond [:markh] from comment #18)
> Created attachment 8561245 [details] [diff] [review]
> 36/37 version
> 
> This patch had non-trivial conflicts in nsSearchService.js [...]
> due to bug 1117186 not being uplifted

Bug 1117186 has been approved for uplift, do we still need this branch patch?

Comment 20

[:Gavin Sharp [email: [e-mail]]]

(In reply to Mark Hammond [:markh] from comment #18)
> This patch had non-trivial conflicts in nsSearchService.js (eg, when we
> check locale="US" has changed and geoSpecificDefaultsEnabled() logic remains
> inline) due to bug 1117186 not being uplifted

We're uplifting that, right? Has approvals now. Is this still needed?

Comment 21

[Petruta Horea [:phorea], Desktop QA]

Can this be verified by QA? Thanks!

Comment 22

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8561245 [details] [diff] [review]
36/37 version

Going to assume not needed.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4c1cf4c0a988
https://hg.mozilla.org/releases/mozilla-beta/rev/fafd3abc1d01

Comment 24

[Mark Hammond [:markh] [:mhammond]]

Yeah - I thought we decided to not uplift bug 1117186 - I should have checked!

Comment 25

[Petruta Horea [:phorea], Desktop QA]

Mark, can you please provide us some details in order to verify this? We are reaching the end of the cycle and we should verify Firefox 36 too. Thank you!

Comment 26

[Mark Hammond [:markh] [:mhammond]]

(In reply to Petruta Rasa [QA] [:petruta] from comment #25)
> Mark, can you please provide us some details in order to verify this? We are
> reaching the end of the cycle and we should verify Firefox 36 too. Thank you!

* With a US timezone, start 35 in a new profile and ensure the pref browser.search.isUS is true and that Yahoo is the default engine.
* Upgrade to 36, wait a minute or so.
* Verify that the browser.search.region preference is "US".
* Verify that the browser.search.countryCode preference is set to some 2 character string which reflects the geoip lookup from your IP address (ie, this may *not* be "US")
* Verify the default search engine remains Yahoo.

Comment 27

[Petruta Horea [:phorea], Desktop QA]

Thanks for the clear steps, Mark!

I verified and obtained the expected results under Win 7 64-bit, Mac OSX 10.9.5 and Ubuntu 14.04 64-bit when updating from Firefox 35 beta x to Firefox 36 beta 9.

I wonder if I could use older Aurora and Nightly builds (2015-01-20) and upgrade to latest builds in order to verify 37 and 38 versions too.

Comment 28

[Petruta Horea [:phorea], Desktop QA]

Updates for Firefox 37 beta 1 are available so I could verify this version too.

Verified as fixed under Win 7 64-bit, Mac OSX 10.10 and Ubuntu 14.04 64-bit.

Comment 29

[Petruta Horea [:phorea], Desktop QA]

Verified as fixed by updating to Firefox 38 beta 1 under Win 7 64-bit, Mac OSX 10.9.5 and Ubuntu 12.04 32-bit.