1254694 - Search engines: keywords and order gone after upgrading from 44.0.2 to 45.0 with language pack active

Status: RESOLVED FIXED

(Firefox :: Search, defect, P1)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

After upgrading from Firefox 44.0.2 64 bit to 45.0 64 bit on a Windows 8.1 machine, my search engines have all lost their keywords and their order (it's alphabetic now). I wasn't able to reproduce with a new profile created in 44.0.2 and launching that in 45.0 but will test with a copy of my backup tomorrow.

Further information:
- The profile is very old. But this shouldn't matter since the search engines got migrated from files to a database like structure.
- Some of the search engines got added with the extension 'Add To Search Bar': https://addons.mozilla.org/firefox/addon/add-to-search-bar/
- amazon.de is twice in my list of search engines (might have been done by myself): Once as 'amazon.de', once as 'Amazon.de'

Comment 1

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Steps to reproduce:
1. Create a new profile in Firefox 44.0.2.
2. Install the German language pack from https://ftp.mozilla.org/pub/firefox/releases/44.0.2/win32/xpi/de.xpi
3. Open about:config.
4. Set general.useragent.locale to 'de'
5. Close and relaunch Firefox.
6. Open search settings.
7. Set keyword for Bing search to 'bg'.
8. Close Firefox.
9. Launch profile in Firefox 45.0 and accept to upgrade for the language pack.
10. Open search settings.
Actual result:
Search keyword for Bing is gone (search engines have still their order). If I reorder the search engines in step 7, they will be order alphabetically after step 10.
There is no error in the Browser Console.
If the language pack doesn't get updated in step 9, keyword and order are untouched after step 10.
Expected result:
Search engines unaltered.

See also bug 1236347 which is also about issues with language packs starting in Firefox 45.

Comment 2

[Sylvestre Ledru [:Sylvestre]]

Florian, any clue? Merci

Comment 3

[Florian Quèze [:florian]]

This is likely a side effect of bug 1203167 (large search service refactoring that landed for Firefox 45).

Comment 4

[:shell escalante]

will demote if appropriate after understand what is happening and how large the impact - scope.

Comment 5

[:shell escalante]

Florian, just passing on some information from JorgeV as others noticed this bug being flagged as well
 
<jorgev> this should give you a rough idea https://addons.mozilla.org/en-US/firefox/search/?appver=45.0&atype=5
<jorgev> potentially millions
<kev> yeah, don't know how many people change order of searches and/or use keywords
 — eviljeff suspects few of the former and very few for the latter
 <kev> same but should be evaluated, all the same
<jorgev> also, it looks like the real current usage of these language packs is much higher than the average

Comment 6

[Kohei Yoshino]

:aryx pointed me to this bug. I'm using the en-GB locale build with no language packs installed.

Comment 7

[Florian Quèze [:florian]]

(In reply to Kohei Yoshino [:kohei] from comment #6)
> :aryx pointed me to this bug. I'm using the en-GB locale build with no
> language packs installed.

Do you have specific steps to reproduce that don't involve a language pack?

Comment 8

[Kohei Yoshino]

I have just upgraded from 44.0.2 to 45. And I don't have search engine related extensions neither. No extra steps.

My default search provider is DuckDuckGo that remains as-is. The order of other search providers, Google was first in my case, was lost, and some of my disabled search providers, Bing and Chanbers (UK), were enabled once upgraded.

Comment 9

[Florian Quèze [:florian]]

(In reply to Kohei Yoshino [:kohei] from comment #8)
> I have just upgraded from 44.0.2 to 45. And I don't have search engine
> related extensions neither. No extra steps.

I can't reproduce, so there must be a detail or a step missing.

> some of
> my disabled search providers, Bing and Chanbers (UK), were enabled once
> upgraded.

What do you mean with "enabled"?

Comment 10

[Florian Quèze [:florian]]

Attachment: Fix v1

One-line fix. I'll try to write a test.

Comment 11

[Florian Quèze [:florian]]

Attachment: Fix for synchronous code path

Someone very helpfully emailed me the relevant parts of a profile that could reproduce the issue without lang pack involved. This profile contains lots of user-installed engines (42), and for some reason the synchronous initialization code path gets triggered. Turns out that code path isn't currently covered by tests, and had trivial mistakes.

Comment 12

[Kohei Yoshino]

(In reply to Florian Quèze [:florian] [:flo] from comment #9)
> > some of
> > my disabled search providers, Bing and Chanbers (UK), were enabled once
> > upgraded.
> 
> What do you mean with "enabled"?

Those providers have appeared in the search bar's drop down list.

I have "user-installed engines" but only 4, including Bugzilla@Mozilla (this site), FxSiteCompat.com (my site), MDN and GitHub.

Comment 13

[Florian Quèze [:florian]]

Do you have an add-on enabled that forces a synchronous initialization of the search service? (This causes a warning in the browser console)

If you would like me to try to reproduce with your profile, you can email me these files from the profile:
- search.json
- search-metadata.json
- the searchplugins/ folder

Comment 14

[Florian Quèze [:florian]]

Attachment: Fix synchronous code path v2 (with test)

Note: Surprisingly, the patch was developped against mozilla-release, but also applies cleanly to the current fx-team.

Comment 15

[Florian Quèze [:florian]]

Attachment: Fix for engines from language packs, v2 (with test)

Comment 16

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=324cc19768c3

Comment 17

[Egon Upitis]

OK, the loss of order and keywords happened before -- annoying but I have already made a txt list that helps me to restore it manually.

However with this latest update (v45.0) I cannot even edit the keywords! There's no option for that. Extremely annoying as I do a lot of research and use them hundreds of times every day.

How is this related to a language pack anyway? I am using English Mozilla.. I think I only have additional Russian spell-checking dictionary installed.

Florian, can I use the fixes you provided and how?

Comment 18

[Florian Quèze [:florian]]

(In reply to Egon Upitis from comment #17)

> However with this latest update (v45.0) I cannot even edit the keywords!
> There's no option for that.

See bug 1138112.

> How is this related to a language pack anyway? I am using English Mozilla..
> I think I only have additional Russian spell-checking dictionary installed.

If you think you are encountering a different case I may not have identified yet, and would like me to try to reproduce with the search settings of the profile, you can email me the relevant files (see comment 13).

> Florian, can I use the fixes you provided and how?

Not until they are released.

Comment 19

[Egon Upitis]

Thanks, Florian -- I thought I tried everything, glad the keywords still work.

I think my case is the same as https://bugzilla.mozilla.org/show_bug.cgi?id=1254694#c11 -- lots of search engines installed. In fact I am now removing some that I don't ever use.

Comment 20

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/eed38188ee0ef70c90db4767dd614508a1a9042c
Bug 1254694 - Fix synchronous code path for search engines metadata migration, r=adw.

https://hg.mozilla.org/integration/fx-team/rev/0837015740ff57ed9c40a6733987f437985b9d9b
Bug 1254694 - Fix search engines metadata migration for engines from language packs, r=adw.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/eed38188ee0e
https://hg.mozilla.org/mozilla-central/rev/0837015740ff

Comment 22

[Florian Quèze [:florian]]

Comment on attachment 8728957 [details] [diff] [review]
Fix synchronous code path v2 (with test)

Approval Request Comment
[Feature/regressing bug #]: bug 1203167
[User impact if declined]: users with lots of user-installed search plugins are likely to lose search engine metadata (aliases, order, removed default engine).
[Describe test coverage new/current, TreeHerder]: the patch contains an xpcshell test.
[Risks and why]: very low, the patch fixes trivial mistakes in a code path that isn't used in the general case, and that's wrapped in a try/catch block. The code touched by the patch is completely broken without the patch.
[String/UUID change made/needed]: none

Comment 23

[Florian Quèze [:florian]]

Comment on attachment 8729028 [details] [diff] [review]
Fix for engines from language packs, v2 (with test)

Approval Request Comment
[Feature/regressing bug #]: bug 1203167
[User impact if declined]: users using a language pack are likely to lose search engine metadata (aliases, order, removed default engine).
[Describe test coverage new/current, TreeHerder]: the patch contains an xpcshell test.
[Risks and why]: low; one line fix, modifying the engine id, which is only ever used during metadata migration.
[String/UUID change made/needed]: none

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8728957 [details] [diff] [review]
Fix synchronous code path v2 (with test)

Fix a migration issue, taking it on all branches.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5b899aff5f55
https://hg.mozilla.org/releases/mozilla-aurora/rev/83559d0c94aa

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6f9cd7e53af2
https://hg.mozilla.org/releases/mozilla-beta/rev/67c6886836d1

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/6b27b0a8ade6
https://hg.mozilla.org/releases/mozilla-release/rev/156844d61287

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/6b27b0a8ade6
https://hg.mozilla.org/releases/mozilla-esr45/rev/156844d61287

Comment 29

[Andrei Vaida [:avaida]]

We should perform further Exploratory Testing around this scenario.

Comment 31

[Sylvestre Ledru [:Sylvestre]]

Added to the release notes with "Fix a regression causing search engine metadata to be lost in some context (1254694)" as wording

Comment 32

[Florian Quèze [:florian]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #31)
> Added to the release notes with "Fix a regression causing search engine
> metadata to be lost in some context (1254694)" as wording

"metadata" seems a bit jargon-ish to me, maybe "settings" instead would be more understandable?

Comment 33

[Sylvestre Ledru [:Sylvestre]]

Right, updated, merci!

Comment 34

[Adrian Florinescu [:aflorinescu]]

I. 
Initial Scenario:  Using steps from comment 1 and referring only to the update scenario which breaks search settings.
44.02 (update to) 45.0
44.02 (update to) 45.0.1
45ESR (update to) 45.0.1 ESR

Windows10 -> reproducible
Mac -> not reproducible
Ubuntu -> reproducible

The update/migration bug can be verified fixed on WINDOWS and UBUNTU for 45.0.1, 45.0.1 ESR with the note that (comment 1) wasn't reproducible on MAC. 

II. 
What I noticed for all OS'es was that installing the language pack and setting the " general.useragent.locale to 'de' ", then just restarting FF was causing the search engines settings to break. (behavior reproducible for 44.02, 45, 45.01, 45.01ESR)


I also went back a bit and checked if I. and II. scenarios are reproducible for older releases: 42.0 and 43.0.1 with the following result: I. No  II. Yes.
Please advise if scenario II. should be split up into a separate issue.

Comment 35

[Florian Quèze [:florian]]

(In reply to Adrian Florinescu [:AdrianSV] from comment #34)

> II. 
> What I noticed for all OS'es was that installing the language pack and
> setting the " general.useragent.locale to 'de' ", then just restarting FF
> was causing the search engines settings to break. (behavior reproducible for
> 44.02, 45, 45.01, 45.01ESR)
> 
> 
> I also went back a bit and checked if I. and II. scenarios are reproducible
> for older releases: 42.0 and 43.0.1 with the following result: I. No  II.
> Yes.
> Please advise if scenario II. should be split up into a separate issue.

Unless I'm misunderstanding something, II. may not be a real issue. When you switch to a different language pack, you switch to a different set of default search engines, so settings can't really be kept, or at least not all of them. We may want to try to preserve the settings automatically for engines that have the same name for both languages (eg. 'Google'), but that's not something we should attempt to do in a dot release. Us losing some settings when the user actively switches back and forth between languages is very different from (and less problematic than) losing settings during an update.

Comment 36

[Adrian Florinescu [:aflorinescu]]

I agree that the scope of this bug was to track and fix the version update issue that was ruining the search settings. In regards to it, the fix can be marked as verified;
I can also confirm that the bug does not regress on build 2 for 45.0.1 ESR:

45.0.1 Build-ID  20160316151906 User-Agent  Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0
45.0.1Build-ID 20160316151906 Mozilla/5.0 (X11; Linux i686; rv:45.0) Gecko/20100101 Firefox/45.0
45.0.1 Build-ID 20160316151906 User-Agent Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:45.0) Gecko/20100101 Firefox/45.0

Adding the above to comment 34 /point I., we can mark this issues as verified on: Ubuntu14.04/Mac10.11/Windows10 for Release 45.0.1 and 45.0.1ESR (build1+build2)