341981 - sporadic "engine has no properties" error on startup

Status: VERIFIED FIXED

(Firefox :: Search, defect)

Description

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

I'm not sure the exact situations in which this happens, I think it has to do with using multiple builds with the same profile, but I've hit it three times in the past day. What ends up happening is that the _buildSortedEngineList pre-allocates an array of a certain size, then fills the engine in order from the storage file. If not all slots are full, then this._engines contains undefined items and iterating through the engine list later in the startup process fails, giving a "engine has no properties" error and a blank search dropdown.

I guess this could have happened when I ran a build including the "use storage" patch, which created the db, and then used an older build that didn't include that patch to install an engine. When the newer build starts up, a new engine exists that doesn't have an entry in the db, which leaves the blank slot in the array.

To fix this, we should make sure that the resultant array from _buildSortedEngineList doesn't contain any undefined items.

Comment 1

[Joey Minta]

The patch in Bug #340331 has a fix for this.

Comment 2

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

*** Bug 342122 has been marked as a duplicate of this bug. ***

Comment 3

[Ria Klaassen (not reading all bugmail)]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060619 Minefield/3.0a1

Under unknown circumstances Firefox becomes location-sensitive. Only a search in Google works, although all plugins are present, also in the app directory.

Error: uncaught exception: [Exception... "'[JavaScript Error: "engine has no properties" {file: "file:///C:/Docume~1/Ria/Bureau~1/trunk/firefox/components/nsSearchService.js" line: 2118}]' when calling method: [nsIBrowserSearchService::getVisibleEngines]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://browser/content/search/search.xml :: rebuildPopup :: line 256"  data: yes]

When I copy the application to another place, suddenly the gray text and the plugins are working again.

Comment 4

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

If the search component doesn't successfully load the first time you run a build after unzipping it (like when you remove the plugins folder), subsequent startups may also fail because Firefox won't try to reload the search component again. By moving the program folder, you're changing the last-modified date of the .autoreg file, which tells Firefox to try reloading the component at the next startup.

Comment 5

[Phil Ringnalda (:philor)]

*** Bug 342168 has been marked as a duplicate of this bug. ***

Comment 6

[Simon Bünzli]

Attachment: respect the fact that _engines is no Array

Comment 7

[Simon Bünzli]

As I see it, this is a regression from bug 335101 which initializes the _sortedEngines array always with length 1 - which causes an undefined engine if there are none.

Comment 8

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

Comment on attachment 226388 [details] [diff] [review]
respect the fact that _engines is no Array

I don't see how this helps, the array will still have holes if one of the earlier engines in the db sort order no longer exists. I prefer the patch in bug 340331 (just filter the array once we've gone through all db entries).

Comment 9

[Simon Bünzli]

Even if it doesn't fix the general case, that bit of code is still wrong and should be fixed (_engines is an Object and no Array).

Would you prefer a new bug to check this fix in, should I mention the issue over at bug 340331 or will you reconsider fixing it here?

Comment 10

[Peter van der Woude [:Peter6]]

this should be gone before beta1

Comment 11

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

Comment on attachment 226388 [details] [diff] [review]
respect the fact that _engines is no Array

Ah, you're right, sorry. Yes, we should take both this fix and the filter fix from bug 341986.

Comment 12

[Darin Fisher]

Comment on attachment 226388 [details] [diff] [review]
respect the fact that _engines is no Array

a=darin on behalf of drivers (please land this on the MOZILLA_1_8_BRANCH and add the fixed1.8.1 keyword to this bug)

Comment 13

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

Checked in on the branch and trunk. I'll leave this open until bug 335102 lands because the problem isn't entirely fixed until then. I can't mark it as depending on bug 335102, though, because Bugzilla sucks (I wish bug 251556 was fixed).
mozilla/browser/components/search/nsSearchService.js 	1.1.2.37
mozilla/browser/components/search/nsSearchService.js 	1.43

Comment 14

[Ria Klaassen (not reading all bugmail)]

I still see the problem that the menus are too large (too many items).
- Delete searchplugins everywhere (remove folders)
- Start Firefox, double-click a word on a page, watch the context-menu

Comment 15

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

(In reply to comment #14)
> I still see the problem that the menus are too large (too many items).

Can you file another bug on that? It's a separate issue - the context menu code should check for a null currentEngine. It's pretty unlikely to affect any "normal" user, though :).

Comment 16

[Ria Klaassen (not reading all bugmail)]

(In reply to comment #15)
>
Filed Bug 342643 for this.

Comment 17

[Darin Fisher]

Adding fixed1.8.1 flag since this patch has been landed on the MOZILLA_1_8_BRANCH per comment #13.  I realize that we want bug 335102 to be fixed as well, but that one looks iffy for beta1.

Comment 18

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

I think we should take the one line fix from that patch to fix this issue. This bug sucks, and the fix is easy, so we shouldn't wait for bug 335102 to land to fix this.

Comment 19

[Joey Minta]

Attachment: filter fix

Splitting the .filter fix out of my renaming patch, since renaming has been retargeted for beta2.

Comment 20

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

Comment on attachment 228216 [details] [diff] [review]
filter fix

>+    // Filter out any nulls for engines that may have been removed
>+    this._sortedEngines = this._sortedEngines.filter(function(a) { return a; });

I'd kinda prefer "!!a" to make it clear it's the boolean value that matters. Put this right above the "// Array for the remaining engines, alphabetically sorted" comment, too, since this isn't related to the alphaEngines stuff.

Comment 21

[Joey Minta]

Checking in browser/components/search/nsSearchService.js;
/cvsroot/mozilla/browser/components/search/nsSearchService.js,v  <--  nsSearchService.js
new revision: 1.49; previous revision: 1.48
done

Comment 22

[Joey Minta]

Comment on attachment 228216 [details] [diff] [review]
filter fix

requesting approval for low-risk high-reward fix, intentionally split off to be acceptable for beta1.

Comment 23

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

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