1194265 - Some default search engines should only be visible in specific countries

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[Florian Quèze [:florian]]

Attachment: Patch v1

This is, per mconnor's last minute request, an extension to what we do with the absearch service.

The goal is to ship more default engines than we would show in each locale, and to hide/show them based on the user's country.

We need to have a local fallback in case we can't reach the server, and I remember localized prefs didn't work all that well when we launched Flare last year, because multi-locale builds (on Linux distributions) ended up with all locales using the en-US prefs.

Instead I've decided to add a :hidden suffix inside the list.txt file (which is in a localized folder) to annotate the engines that we ship, but don't show by default until the absearch server says so. I'm not fully set on the ':hidden' string, but it needs to be something parsable by gmake so that we don't break l10n packaging.

This has the benefit that I can put the code checking if we received an engine list from the server in the same function as the code parsing the list.txt file, and this makes the patch 'relatively self contained'.

The attached patch already works (I only tested briefly, and haven't really tested the sync init code path).

Some things would need to be through through some more, eg:
- what happens if the user attempts to install an engine that we already ship but hide
- are there edge cases related to distribution partner
- I haven't looked at anything related to ordering.
- This definitely needs tests. I can try to write some tomorrow if the feedback I get tonight indicates this is the best way forward.

Comment 1

[Mike Connor [:mconnor]]

Overall, this is pretty clean and straightforward, so I like it.  The idea is :hidden applies to the fallback case, but the server contents always win?

* I think order can wait, it's a pain with distro builds.
* We should test this with a real distro config to make sure it works.  Drop the contents of https://hg.mozilla.org/build/partner-repacks/file/tip/partners/bing/distribution/ into /distribution and start a new profile to test.  You can ignore /extensions for this, it's orthogonal.
* Just confirming, though I assume this to be the case: older clients will just ignore the new array, correct?

Comment 2

[Florian Quèze [:florian]]

(In reply to Mike Connor [:mconnor] from comment #1)
> Overall, this is pretty clean and straightforward, so I like it.  The idea
> is :hidden applies to the fallback case, but the server contents always win?

:hidden is used when we can't use the server value.
We won't have a server value if either:
- the server isn't reachable (or is taking too long to respond, in which case the server list will apply only after a restart)
- the list returned by the server contains an engine name for which we don't have an associated .xml file. In this case we ignore the whole list returned by the server.

> * I think order can wait, it's a pain with distro builds.
> * We should test this with a real distro config to make sure it works.  Drop
> the contents of
> https://hg.mozilla.org/build/partner-repacks/file/tip/partners/bing/
> distribution/ into /distribution and start a new profile to test.  You can
> ignore /extensions for this, it's orthogonal.

I'm not too worried about distribution builds, because my patch only messes with the way the list.txt file is handled.

> * Just confirming, though I assume this to be the case: older clients will
> just ignore the new array, correct?

Yes.

And new clients won't get a per-country list until the recheck interval expires. It's apparently currently set to 365 days for en-US/US and 30 days for en-US/<other country>.

Comment 3

[Florian Quèze [:florian]]

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

> Some things would need to be through through some more, eg:

another one: what happens if we start hiding an engine that the user has set as default. I think we'll just reset the user's default engine, which may not be super nice.

Comment 4

[Dave Townsend [:mossop]]

Comment on attachment 8647540 [details] [diff] [review]
Patch v1

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

This is good, particularly happy with how self-contained this is, this should help us keep our risk manageable.

::: browser/app/profile/firefox.js
@@ +370,5 @@
>  // This is disabled globally, and then enabled for individual locales
>  // in firefox-l10n.js (eg. it's enabled for en-US).
>  pref("browser.search.geoSpecificDefaults", false);
> +//pref("browser.search.geoSpecificDefaults.url", "https://search.services.mozilla.com/1/%APP%/%VERSION%/%CHANNEL%/%LOCALE%/%REGION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%");
> +pref("browser.search.geoSpecificDefaults.url", "data:application/json,{\"interval\": 31536000, \"settings\": {\"searchDefault\": \"Yahoo\", \"visibleDefaultEngines\": [\"yahoo\",\"eBay\",\"bmo\"]}}");

Assuming this is for testing

::: browser/locales/en-US/searchplugins/list.txt
@@ +1,2 @@
>  amazondotcom
> +bmo:hidden

Assuming this is for testing

::: toolkit/components/search/nsSearchService.js
@@ +562,5 @@
> +        engineMetadataService.getGlobalAttr("searchDefaultHash") == getVerificationHash(defaultEngine);
> +      if (validHash) {
> +        let visibleDefaultEngines =
> +          engineMetadataService.getGlobalAttr("visibleDefaultEngines");
> +        let validHash = !visibleDefaultEngines ||

Redeclaring the same variable can be confusing, use validVisibleHash

@@ +566,5 @@
> +        let validHash = !visibleDefaultEngines ||
> +          engineMetadataService.getGlobalAttr("visibleDefaultEnginesHash") == getVerificationHash(visibleDefaultEngines);
> +
> +        if (validHash) {
> +          // No geo defaults, or valid hashes; nothing to do.

We only check the hash if we have a setting for visibleDefaultEngines. Does this mean that someone can alter prefs to force us back to the shipped defaults fairly easily? Not sure there is much to gain there but maybe we could just always verify the hash here, I guess that means you'd have to generate one on first run though

@@ +3753,5 @@
>                         cache.buildID != buildID ||
>                         cachePaths.length != toLoad.length ||
>                         toLoad.some(notInCachePath) ||
> +                       cache.visibleDefaultEngines.length != this._visibleDefaultEngines.length ||
> +                       this._visibleDefaultEngines.some(notInCacheVisibleEngines) ||

There is a weird edge-case here where if visibleDefaultEngines contains duplicates then we don't end up rebuilding the cache when we should. It's probably safe to leave as-is but could be avoided by using sets for these instead.

@@ +4291,5 @@
>          LOG("_findJAREngines: failed to retrieve list.txt from " + listURL + ": " + ex);
>  
>          return;
>        }
> +    }, this);

You could avoid this by making the function a fat arrow function.

@@ +4367,5 @@
> +
> +  _parseListTxt: function SRCH_SVC_parseListTxt(list, root, jarPackaging,
> +                                                chromeFiles, uris) {
> +    let names = list.split("\n").filter(function (n) !!n);
> +    let jarNames = new Map();

Add a comment that this maps from engine name to hidden

@@ +4390,5 @@
> +        // If all engineName values are part of jarNames,
> +        // then we can use the country specific list, otherwise ignore it.
> +        if (!jarNames.has(engineName)) {
> +          LOG("_parseListTxt: ignoring visibleDefaultEngines value because " +
> +              engineName + " is not in the jar engines we have found");

I'm a little worried by this. Why wouldn't we just return the intersection of engineNames and jarNames?

@@ +4418,5 @@
> +      }
> +    }
> +
> +    // Store this so that it can be used while writing the cache file.
> +    this._visibleDefaultEngines = engineNames;

Do we need to update the hash here?

Comment 5

[Florian Quèze [:florian]]

(In reply to Dave Townsend [:mossop] from comment #4)

> ::: toolkit/components/search/nsSearchService.js
> @@ +562,5 @@
> > +        engineMetadataService.getGlobalAttr("searchDefaultHash") == getVerificationHash(defaultEngine);
> > +      if (validHash) {
> > +        let visibleDefaultEngines =
> > +          engineMetadataService.getGlobalAttr("visibleDefaultEngines");
> > +        let validHash = !visibleDefaultEngines ||
> 
> Redeclaring the same variable can be confusing, use validVisibleHash

I meant to keep using it, the 'let' there is a copy/paste accident. This code has been changed a couple times today, given the little that remains I could as well simplify and just make a larger if.

> 
> @@ +566,5 @@
> > +        let validHash = !visibleDefaultEngines ||
> > +          engineMetadataService.getGlobalAttr("visibleDefaultEnginesHash") == getVerificationHash(visibleDefaultEngines);
> > +
> > +        if (validHash) {
> > +          // No geo defaults, or valid hashes; nothing to do.
> 
> We only check the hash if we have a setting for visibleDefaultEngines. Does
> this mean that someone can alter prefs to force us back to the shipped
> defaults fairly easily?

Yes. Our shipped defaults should always be fine, and shouldn't help at all something attempting to hijack us.

> @@ +3753,5 @@
> >                         cache.buildID != buildID ||
> >                         cachePaths.length != toLoad.length ||
> >                         toLoad.some(notInCachePath) ||
> > +                       cache.visibleDefaultEngines.length != this._visibleDefaultEngines.length ||
> > +                       this._visibleDefaultEngines.some(notInCacheVisibleEngines) ||
> 
> There is a weird edge-case here where if visibleDefaultEngines contains
> duplicates

If visibleDefaultEngines contains duplicates, the server (which we control) is broken.

> then we don't end up rebuilding the cache when we should. It's
> probably safe to leave as-is but could be avoided by using sets for these
> instead.

I'll see if I can do that easily.

> @@ +4390,5 @@
> > +        // If all engineName values are part of jarNames,
> > +        // then we can use the country specific list, otherwise ignore it.
> > +        if (!jarNames.has(engineName)) {
> > +          LOG("_parseListTxt: ignoring visibleDefaultEngines value because " +
> > +              engineName + " is not in the jar engines we have found");
> 
> I'm a little worried by this. Why wouldn't we just return the intersection
> of engineNames and jarNames?

The only reason for visibleDefaultEngines to contain engines that we don't have is that the server is confused about which version of Firefox we are running. In this case, if the value returned by the server is broken, I think it's safer to ignore it completely.

> @@ +4418,5 @@
> > +      }
> > +    }
> > +
> > +    // Store this so that it can be used while writing the cache file.
> > +    this._visibleDefaultEngines = engineNames;
> 
> Do we need to update the hash here?

No. The hash and the cache are purposefully updated at different times. The cache reflects what we have loaded (last time). The hash secures what we should be loading next time. Usually these are the same, but if we timeout (after 2s) while waiting for the server's response, we'll store in the cache something that is based on the local fallback, and the hash will be related to the value the server has eventually returned.

Comment 6

[Florian Quèze [:florian]]

Attachment: Patch v2

Addressed Dave's comment 4, except for the suggestions of using set to avoid duplicates (the cache invalidation has enough other things that I find unfortunate that I would rather clean that up in a separate patch that will ride the trains). I added comments in a few places to clarify in the code the things that we discussed in the above bug comments. I also fixed existing tests.

Comment 7

[Florian Quèze [:florian]]

Attachment: Patch v3 (with tests)

Comment 8

[Dave Townsend [:mossop]]

Comment on attachment 8648094 [details] [diff] [review]
Patch v3 (with tests)

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

Do we need a check verifying the behaviour if visibleDefaultEngines is missing from the server response?

Comment 9

[Florian Quèze [:florian]]

(In reply to Dave Townsend [:mossop] from comment #8)

> Do we need a check verifying the behaviour if visibleDefaultEngines is
> missing from the server response?

I haven't modified the server responses in test_geodefaults.js, so we already have coverage for this case.

Comment 10

[Dave Townsend [:mossop]]

Attachment: Fix test_json_cache.js

This fixes test_json_cache.js to always load the visible engines from the list even in packaged builds. Running on try now but this passes in both packaged and unpackaged builds locally now.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=82d9aea4b48f

Comment 11

[Dave Townsend [:mossop]]

Attachment: Fix test_json_cache.js

Comment 12

[Florian Quèze [:florian]]

Comment on attachment 8648239 [details] [diff] [review]
Fix test_json_cache.js

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

Thanks for figuring this out! :)

::: toolkit/components/search/tests/xpcshell/test_json_cache.js
@@ +34,5 @@
>  
>  let cacheTemplate, appPluginsPath, profPlugins;
>  
> +function loadList(root) {
> +  let engines = [];

You can remove this variable which doesn't seem to actually be used.

@@ +93,5 @@
>    } else {
>      let rootURIPref = defaultBranch.getCharPref("jarURIs");
>      let rootURIs = rootURIPref.split(",");
>      for (let root of rootURIs) {
> +      visibleDefaultEngines = loadList(root);

The separate function doesn't really help readability. Could you just moved that block of code to here outside the if/else?

Comment 13

[Florian Quèze [:florian]]

Looks like there's another failure that needs to be investigated, on Windows:
 TEST-UNEXPECTED-TIMEOUT | toolkit/components/search/tests/xpcshell/test_hidden.js | Test timed out

Comment 14

[Dave Townsend [:mossop]]

Hmm, looks like some file lock problem:

 15:18:41 INFO - PROCESS | 3832 | *** Search: metadata writeCommit: start
15:18:41 INFO - PROCESS | 3832 | *** Search: metadata writeCommit: path c:\\docume~1\\cltbld~1.t-x\\locals~1\\temp\\xpc-profile-44l6vy\\search-metadata.json
15:18:41 INFO - "CONSOLE_MESSAGE: (info) metadata writeCommit: start"
15:18:41 INFO - "CONSOLE_MESSAGE: (info) metadata writeCommit: path c:\\\\docume~1\\\\cltbld~1.t-x\\\\locals~1\\\\temp\\\\xpc-profile-44l6vy\\\\search-metadata.json"
15:18:41 INFO - "CONSOLE_MESSAGE: (error) [JavaScript Error: "Win error 5 during operation move on file c:\\\\docume~1\\\\cltbld~1.t-x\\\\locals~1\\\\temp\\\\xpc-profile-44l6vy\\\\search-metadata.json.tmp (Access is denied.
15:18:41 INFO - )" {file: "resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js" line: 937}]"

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/462b94db3335

Comment 16

[Florian Quèze [:florian]]

Attachment: Patch v4 (as landed)

I integrated Mossop's test fix with a few tweaks (I noticed it didn't support having more than one value in the jarURIs pref), and fixed the Windows timeout by removing the |yield commitPromise;| line at the end of the test_hidden.js file (this line wasn't really needed, it was just there to potentially help if we want to add more tasks to the test later). It was green on try (https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f73cc88db26) so I pushed it to fx-team.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/462b94db3335

Comment 18

[Dave Townsend [:mossop]]

Comment on attachment 8648355 [details] [diff] [review]
Patch v4 (as landed)

Approval Request Comment
[Feature/regressing bug #]: Search defaults
[User impact if declined]: We won't be able to provide the correct search defaults when users are using a locale different to their geographic region
[Describe test coverage new/current, TreeHerder]: On m-c with good automated tests running
[Risks and why]: This is fairly low risk, we have good tests and the code implementing this is pretty well contained. There are a few potential corner cases that could be a problem but they would have to be triggered by (and could be corrected by) bad data on the server side.
[String/UUID change made/needed]: None

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8648355 [details] [diff] [review]
Patch v4 (as landed)

Approved for uplift to Aurora and Beta as this is a critical ask. It has been on m-c for 2 days so should be safe.

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Requesting QE team to do a sanity test (if possible) on this one.

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/aea52aa8c59a

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/99ba19fed20a

Comment 23

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

(In reply to Ritu Kothari (:ritu) from comment #20)
> Requesting QE team to do a sanity test (if possible) on this one.

Comment 18 states this has good automation tests. What should we cover on the manual testing side for this fix? 

Note that we've already planned for tomorrow to do some basic regression testing for Search (changing providers and performing searches) with Firefox 41 beta 3.

Comment 24

[Dave Townsend [:mossop]]

(In reply to Florin Mezei, QA (:FlorinMezei) from comment #23)
> (In reply to Ritu Kothari (:ritu) from comment #20)
> > Requesting QE team to do a sanity test (if possible) on this one.
> 
> Comment 18 states this has good automation tests. What should we cover on
> the manual testing side for this fix? 
> 
> Note that we've already planned for tomorrow to do some basic regression
> testing for Search (changing providers and performing searches) with Firefox
> 41 beta 3.

Since most of this is server-side controlled it is difficult to manually test at this point so I'd say as long as we are doing those basic tests you mention we should be good.

Comment 25

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Basic Regression Testing was performed today using Firefox 41 Beta 3, on multiple platforms (see https://docs.google.com/spreadsheets/d/1rQS6fGgzlWHfk3zDbe-6gK5ESirvPCI6YcESlpg2Sk0/edit#gid=356627606).

No new issues were encountered, so Search still looks good.

Removing the qe-verify+ flag.