295680 - extension should not install when chrome registration fails

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Przemyslaw Bialik]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050526 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050526 Firefox/1.0+

Extension should not install when chrome registration fails

Reproducible: Always

Steps to Reproduce:
Install extension with bad path to filename.jar, package, locale, skin... in
install.rdf


Actual Results:  
After accepting 'chrome registration failed' alert the EM shows the extension as
'properly' installed 

Expected Results:  
After accepting 'chrome registration failed' alert the extension is silently
uninstalled before loading the browser (it is not much usable...)
More ? - display the buggy line in alert window

Comment 1

[Przemyslaw Bialik]

The same should be applied also when there are bad paths in chrome.manifest
although there is little inconsistency because the 'chrome registration failed'
alert is not showing.

Comment 2

[Gabriel Chadwick]

Could you post a testcase.

Comment 3

[Przemyslaw Bialik]

Attachment: Configuration_Mania_1.06.2005030801_broken.xpi

(In reply to comment #2)
> Could you post a testcase.

I thought that the testcase was not needed here because every extension can be
easily changed to fit this bug but here you go:
Configuration_Mania_1.06.2005030801_broken.xpi - manually broken path to the
skin package (testcase for Comment #0).
Note that in this case (not all apply) if you would install it on FF 1.0.x you
would only see the chrome registration failed alert and the extension would not
be installed (no entry in EM, only guid/chrome/confmania.jar under extensions
dir created) so this bug might be partially considered as regression.

Comment 4

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

The chrome registration failure message is displayed due to not being able to
create a chrome.manifest and the error message should probably be changed to
state that a chrome.manifest couldn't be created so what has happened is
clearer. I have seen this before and I believe the code for
safeInstallOperations may not be working properly for this error condition. I
plan on thoroughly testing and possibly patching that code after 1.1a is
released so several other patches can land first.

The issue you bring up in comment #1 is significantly different since the
chrome.manifest is supplied by the extension so there is no need to create it.
In this case the modified error message above would be inappropriate and adding
validation of the chrome.manifest entries may be inappropriate due to added
complexity and overhead in the code since this would primarily be used for
debugging when writing an extension and not by most users.

In both cases the error should not be seen by a user if the extension author
tested their extension before releasing it which is normal practice.

Comment 5

[Przemyslaw Bialik]

(In reply to comment #4)

Thanks for comments.
OK the issues are little different but in first one chrome.manifest can't be
created because of bad paths in install.rdf (<em:file>) and the second one is
about the bad paths in chrome.manifest itself (both are supplied by the extension).
Do we agree that the extension shouldn't been installed in such cases ?
(user should get alert with info to report to the author and not have installed
extension which is fully or almost unusable and 'flame' the author about
different buggy results)

Yeah I agree that such errors shouldn't been seen by a user but I've tested Many
extensions and there were (and will be) cases that author didn't tested
extension after packaging the xpi (this is the most common on mozdev projects
where xpi can by build by a script from cvs).
And UMO is not the only source for extensions for many users because they don't
host all extensions or not as fast as the users would like.
Also more friendly alerts would be better for those starting with extensions
development to not waste time on figuring out the buggy lines (by a mistake or
lack of knowledge).

Comment 6

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

From a code perspective they are actually significantly different. In the first
case the reason it fails is that it reads the contents.rdf files that are
located in the locations specified in the install.rdf in order to create a
chrome.manifest. In the second case there is no need to read the contents.rdf
files (they don't even have to exist) since there is already an author supplied
chrome.manifest.

I believe I have a patch in my tree for the first case originally reported that
I wrote while patching a couple of other EM bugs. I don't believe the EM code
should handle the second case or the remaining points you made since it would
add complexity and possibly a performance impact to the code for something that
is a basic part of extension development and is already documented.

Comment 7

[PikeUK]

->NEW

Comment 8

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

This is the extension version of bug 295013 in that it appears the zip reader
cache is keeping a reference to the jar file after it has been extracted and
preventing the deletion of it.

Please note that the patch I am working on will fix the problem as originally
described and not the separate issue of validating an extension provided
chrome.manifest. Validating an extension provided chrome.manifest file is IMO a
wontfix because this would only be valuable to extension authors since this
would not happen to users using an extension that has gone through minimal
testing... this sort of testing would be better provided by a separate utility
and not within Firefox itself. Also, if the code tests for validity of the
chrome.manifest entries then it is just as reasonable for it to test the
validity of the entries in all of the extension chrome (e.g. css, dtd, xul in
content, locale, and skin) since they can have the same results. Please file a
new bug for the separate issue if you would like the EM to validate an extension
provided chrome.manifest.

Comment 9

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

Attachment: patch

Benjamin - this fixes the issue in as simple of a manner as possible by
removing the staged xpi and writing to the startup manifest so it will be read
immediately. I also close the open zip reader used by xpinstall in
nsSoftwareUpdateRun.cpp since the xpi has to be deleted at this point the same
as I had to do for themes.

As a follow up to this I think it makes sense to move the chrome.manifest
creation code into safeInstallOperations. That way if it fails it can revert
back to a previously installed version. Either way this bug is an edge case
that will only occur when an extension that hasn't been tested is distributed
or possibly if someone distributed a malicious extension and this would be the
least of the worries in that case.

Comment 10

[Benjamin Smedberg]

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

I shall land this quickly.

Comment 11

[Benjamin Smedberg]

Fixed on trunk for 1.8b3.