Closed Bug 570484 Opened 14 years ago Closed 14 years ago

Including multiple targetApplication entries for the same application causes a SQL constraint error when installing an add-on

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla2.0b1
Tracking Status
blocking2.0 --- final+

People

(Reporter: morac, Assigned: mossop)

References

Details

Attachments

(3 files)

In a profile that I use with the trunk loads, I recently ended up deleting all the extensions.* files so that they would be regenerated. After doing so I found that Minefield would spit out tons of SQL errors on start-up and that most add-ons would fail to import. The first error was a SQL 19 (constraint) error. Once that occurred, any subsequent add-on would fail to install with a SQL 21 (misuse) errors. I tracked the problem down to the extension console2 which I'm attaching. The problem here isn't simply that console2 failed to install, but that it prevented all other add-ons with a higher GUIDs from being installed once the install of console2 failed, since console2 was actually added to the extensions folder. It also caused an exception in XPIProvider.jsm :: XPIDB_shutdown :: line 2642 at shutdown. Steps to recreate: 1. Install attached add-on in Minefield. It will be added to the extensions folder, but will fail to actually install do to a SQL 19 (constraint) error. 2. Attempt to install another add-on with a larger GUID. That will be added to the extensions folder and fail to install because of a SQL 21 error. 3. Repeat #2 until bored. Work around is to check the extensions.log file for "SQL error 19" and physical remove the extension with the GUID on the following line from the extensions folder. Once that is done, all add-ons not installed because of SQL error 21 will be installed.
Installed Console2 from AMO and I can't see any problems: https://addons.mozilla.org/en-US/firefox/addon/1815/ Which version of Minefield are you using and what's the exact error you see? Also please attach the extension.* files to that bug.
Attached file Extensions log (deleted) —
This is with Minefield Gecko/20100607. The add-on version is actually version 0.6.0 which isn't available at AMO (which is why I atteched it). That version has typos in the install.rdf file. So basically the install.rdf file isn't valid. The issue isn't that the add-on failed to install, but that it partially installed which resulted in a constraint error every time the browser is started. That constraint error prevents other add-ons from being installed. A bad install shouldn't prevent other add-ons from being installed. Instead of constantly throwing a constraint on start-up, the bad add-on should be removed or ignored.
Also of note, since the failed add-on isn't listed in the Add-ons Manager, the only way to get rid of it, is to open the profile and delete the folder by hand. Basically an average user wouldn't know what to do if s/he ran into this in the wild. Granted installing only from AMO should prevent this since all the add-ons on there are tested, but it could be possible to have an older buggy add-on that installed fine in Firefox 3.6, but fails in 4.0.
All the errors I can find in the error console: Error: ERROR addons.xpi: SQL error 19: constraint failed Error: ERROR addons.xpi: Failed to add add-on {1280606b-2510-4fe0-97ef-9b5a22eafe80} in app-profile to database When moving the extension files manually into the profiles extension folder and clearing all extension files errors like those appear: Error: ERROR addons.xpi: SQL error 21: library routine called out of sequence Error: ERROR addons.xpi: Failed to add add-on {f13b157f-b174-47e7-a34d-4815ddfdfeb8} in app-profile to database This should block at least the beta1, or what would you think Dave?
blocking2.0: --- → ?
While checking the install.rdf I have found that the usage of the following target application section for Postbox is causing this problem: <targetApplication><!-- Postbox--> <RDF:Description> <id>{92650c4d-4b8e-4d2a-b7eb-24ecf4f6b63a}</id> <minVersion>1.0</minVersion> <maxVersion>1.2.*</maxVersion> <!-- don't take my word for it! --> </RDF:Description> </targetApplication>
The problem is that the add-on includes two targetApplication entries with the same ID which the code doesn't spot and tries to put into the database which is correctly disallowed. The code mostly recovers from this except due to bug 570529 the statement in use isn't reset and so nothing else can be done with the database. Maybe worth deleting the extension's folder if we get this far and haven't managed to add it to the database but that feels a little nasty to me. I don't think this necessarily blocks the beta since it is only triggered by a bad add-on which should be rare, but I intend to get it fixed pretty quickly anyway.
blocking2.0: ? → final+
Depends on: 570529
Summary: SQL constraint error when installing add-on, prevents installing any more add-ons → Including multiple targetApplication entries for the same application causes a SQL constraint error when installing an add-on
(In reply to comment #6) [...] > I don't think this necessarily blocks the beta since it is only triggered by a > bad add-on which should be rare, but I intend to get it fixed pretty quickly > anyway. Bad addons should be rare... at AMO where they are scrutinized before being made public, maybe. But malware posing as a legitimate addon, or a legitimate addon with typos in its install.rdf, could happen anywhere. The problem is that once the user has tried to install the bad add-on (and failed, well, not succeeded at least), he can no more install anything else (and an "ordinary" user wouldn't know, or dare, remove a subfolder [and which one?], with all its contents, from his <profile>/extensions/ folder). I believe this bug is a security liability. In this case, the duplication is for SeaMonkey's application ID; I would have thought that Firefox, Thunderbird, etc., would just ignore <targetApplication> blocks which were "not for them". I guess I'm naïve.
(In reply to comment #8) > (In reply to comment #6) > [...] > > I don't think this necessarily blocks the beta since it is only triggered by a > > bad add-on which should be rare, but I intend to get it fixed pretty quickly > > anyway. > > Bad addons should be rare... at AMO where they are scrutinized before being > made public, maybe. Actually there are a bunch of add-ons on AMO with bad or at least abnormal install.rdf data. > But malware posing as a legitimate addon, or a legitimate > addon with typos in its install.rdf, could happen anywhere. The problem is that > once the user has tried to install the bad add-on (and failed, well, not > succeeded at least), he can no more install anything else (and an "ordinary" > user wouldn't know, or dare, remove a subfolder [and which one?], with all its > contents, from his <profile>/extensions/ folder). I believe this bug is a > security liability. The problem with not being able to do anything else after trying to install one of these faulty add-ons should have been fixed by bug 570529. All that is left here is to make the new manager behave a little more like the old one and ignore multiple target app entries. I just haven't looked yet to see precisely what the old one did when it encountered two entries for Firefox for example, it is undefined behaviour. > In this case, the duplication is for SeaMonkey's application ID; I would have > thought that Firefox, Thunderbird, etc., would just ignore <targetApplication> > blocks which were "not for them". I guess I'm naïve. For various reason we're storing all in the database right now. But even if we weren't we'd still trip over this for add-ons that had Firefox in them twice.
For reference the old add-ons manager pays attention to the first targetApplication entry it comes across for the Firefox ID.
Attached patch patch rev 1 (deleted) — Splinter Review
This also fixed bug 574021. This makes us ignore targetApplication entries that are missing values or that are for application IDs that have already been seen. It also makes us ignore localized entries that have missing or empty <locale> entries or entries that have been seen before.
Assignee: nobody → dtownsend
Attachment #453773 - Flags: review?(robert.bugzilla)
Blocks: 574021
Status: NEW → ASSIGNED
Comment on attachment 453773 [details] [diff] [review] patch rev 1 >diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm >--- a/toolkit/mozapps/extensions/XPIProvider.jsm >+++ b/toolkit/mozapps/extensions/XPIProvider.jsm >@@ -295,26 +295,39 @@ function loadManifestFromRDF(aUri, aStre > let values = []; > let targets = aDs.GetTargets(aSource, EM_R(aProperty), true); > while (targets.hasMoreElements()) > values.push(getRDFValue(targets.getNext())); > > return values; > } > >- function readLocale(aDs, aSource, isDefault) { >+ function readLocale(aDs, aSource, isDefault, aSeenLocales) { Might be worth adding a comment for this function? >@@ -387,34 +400,46 @@ function loadManifestFromRDF(aUri, aStre >... >- !targetAppInfo.maxVersion) >- throw new Error("Invalid targetApplication entry in install manifest"); >+ !targetAppInfo.maxVersion) { >+ WARN("Ignoring invalid targetApplication entry in install manifest"); >+ continue; >+ } >+ if (seenApplications.indexOf(targetAppInfo.id) != -1) { >+ WARN("Ignoring repeat targetApplication entry for " + targetAppInfo.id + >+ " in install manifest"); nit s/repeat/repeated/ just noticed you used duplicate for locale which I think is better. >+ continue; >+ } >+ seenApplications.push(targetAppInfo.id); > addon.targetApplications.push(targetAppInfo); > }
Attachment #453773 - Flags: review?(robert.bugzilla) → review+
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a6
Blocks: 569138
Verified fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a6pre) Gecko/20100625 Minefield/3.7a6pre
Status: RESOLVED → VERIFIED
OS: Windows XP → All
Hardware: x86 → All
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: