Deleted search engines from a language pack reappear after an update

VERIFIED FIXED in Firefox 63

Status

defect
P2
normal
VERIFIED FIXED
3 years ago
a month ago

People

(Reporter: adam.morris, Assigned: gandalf)

Tracking

Trunk
mozilla64
x86_64
Linux
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox63 fixed, firefox64 verified)

Details

Attachments

(3 attachments)

(Reporter)

Description

3 years ago
User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20160919122641

Steps to reproduce:

Preferences→Search→Remove several 'default' search engines (like Yahoo, Amazon, Chambers)


Actual results:

The removed search engines are automatically restored some time later (likely after updates).


Expected results:

The removed search engines should stay removed.
(Reporter)

Updated

3 years ago
Component: Untriaged → Search
OS: Unspecified → Linux
Hardware: Unspecified → x86_64
(In reply to adam.morris from comment #0)
> The removed search engines are automatically restored some time later
> (likely after updates).

Florian, are default engines that the user has removed restored when they update Firefox?
Flags: needinfo?(florian)
(In reply to Drew Willcoxon :adw from comment #1)
> (In reply to adam.morris from comment #0)
> > The removed search engines are automatically restored some time later
> > (likely after updates).
> 
> Florian, are default engines that the user has removed restored when they
> update Firefox?

That's not the expected behavior, no.

If this happens, there's likely something messing with the user profile on the disk. On Fedora I don't expect hijacking, but that could still be for example a backup script. I think we also still have some brokenness when there's an update that causes a language pack to be briefly disabled before it's re-enabled after being updated too.

Adam, do you use Firefox in en-US, or do you have a language pack installed?
Flags: needinfo?(florian) → needinfo?(adam.morris)
(Reporter)

Comment 3

3 years ago
(In reply to Florian Quèze [:florian] [:flo] from comment #2)
> (In reply to Drew Willcoxon :adw from comment #1)
> > (In reply to adam.morris from comment #0)
> > > The removed search engines are automatically restored some time later
> > > (likely after updates).
> > 
> > Florian, are default engines that the user has removed restored when they
> > update Firefox?
> 
> That's not the expected behavior, no.
> 
> If this happens, there's likely something messing with the user profile on
> the disk. On Fedora I don't expect hijacking, but that could still be for
> example a backup script. I think we also still have some brokenness when
> there's an update that causes a language pack to be briefly disabled before
> it's re-enabled after being updated too.
> 
> Adam, do you use Firefox in en-US, or do you have a language pack installed?

I have the English (GB) language pack installed
Flags: needinfo?(adam.morris)
Interesting. But I'm afraid there's still not enough information here to reproduce and fix this. Next time it happens, can you confirm if it happened right after an update?

This could be related to bug 1267719 (but we don't have enough information there either).
(Reporter)

Comment 5

3 years ago
I updated to 50.0 today and two default search engines (Amazon & Yahoo) returned without any action on my part.
The list of default engines for en-GB is:
  "google", "yahoo-en-GB", "bing", "amazon-en-GB", "chambers-en-GB", "ddg", "twitter", "wikipedia"

For en-US it is:
  "google", "yahoo", "amazondotcom", "bing", "ddg", "twitter", "wikipedia"

The yahoo and amazon engines have different names, so we may have lost metadata for them if somehow you were temporarily switched to en-US before the en-GB language pack got updated and re-enabled.

Have you kept the Chambers search plugin? Or did it reappear too?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
Summary: Deleted search engines keep returning → Deleted search engines from a language pack reappear after an update
Version: 49 Branch → Trunk
Can you also give me the value of the browser.search.region preference?
Repining.
Flags: needinfo?(adam.morris)
(Reporter)

Comment 9

2 years ago
browser.search.region is user-set to "FR" 

I did a test of downgrading to FF 47 (default version when doing `dnf downgrade` in Fedora 24): Amazon.com, eBay and Yahoo appeared during this
then I deleted them and upgraded back to FF 50: Amazon.co.uk, Chambers (UK) and Yahoo.co.uk appeared
Here are some screenshots: http://imgur.com/a/ydoYr
Flags: needinfo?(adam.morris)
Duplicate of this bug: 1339818

Comment 11

2 years ago
I reported bug # 1339818, the duplicate of this bug, which was just marked as such.

Since I had a Firefox update queued up today, my findings:

*Situation*
Update from Firefox 52.0 to 52.0.1
Additional locales: firefox-locale-en, firefox-locale-fr, firefox-locale-nl
Search engines before the update: DuckDuckGo, Google, Wolfram|Alpha, Le Conjugueur
browser.search.region: IT (for some strange reason, simply because that is where I reinstalled my system, in language EN at the time)
Current system language: FR
OS: Ubuntu GNOME 16.10 (x64)

After a restart of Firefox, no additional search engines were added this time. So if the cause is a language pack update, it apparently does not happen when it's a minor update.

I will keep an eye out and report back as soon as the list of search engines changes again.

Let me know if you need any more information.
(In reply to Age Bosma from comment #11)

Thanks for the additional info!

> *Situation*
> Update from Firefox 52.0 to 52.0.1

Has this update of the browser also updated the language pack? Is this with a Firefox downloaded yourself from the Mozilla websites, or with the Firefox package distributed by Ubuntu?

Comment 13

2 years ago
Yes, all three language packs were updated as well, sorry for not being clear enough about that.
As always for me, it an update with the Firefox packages distributed by Ubuntu.

Comment 14

a year ago
This bug also happens for me (Linux, 64bit).
I'm using the website developer edition download of firefox.

After updating e.g. from firefox-58.0b15 to firefox-58.0b16 all the default search engines reappeared.
Last happened after updating to firefox-59.0b1.

Comment 15

9 months ago
I can confirm the issue. Using a package manager to update the installed language pack will reset all the default search engines on any update, which is slightly annoying. Is there a workaround to keep this from happening? I know ESR 60 has a policy to set search engines, but I'm not on ESR.
My guess is that it's switching back to English for some period of time which is resetting the engines.

If someone could turn the pref

browser.search.log

to true

and then look at the javascript console after an update, that would be helpful.

I'll try to recreate using the same mechanisms that the distros use.

Comment 17

9 months ago
That guess might be spot on. I was trying to reproduce with a fresh profile, downgrading and upgrading but it didn't work. I think it might be possible that distributions (or repository mirrors) are not shipping the new Firefox build and the language packs contemporaneously. That would result in the following:

1. installed Firefox version is upgraded, the language pack is not
2. on next launch Firefox says installed language pack is incompatible with this version, resets to English and resets all default (English) engines
3. user configures search engines to his preference
4. language pack is upgraded, enabled by FF again, resetting the search engines to the default of the language pack
5. user has to reset to his preference again

However, this theory could easily be validated if FF is reset to English for a short amount time after an update. While I've never noticed this, I wouldn't rule it out maybe users aren't noticing because English is so common.

The log in the browser console didn't tell anything useful but I'll keep this preference on while using the system normally and waiting for the issue to repeat.

If this theory is true, it is actually a packaging/distribution issue. But I still think Firefox shouldn't reset preferences by (de)activating language packs. Especially if for the English default, they had already been set and switching back to English doesn't respect those settings.
We've moved search engines out of language packs in Firefox 62, so there's a chance we might get this fixed for free.

What I'm guessing is happening is the search service is looking for engines and not finding them and resetting to the default (since they were in the language pack).

It will now find every engine.

Comment 19

8 months ago
In Firefox 62 it is even worse, deleted language specific search engines reappear after every Firefox restart.
That's odd. Are you using English Firefox with a language pack or a translated version?

What search engines are you hiding?

Comment 21

8 months ago
Can confirm that hidden engines are re-added at each start now.

FF 62.0, packaged version (Arch Linux), packaged language pack (German).

I've attached a log of the relevant time frame (browser.search.log enabled). The relevant line might be:

"Absent or outdated cache. Loading engines from disk."?

Comment 22

8 months ago
I use Debian (sid) packaged English Firefox with several language packs.

For instance:

#firefox-l10n-fr
Google              stays hidden
Bing                stays hidden
Amazon.fr           reappaers
DuckDuckGo          stays hidden
eBay                stays hidden
Portail Lexical     reappaers
Qwant               reappaers    
Wikipedia (fr)      reappaers

#firefox-l10n-de
Google              stays hidden
Bing                stays hidden
Amazon.de           reappaers
DuckDuckGo          stays hidden     
eBay                stays hidden
Ecosia              reappaers
LEO Eng-Deu         reappaers    
Wikipedia (de)      reappaers

#disabled language pack (fallback to original en)
Google              stays hidden
Bing                stays hidden
Amazon.com          stays hidden
DuckDuckGo          stays hidden     
eBay                stays hidden
Twitter             stays hidden 
Wikipedia (en)      stays hidden

Comment 23

8 months ago
I'm using Firefox 62 from the Ubuntu 18.04 repository.
For me the search engines also reappear after a restart.
As others mentioned above this changes when using another locale.
Could you set the preference browser.search.log to true and then restart the browser.

I then need the contents of the Javascript console.

Comment 25

8 months ago
Added the log file when restarting Firefox with search log enabled. The search engines I dot't want to have are for example Amazon.co.uk Chambers (UK) or eBay.
Attachment #9007589 - Attachment mime type: text/x-log → text/plain
What do you see when you paste this in the URL bar?

resource://search-plugins/amazondotcom.xml

Can you post the contents of:

resource://search-plugins/list.json

Updated

8 months ago
Duplicate of this bug: 1489820
Never mind, I can recreate. Very strange.
I'll keep debugging, but here's what I see so far (testing German).

The search cache is German, but when you delete a search engine and close Firefox and look at the search cache, it has switched to English search engines.

    "visibleDefaultEngines": ["google-2018", "amazondotcom", "bing", "ddg", "ebay", "twitter", "wikipedia"],

And it stays that way.

So the cache never matches.

If you delete the cache file and restart, you get a German cache file. But then if you make a change, you end up with English again.

So we must be using the wrong locale at some point for list.json.
Side note. If the cache is wrong on disk, we should wipe it and create a new cache. Once you have a bad cache, it seems to just stay that way.
An async Reinit is happening on shutdown

*** Search: _asyncReInit
[Tracking Requested - why for this release]: Regression with Firefox 62.
So it's the new code that handles locale changes:

      case TOPIC_LOCALES_CHANGE:
        // Locale changed. Re-init. We rely on observers, because we can't
        // return this promise to anyone.
        // FYI, This is also used by the search tests to do an async reinit.
        this._asyncReInit();
        break;

When you shutdown, there's a locale change notification from the L10nRegistry when a source is removed, and at that point, the L10n entry doesn't have the locale in it anymore.

gandalf: is this expected behavior?

I'm not sure how to work around this. For long term locale switching, we do need to reinit when a locale is removed.

But we don't want to reinit on shutdown.
Flags: needinfo?(gandalf)
How can a 2 year old bug be a regression in 62 which shipped last week?  If there is a regression in 62, it seems to me it should be its own bug...
> How can a 2 year old bug be a regression in 62 which shipped last week?  If there is a regression in 62, it seems to me it should be its own bug...

I felt weird forward duping this, but you are right.

This bug had better information in in it (because a reporter of this gave us the info we needed).
I've reponed 1489820 as the bug to handle this in.
Keywords: regression
(Assignee)

Comment 37

8 months ago
> When you shutdown, there's a locale change notification from the L10nRegistry when a source is removed, and at that point, the L10n entry doesn't have the locale in it anymore.

> gandalf: is this expected behavior?

I would assume so. LocaleService is managing the state of locales, so as sources are added and removed, and requested locales are added/removed it renegotitates and broadcasts the state.

L10nRegistry is managing the state of langpacks, and similarly just reports when they're added/removed.

And there seem to be no difference between "disable/remove langpack" and "Firefox is shutting down" here.

I'm wondering if:

a) we could communicate the reason of app-locales-changed to indicate shutdown
b) you could somehow check in your code if the app is currently shutting down and ignore the event?
Flags: needinfo?(gandalf)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #37)

> a) we could communicate the reason of app-locales-changed to indicate
> shutdown
> b) you could somehow check in your code if the app is currently shutting
> down and ignore the event?

Why is that notification sent during shutdown, at a point where no localized string needs to be used anymore?
(Assignee)

Comment 39

8 months ago
> Why is that notification sent during shutdown, at a point where no localized string needs to be used anymore?

Because the state of the system changes and the notification is about a change to the state of the system. Locales are not only used for localization, but also internationalization etc.

System operates in states that change during bootstrap, lifecycle and shutdown. In the same way as you are sending info about element being unlinked from a document during the document's closing.
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #39)
> > Why is that notification sent during shutdown, at a point where no localized string needs to be used anymore?
> 
> Because the state of the system changes and the notification is about a
> change to the state of the system.

This seems like an abstraction that's working against us. During shutdown, unless data needs to be saved to disk, the code should do nothing.

If all consummers of an API need to add shutdown notification observers just to know when to ignore the other notifications, it would be easier to just not send the notification at all during shutdown.

> In the same way as you are sending info about element being unlinked
> from a document during the document's closing.

Are we really doing this?
(Assignee)

Comment 41

8 months ago
> If all consummers of an API need to add shutdown notification observers just to know when to ignore the other notifications, it would be easier to just not send the notification at all during shutdown.

I disagree on the design level. Consumers of the API should manage life cycle of their system, rather than asking other systems to special case their state model for it.

LocaleService does just that, it maintains the state of locale selection. Asking it to become "smart" seems like a foot gun to me.
Asking it to provide more information about its status makes sense, so I'm open to that avenue.

> Are we really doing this?

Absolutely[0]. In some cases we even fire an event [1] so that customers can react. Every element in HTML, XUL and XHTML reports when it gets unlinked.


[0] https://searchfox.org/mozilla-central/source/dom/html/HTMLSharedElement.cpp#274
[1] https://searchfox.org/mozilla-central/source/dom/html/HTMLLinkElement.cpp#163
Tracking bug 1489820 instead.
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #41)

> LocaleService does just that, it maintains the state of locale selection.
> Asking it to become "smart" seems like a foot gun to me.
> Asking it to provide more information about its status makes sense, so I'm
> open to that avenue.

So what's triggering a change of state of the locale service during shutdown, and can we avoid that? It seems we are not looking at this at the same level, you are talking about API design, but my concern is just about how much code runs during shutdown. So as far as I'm concerned, the earlier we stop (what's triggering) the notification, the better. That may not be in the locale service, it could be even earlier if that's possible.


> > Are we really doing this?
> 
> Absolutely[0]. In some cases we even fire an event [1] so that customers can
> react. Every element in HTML, XUL and XHTML reports when it gets unlinked.
> 
> 
> [0]
> https://searchfox.org/mozilla-central/source/dom/html/HTMLSharedElement.
> cpp#274
> [1]
> https://searchfox.org/mozilla-central/source/dom/html/HTMLLinkElement.cpp#163

That's code running when removing specific DOM nodes from a document. I don't think this runs everytime we close a tab.
(Assignee)

Comment 44

7 months ago
> So what's triggering a change of state of the locale service during shutdown, and can we avoid that?

Shutdown of the langpack managed in [0]. I'm not sure how and if we should avoid that. Shutdown process is a cascade of operations that unlink components. From the architectural perspective you want to shutdown extensions.

> It seems we are not looking at this at the same level, you are talking about API design, but my concern is just about how much code runs during shutdown.

I agree. I also agree that your perspective is valuable and we should look for ways to reduce the amount of code we're executing at shutdown, especially if that code does not impact user experience.

I think we need a conversation on a architectural level on how to achieve it. Maybe we should aim for separate models of "shutdowns" and "unlinks" depending on whether the operation takes place during the lifetime, or during the shutdown.

> That's code running when removing specific DOM nodes from a document. I don't think this runs everytime we close a tab.

I recommend you instrumenting the code to test for yourself, or asking anyone from the DOM team then.

And it really is just an example. All architectural code has a init/shutdown cycle. Solving it just in one case because it causes a problem is going to be chasing symptoms.


[0] https://searchfox.org/mozilla-central/rev/dd965445ec47fbf3cee566eff93b301666bda0e1/toolkit/components/extensions/Extension.jsm#2036
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #44)
> > So what's triggering a change of state of the locale service during shutdown, and can we avoid that?
> 
> Shutdown of the langpack managed in [0]. I'm not sure how and if we should
> avoid that.

> [0]
> https://searchfox.org/mozilla-central/rev/
> dd965445ec47fbf3cee566eff93b301666bda0e1/toolkit/components/extensions/
> Extension.jsm#2036

Thanks, that seems to be where the problem is. The Dictionary class has a check to avoid doing work during normal shutdowns, see https://searchfox.org/mozilla-central/rev/dd965445ec47fbf3cee566eff93b301666bda0e1/toolkit/components/extensions/Extension.jsm#1980
I think the Langpack class should do the same.
(Assignee)

Comment 46

7 months ago
Cool! That sounds good!

With Kris' blessing, I'm going to take this bug and write this line of code :)
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Component: Search → General
Product: Firefox → WebExtensions
So with this patch the workaround from bug 1489820 is no longer needed, right?
(Assignee)

Comment 49

7 months ago
I think so, but would like Mike to verify.
Comment on attachment 9009274 [details]
Bug 1305705 - Don`t unregister langpacks on shutdown. r=aswan

Andrew Swan [:aswan] has approved the revision.
Attachment #9009274 - Flags: review+

Comment 51

7 months ago
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/afe1daa73e56
Don`t unregister langpacks on shutdown. r=aswan

Comment 52

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/afe1daa73e56
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #49)
> I think so, but would like Mike to verify.

Mike, do we still need the hack from bug 1489820, or can we remove it?
Flags: needinfo?(mozilla)
(In reply to Florian Quèze [:florian] from comment #53)
> (In reply to Zibi Braniecki [:gandalf][:zibi] from comment #49)
> > I think so, but would like Mike to verify.
> 
> Mike, do we still need the hack from bug 1489820, or can we remove it?

We'll need to test and see. I imagine we don't. Unfortunately langpack testing on nightly is very painful
Flags: needinfo?(mozilla)

Comment 55

7 months ago
Verified using a DE and FR language pack along with Nightly 64.0a1 using steps from comment #27 available on bug 1489820. On this version the issue appears to be fixed as I could clearly reproduce it using 63.0b5. Testing was performed using Windows 10 x64 and Ubuntu 18.04 LTS.

Updated

7 months ago
Status: RESOLVED → VERIFIED
Is this something we should consider for Beta uplift?
Flags: needinfo?(gandalf)
QA Contact: ddurst
QA Contact: ddurst
(Assignee)

Comment 57

7 months ago
Comment on attachment 9009274 [details]
Bug 1305705 - Don`t unregister langpacks on shutdown. r=aswan

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None

User impact if declined: We're unregistering langpacks during shutdown, which may, in some edge cases, cause issues. This is a trivial patch that disables the unregistering during shutdown.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): It's a super trivial patch which makes us do one less thing during shutdown (which we already do for other types of addons).

String changes made/needed: None
Attachment #9009274 - Flags: approval-mozilla-beta?
Flags: needinfo?(gandalf)
Comment on attachment 9009274 [details]
Bug 1305705 - Don`t unregister langpacks on shutdown. r=aswan

Trivial patch that baked 2 weeks on Nightly with no reported regression, approved for our last 63 beta.
Attachment #9009274 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify-

I just upgraded to Firefox 66.0 on Fedora 29, and the Chambers (UK) search engine was automatically reinstalled and enabled. This is not fixed.

You need to log in before you can comment on or make changes to this bug.