1583383 - Failure of 56 tests from toolkit/components/search/tests/xpcshell/ when run in the Thunderbird environment on 2019-09-23

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/components/search/tests/xpcshell/test_nocache.js | xpcshell return code: 0
and 55 more.

So I ran that test manually and saw:
ERROR Unexpected exception NS_ERROR_FAILURE: SearchService initialization failed
init@resource://gre/modules/SearchService.jsm:2119:24
async*test_nocache@c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/_tests/xpcshell/toolkit/components/search/tests/xpcshell/test_nocache.js:24:16

So Services.search.init() throws in line 2119
https://searchfox.org/mozilla-central/source/toolkit/components/search/SearchService.jsm#2092

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2b8364cfdb04bfdfde7c01589e48e24973&tochange=07eb2cc7e1c31f2bc6ae24ade3847f39e1

What do we need to do to get going again? I see that FF search now uses an engines.json file introduced in bug which we don't have.

Comment 1

[Mark Banner (:standard8)]

(In reply to Jorg K (GMT+2) from comment #0)

What do we need to do to get going again? I see that FF search now uses an engines.json file introduced in bug which we don't have.

Sorry, I've been meaning to get around to filing a bug for you and I forgot that this would impact.

We are currently rewriting the configuration management for search engines, and moving it to remote settings - for several benefits, a) reduce the complexity of the current system, b) have more flexibility in how engines get deployed per locale/region (the current config specifies per locale, which is a lot of work to maintain, especially with region variations).

This work is aimed at 71. Bug 1542235 is the tracker, and there's some docs available. We are still working towards the final format at the moment, so you probably want to wait a couple more weeks until you pick it up.

Probably during the 73 cycle (maybe before, but I doubt it), we'll be removing the code associated with the old configuration.

Initially you could probably just skip that xpcshell.ini file, but you'll want to enable it at some stage soon.

Comment 2

[Mark Banner (:standard8)]

Opps pressed submit too soon.

Just wanted to additionally note that although our version is being served by remote settings, we'll have some way so that you can either ship a built-in file, or pick up a different config remotely (I haven't figured out exactly what is best to do there yet).

Comment 3

[Jorg K (CEST = GMT+2)]

I have a hack that will fix the tests ;-)

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7d95be5ef5a5
temporarily use the default search extensions from mozilla-central. rs=bustage-fix

Comment 5

[Mark Banner (:standard8)]

Please work to revert that soon - Thunderbird isn't allowed to use the same codes as Firefox.

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: 1583383-skip-search-tests-for-thunderbird.patch

OK, I'll back that out when this patch lands.

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: 1583383-provide-engines-json.patch [landed in comment #9]

Instead of swtiching tests off, I'd really prefer to follow Firefox around and keep them running. Here's a copy of engines.json with eBay removed. The first test I tried, test_nocache.js, passes, but 16 (or more?) others still fail:
toolkit/components/search/tests/xpcshell/test_SearchStaticData.js
toolkit/components/search/tests/xpcshell/test_addEngine_callback.js
toolkit/components/search/tests/xpcshell/test_async.js
toolkit/components/search/tests/xpcshell/test_async_addon.js
toolkit/components/search/tests/xpcshell/test_async_distribution.js
toolkit/components/search/tests/xpcshell/test_geodefaults.js
toolkit/components/search/tests/xpcshell/test_migrateWebExtensionEngine.js
toolkit/components/search/tests/xpcshell/test_multipleIcons.js
toolkit/components/search/tests/xpcshell/test_purpose.js
toolkit/components/search/tests/xpcshell/test_resultDomain.js
toolkit/components/search/tests/xpcshell/test_searchSuggest.js
toolkit/components/search/tests/xpcshell/test_searchSuggest_cookies.js
toolkit/components/search/tests/xpcshell/test_sendSubmissionURL.js
toolkit/components/search/tests/xpcshell/test_validate_engines.js
toolkit/components/search/tests/xpcshell/test_validate_manifests.js
toolkit/components/search/tests/xpcshell/test_webextensions_install.js

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=2cfc7be190f3be294bad6e237253ddb2ce1bd7ad

That can have various reasons, for example, TB doesn't have the the google-b-d stuff. It might be worth reading through bug 1544013 to see what the differences are.

Any advice of what we need to change/strip would be most welcome.

Comment 8

[Jorg K (CEST = GMT+2)]

Another issue could be that the engines.json file now contains all FF locales, for example also "ca-valencia" which TB doesn't have. So that would need to be removed, I suppose.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/27d3d9011a13
Backed out changeset 7d95be5ef5a5 to return to Thunderbird's own set of search engines. a=backout
https://hg.mozilla.org/comm-central/rev/27b964c4d531
copy engines.json from Firefox and adjust accordingly (remove eBay). rs=bustage-fix DONTBUILD

Comment 10

[Jorg K (CEST = GMT+2)]

That try passed miraculously although tests fell over locally, strange. I've landed it now to honour comment #5 and not ship "the same codes as Firefox" even in a Daily.

Apart from removing eBay, I left the file as it was, but I'm more than happy to do further clean-up.

Comment 11

[Mark Banner (:standard8)]

You'll need to go through your engines.json and match it with your existing list.json - I suspect our engines may have diverged over time, and you may find you don't have some locales for some engines.

You'll also need to go through the various search engines and check your params match with the configuration, or remove the Firefox codes that are there.

Comment 12

[Jorg K (CEST = GMT+2)]

Thanks for the comment.

Right now the tests pass and I assume we're not using "the same codes as Firefox", or are we via the engines.json?

I'll work on a clean-up and present it for review. I last synced search in bug 1427133 (Jan 2019) and then a bit more in bug 1544013 comment #37 and further down (May 2019).

Mark, maybe you could go through the patch and tell me what exactly needs to be removed. Stuff like
"client": "firefox-b-d"
"tag": "firefox-uk-21" and anything else with "firefox"?
"telemetryId": "google-b-d"
"pc": "MOZI" (Partner code?)
and of course ca-valencia which we don't have.

Overall, this looks like a doable exercise.

Another question: What about Dale's availability? Away until Jan 2020 yet landing patches ;-)

Comment 13

[Mark Banner (:standard8)]

(In reply to Jorg K (GMT+2) from comment #12)

Right now the tests pass and I assume we're not using "the same codes as Firefox", or are we via the engines.json?

You'd be using them if the new configuration was turned on, but it isn't for now.

I'll work on a clean-up and present it for review. I last synced search in bug 1427133 (Jan 2019) and then a bit more in bug 1544013 comment #37 and further down (May 2019).

I have no idea if Thunderbird has different permissions to Firefox for including search engines, nor do I know what the the differences between the two should be. What I believe is that you at least need permission to include various search engines because of trademarks/logos.

Mark, maybe you could go through the patch and tell me what exactly needs to be removed. Stuff like
"client": "firefox-b-d"
"tag": "firefox-uk-21" and anything else with "firefox"?
"telemetryId": "google-b-d"
"pc": "MOZI" (Partner code?)
and of course ca-valencia which we don't have.

You need to match all parameters with what you have in your existing settings. Remove anything extra that isn't in your existing settings.

So for instance, with Google, you currently have this:

https://searchfox.org/comm-central/rev/26afcebfc783e24ac2f0bd8a7a527e2124cd3818/mail/components/search/extensions/google/manifest.json#23

"search_url_get_params": "q={searchTerms}",

So you need to translate that into the new engine.json file:

"searchUrlGetParams": {
  "q": "{searchTerms}"
},

On the Google front, you also don't have the split across locales like Firefox does. So you could simplify the appliesTo section to something like:

"appliesTo": [{
  "included": { "everywhere": true },
  "default": "yes-if-no-other"
}]

extraParams you should probably just remove unless you've got a params section (for instance here is the mozilla-central bing section that Thunderbird doesn't have).

telemetryId you probably don't need unless you're actually going to start monitoring it via telemetry. Preferably, you don't want to monitor that specific value anyway (I'm hoping to eventually migrate our metrics folks off that one and onto something better, but it is likely to take a while, hence why it is here for now).

webExtensionLocales also probably needs checking to ensure they exist if referenced.

It doesn't hurt if you specify extra locales that Thunderbird doesn't have. Though it might obviously get a bit confusing for maintenance.

Another question: What about Dale's availability? Away until Jan 2020 yet landing patches ;-)

Dale is now fully away, he was just finishing a couple of in-flight patches.

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: 1583383-cleanup-engines-json.patch

OK, here goes:

• removed extraParams (Yandex, DDG, Bing)
• removed pc MOZI from Bing
• sed -i -e '/telemetryId/d' engines.json and fixed commas in preceding lines :-(
• removed tags and client with firefox
• simplified Google appliesTo as suggested
• removed "tn": "monline_7_dg" from Baidu since it wasn't in the WE
• "tag": "mozillajapan-fx-22" and "tag": "mozilla-20" from Amazon, note sourceid=Mozilla-search is in the WEs

That should be pretty good.

Checked valid JSON here: https://jsonformatter.curiousconcept.com/

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=33d6880e74bec0970d22f0fc1ad265b4c33aa7ae

As for the legal stuff: We added all those engines in bug 1427133, Mike Kaply was involved, it all got shipped in TB 68 ESR and no one complained.

Comment 15

[Jorg K (CEST = GMT+2)]

Comment on attachment 9095305 [details] [diff] [review]
1583383-cleanup-engines-json.patch

Mark, you want to look at this? The try is green. So I don't think we're doing any damage to Firefox any more after this clean-up.

Comment 16

[Jorg K (CEST = GMT+2)]

Comment on attachment 9095305 [details] [diff] [review]
1583383-cleanup-engines-json.patch

Looks like Mark is busy elsewhere. Maybe we should build up some internal knowledge as far as search engines are concerned.

The context is that I copied the file from the browser 1:1 (and removed eBay which TB doesn't have), so this is the clean-up.

Comment 17

[Magnus Melin [:mkmelin]]

Comment on attachment 9095305 [details] [diff] [review]
1583383-cleanup-engines-json.patch

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

I'm not sure we want to remove telemetryId. That could actually be useful, once we get Telemetry going.

Comment 18

[Jorg K (CEST = GMT+2)]

So will you take a patch with the telemetryId left in place? I don't think they make sense now. Once we have agreements with search providers, we can add telemetry for those. I can't see us doing contracts with Google, Amazon or Baidu.

Comment 19

[Magnus Melin [:mkmelin]]

I assume the id is there to know how much the engines are used. For Thunderbird, getting any deals is hard without any sense of how much search engines are used.
Yeah, r=me without the telemetryId removal

Comment 20

[Jorg K (CEST = GMT+2)]

OK, but you're sure you want to distinguish all these:
Line 133: - "telemetryId": "amazondotcom",
Line 156: - "telemetryId": "amazon-au",
Line 173: - "telemetryId": "amazon-ca",
Line 190: - "telemetryId": "amazon-france",
Line 214: - "telemetryId": "amazon-en-GB",
Line 237: - "telemetryId": "amazon-in",
Line 244: - "telemetryId": "amazon-france",
Line 259: - "telemetryId": "amazon-ca",
Line 264: - "telemetryId": "amazon-ca",
Line 269: - "telemetryId": "amazon-jp",
Line 281: - "telemetryId": "amazon-it",
Line 293: - "telemetryId": "amazon-de",
Line 312: - "telemetryId": "amazon-en-GB",
Line 332: - "telemetryId": "amazon-au",
Line 342: - "telemetryId": "amazondotcn",

Line 432: - "telemetryId": "yandex-en"
Line 437: - "telemetryId": "yandex-az"
Line 443: - "telemetryId": "yandex-by"
Line 449: - "telemetryId": "yandex-kk"
Line 455: - "telemetryId": "yandex-ru"
Line 460: - "telemetryId": "yandex-tr"

The id's for Google are also wrong since we don't do "google-b-d", so that would have to change to "google".

IMHO it would make more sense to remove those and then add them back in a controlled fashion for TB. There was talk of doing a deal with a search engine for which we wouldn't collect telemetry right now, so this area will have to be revisited anyway.

Comment 21

[Magnus Melin [:mkmelin]]

Well the ids are just ids, it doesn't matter what they are. I don't see why the point of using different values for Thunderbird, since the telemetry data is per product anyway. Also don't see why we wouldn't collect usage numbers - like I said, lack of those is a more or less a blocker for progress in cutting any deals.

Comment 22

[Mark Banner (:standard8)]

Depending on what you're doing for telemetry reporting, you might not need the telemetry ID.

At the moment, we're reporting:

• defaultSearchEngine
  • This is the field which gets filled out by the telemetry id.
• defaultSearchEngineData.name
  • This is the visible name of the search engine.
• defaultSearchEngineData.submissionURL
  • This is the actual submission URL.
• and a couple of others.

defaultSearchEngine was the original, and so Firefox's reporting dashboards & flows are all set up around that.

However, defaultSearchEngineData.submissionURL (and defaultSearchEngineData.name) actually contain all that information in a much better and more accurate manner, as you're definitely getting the codes that are used by the client if you're interested in monitoring those. I have seen cases in the wild where defaultSearchEngine indicates something different to the submissionURL, so it is worth checking.

Ideally, once we've got modernisation complete, I'm going to push to drop defaultSearchEngine completely. So it is probably worth not relying on that now, and build the dashboards with the newer data. If there's something missing, then we can take another look.

Comment 23

[Mark Banner (:standard8)]

Comment on attachment 9095305 [details] [diff] [review]
1583383-cleanup-engines-json.patch

Sorry, been busy trying to get things done & also been off sick. I've not done extensive checks here, but this generally looks like the right idea. I'll let Magnus do the final sign-off.

Comment 24

[Jorg K (CEST = GMT+2)]

Attachment: 1583383-cleanup-engines-json.patch

Same clean-up only with the telemetry IDs left in place.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/40eba987b24d
Remove Firefox specific information from engines.json. r=mkmelin