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

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Michael Kraft [:morac]]

Attachment: Console2 add-on which throws a constraint when being installed

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.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 2

[Michael Kraft [:morac]]

Attachment: Extensions log

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.

Comment 3

[Michael Kraft [:morac]]

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.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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?

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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>

Comment 6

[Dave Townsend [:mossop]]

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.

Comment 8

[Tony Mechelynck [:tonymec]]

(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.

Comment 9

[Dave Townsend [:mossop]]

(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.

Comment 10

[Dave Townsend [:mossop]]

For reference the old add-ons manager pays attention to the first targetApplication entry it comes across for the Firefox ID.

Comment 11

[Dave Townsend [:mossop]]

Attachment: patch rev 1

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.

Comment 12

[Robert Strong (they/them - no direct email)]

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);
>   }

Comment 13

[Dave Townsend [:mossop]]

landed: http://hg.mozilla.org/mozilla-central/rev/6baa689a5eba

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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