344457 - Removing all search engines other than Google and Answers causes the second search engine in the list to be removed when Firefox exits

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[tommyjb]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060712 BonEcho/2.0b1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060712 BonEcho/2.0b1

 

Reproducible: Always

Steps to Reproduce:
1. Run a recent Bon Echo nightly (I'm using "Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060712 BonEcho/2.0b1") with a fresh profile.
2. Go to "Manage Search Engines".
3. Delete Yahoo, Amazon, Creative Commons, and eBay (leaving Google and Answers).
4. Close Bon Echo.
5. Restart Bon Echo.
6. View the list of search engines.
Actual Results:  
Only the Google engine shows.

Expected Results:  
The Google engine and the Answers engine both show.

I'm able to reproduce this only with just Google and Answers left installed.

This kills whichever is the second one in the list.  So if the first is Google, Answers goes.  If the first is Answers, Google goes.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Indeed, this is a problem with the ordering code.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

This is related to an issue I found while working on the patch for bug 327932, and I included a partial fix for it in the patch attached there. I'm going to move that fix to this bug because it's a separate issue. _saveSortedEngineList is only saving order info for the visible engines, so when you shutdown after removing engines, the new "position 2" in the list gets an "position 2" attribute, but the "position 2" (Yahoo, in this case) keeps it's "position 2" attribute. That means that when we build the list at startup (_buildSortedEngineList), the second engine loaded with a "position 2" attribute overwrites the original in the _sortedEngines array.

The fix is two parts:
1) make sure we don't end up with multiple engines with the same position attribute by saving the position info for all engines (i.e. make sure we don't cause the problem ourselves)
2) protect against the same issue caused by external changes by adding a check to _buildSortedEngineList to ensure it doesn't clobber existing items in the array.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

mozilla/browser/components/search/nsSearchService.js 	1.53

Comment 4

[:Gavin Sharp [email: [e-mail]]]

This has been on the trunk since Wednesday with no reported regressions, and is unlikely to cause any regressions. This is also a requirement for the engine update service (bug 327932).

Comment 5

[Mike Connor [:mconnor]]

Comment on attachment 229079 [details] [diff] [review]
patch

a=mconnor on behalf of drivers for checkin to the 1.8 branch

Comment 6

[:Gavin Sharp [email: [e-mail]]]

mozilla/browser/components/search/nsSearchService.js 	1.1.2.46