1289240 - Services.search.getDefaultEngines() empty, search engine reset triggered when using a language pack

Status: RESOLVED FIXED

(Firefox :: Search, defect, P1)

Description

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

Attachment: screenshot reset search engine with empty search engine list

Firefox Aurora 49.0a2 20160725004033 on Windows 8.1

Launched Firefox Aurora with a German binary l10n repack https://developer.mozilla.org/en-US/docs/Mozilla/Creating_a_language_pack#L10n_binary_repack and a profile which had previously been used with Aurora in English and a German language pack installed on top.

After the launch of the binary repack, I started a search from the search bar and the search engine reset dialog got triggered and the list of available search engines was empty.

Error console:

Zeitstempel: 26.07.2016 00:07:15
Fehler: TypeError: list.selectedOptions[0] is undefined
Quelldatei: chrome://browser/content/search/searchReset.js
Zeile: 35

Calling Services.search.getDefaultEngines() in the scratchpad returns an empty list.

Also happens if I go back to the en-US build and install a language pack (had uninstalled the German one while running the German build).

Duplicate of bug 1267719?

Comment 1

[Florian Quèze [:florian]]

[Tracking Requested - why for this release]: Unfortunate behavior of the feature landed in bug 1203168 for Firefox 49 in an edge case that wasn't tested.

Comment 2

[Florian Quèze [:florian]]

(In reply to Sebastian H. [:aryx][:archaeopteryx] from comment #0)

In my testing, just running a build using a language pack is enough to trigger this. The profile doesn't need to have been used in a different way before.

> Launched Firefox Aurora with a German binary l10n repack
> https://developer.mozilla.org/en-US/docs/Mozilla/
> Creating_a_language_pack#L10n_binary_repack and a profile which had
> previously been used with Aurora in English and a German language pack
> installed on top.
> 
> After the launch of the binary repack, I started a search from the search
> bar and the search engine reset dialog got triggered and the list of
> available search engines was empty.

It's very likely that somehow the language pack installed in the profile was still being used, despite the same language being built-into the 'binary' by the repack.

> Duplicate of bug 1267719?

No, that bug is a problem that occurs only at the time of language pack updates, and causes Services.search.currentEngine to be null. (Bug 1267719 may not be search specific, and could turn out to be a duplicate of bug 1236347)

Here Services.search.currentEngine isn't null, only Services.search.getDefaultEngines() is broken.

Comment 3

[Florian Quèze [:florian]]

Attachment: Fix

The problem here is that search plugins from an active language pack have a load path looking like: jar:[profile]/extensions/[e-mail]!browser/google.xml
This won't pass the _isDefault check.

Each of the 2 changes in the patch I'm attaching can 'fix' the bug:

- The first one relaxes the _isDefault check when using a non-default locale, so that all plugins from resource://search-plugins/ pass the test (this fixes Services.search.getDefaultEngines()).

- The second change is an additional safety net, ensuring we'll never show the searchreset prompt when we are already using the originalDefaultEngine, as the search reset page is guaranteed to be broken in that case.

I don't see a way to write a test for this, as the _isDefault check is already relaxed in the case of xpcshell tests. This area of the code is already well covered by xpcshell tests, so I'm not really concerned by not having a test specifically for this edge case.

Comment 4

[Marcia Knous [:marcia]]

Tracking 49/50+ as search is important.

Comment 5

[Drew Willcoxon :adw]

Comment on attachment 8774717 [details] [diff] [review]
Fix

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

Makes sense

Comment 6

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/581ee0750f81067c33090c5d3958d757ff5f331b
Bug 1289240 - search plugins from an in-use langpack should be treated as default plugins, r=adw.

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/581ee0750f81

Comment 8

[Florian Quèze [:florian]]

Comment on attachment 8774717 [details] [diff] [review]
Fix

Approval Request Comment
[Feature/regressing bug #]: bug 1203168
[User impact if declined]: Users using a language pack will see broken search reset prompts whenever they try to search.
[Describe test coverage new/current, TreeHerder]: no automated test for this specific edge case, but the code the patch touches is generally well covered. QA can verify.
[Risks and why]: Low risk as the patch is simple and self-contained.
[String/UUID change made/needed]: none.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8774717 [details] [diff] [review]
Fix

Fix the search, taking it

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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