Closed Bug 1289240 Opened 3 years ago Closed 3 years ago
.search .get Default Engines() empty, search engine reset triggered when using a language pack
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 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.
(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
The problem here is that search plugins from an active language pack have a load path looking like: jar:[profile]/firstname.lastname@example.org!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.
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.