Closed Bug 1126722 Opened 7 years ago Closed 4 years ago

Overridden search engine order is not as expected


(Firefox :: Search, defect, P4)

31 Branch





(Reporter: gk, Assigned: mkaply)



(Whiteboard: [fxsearch])


(2 files, 2 obsolete files)

Suppose I want to have Bing as my default search engine and as the first one in the search engine list, Google at the second place and Yahoo at the third place. In order to get that I write an extension (attached) containing essentially the following preference (overwrites):

pref("", "Bing");
pref("", "Bing");
pref("", "Google");
pref("", "Yahoo");

Surprisingly that does not give me the desired order after installing the extension:

In ESR 31 I get (across platform and OS): Yahoo, Bing and Google + the remaining search engines and in Firefox 35+ I get Bing, Yahoo, Google + the remaining search engines (across platform and OS as well).

The expected result is: Bing, Google, Yahoo + the remaining search engines.

Note: This is working well if one just specifies and just when the third place should be specified, too, things go weird.
Priority: -- → P4
Whiteboard: [fxsearch]
Rank: 45
Are you in the US? If so, you likely need to also set the* prefs, see
No. Or do you mean whether I used an en-US bundle? Maybe. I could try to reproduce the behavior with an ESR45 based browser. Would we need to set* prefs etc. for the other bundles then, too?
I mean based on your location, detected from the IP you had at the time the profile was created. I assume with Tor that's probably a random country. Or have you disabled the code doing the geo-ip location check?

What values do you have for these prefs:
The reason this is happening is because getChildList is not guaranteed to return the preferences in alphabetical order.

When I tested this, I got back:

I think we should be alphabetizing the results from getChildList in this case.
Attached patch Sort the result of getChildList (obsolete) — Splinter Review
I was testing independently and had just found this problem.

The original design of this pref was for one offs:"Google" 

to move Google to the top, but I think the numeric usage is valid and we should handle it.
Attachment #8747123 - Flags: review?(florian)
Comment on attachment 8747123 [details] [diff] [review]
Sort the result of getChildList

Review of attachment 8747123 [details] [diff] [review]:

Looks reasonable, thanks for investigating this! How much effort would it be to add test coverage for this behavior?

::: toolkit/components/search/nsSearchService.js
@@ +4224,5 @@
>        // ... or engines sorted by default near the top of the list.
>        if (!sendSubmissionURL) {
>          let extras =
> +          Services.prefs.getChildList(BROWSER_SEARCH_PREF + "order.extra.").sort();

Looks like the sort() doesn't make any difference here, as the result of the computation we are doing here isn't an ordered list of engines but just a boolean indicating whether the order for the current default engine has been customized.
Attachment #8747123 - Flags: feedback+
> How much effort would it be to add test coverage for this behavior?

Great question. I was thinking about that this weekend. It looks like order.extra isn't tested at all, so I'll put together a test.
Comment on attachment 8747123 [details] [diff] [review]
Sort the result of getChildList

Removing the pending review request until there's a test then :-).
Attachment #8747123 - Flags: review?(florian)
Patch with extra sort removed and with test added.
Attachment #8747123 - Attachment is obsolete: true
Attachment #8747913 - Flags: review?(florian)
Comment on attachment 8747913 [details] [diff] [review]
Sort the result of getChildList and add test

Looks like you forgot to |hg add| the test_order.js file.
Attachment #8747913 - Flags: review?(florian)
Attached patch Proper patchSplinter Review
Proper patch (I hope)
Attachment #8747913 - Attachment is obsolete: true
Attachment #8748170 - Flags: review?(Flore)
Comment on attachment 8748170 [details] [diff] [review]
Proper patch

Review of attachment 8748170 [details] [diff] [review]:

::: toolkit/components/search/tests/xpcshell/test_order.js
@@ +6,5 @@
> +
> +"use strict";
> +
> +function run_test() {
> +  Services.prefs.setCharPref(BROWSER_SEARCH_PREF + "order.extra.1", "Twitter");

Tests in toolkit/ should avoid relying on the search plugins shipped by browser/.

Most (if not all) of the tests here install test search plugins and only rely on these being around.
Attachment #8748170 - Flags: review?(Flore) → review-
Per policy at If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.
Closed: 4 years ago
Resolution: --- → INACTIVE
Resolution: INACTIVE → ---
Assignee: nobody → mozilla
This isn't something worth fixing at this point. We're moving away from these prefs, and this was a legacy extension.

You can reorder the search engine in preferences.
Closed: 4 years ago4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.