Closed Bug 1289240 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox :: Search, defect, P1)

49 Branch
defect

Tracking

()

RESOLVED FIXED
Firefox 50
Tracking Status
firefox49 + fixed
firefox50 + fixed

People

(Reporter: aryx, Assigned: florian)

References

Details

(Whiteboard: [fxsearch])

Attachments

(2 files)

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?
[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.
Version: 47 Branch → 49 Branch
(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.
Assignee: nobody → florian
OS: Unspecified → All
Priority: -- → P1
Hardware: Unspecified → All
Summary: Services.search.getDefaultEngines() empty, search engine reset triggered → Services.search.getDefaultEngines() empty, search engine reset triggered when using a language pack
Whiteboard: [fxsearch]
Attached patch FixSplinter Review
The problem here is that search plugins from an active language pack have a load path looking like: jar:[profile]/extensions/langpack-fr@firefox.mozilla.org.xpi!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.
Attachment #8774717 - Flags: review?(adw)
Tracking 49/50+ as search is important.
Comment on attachment 8774717 [details] [diff] [review]
Fix

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

Makes sense
Attachment #8774717 - Flags: review?(adw) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/581ee0750f81
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
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.
Attachment #8774717 - Flags: approval-mozilla-aurora?
Comment on attachment 8774717 [details] [diff] [review]
Fix

Fix the search, taking it
Attachment #8774717 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.