291666 - Installation of some extensions creates a file for what should be a directory since bug 286034 landed

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

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

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

The new EM code that landed with bug 286034 creates a file for each entry
returned by findEntries of a nsIZipReader which at times should actually be a
directory.

Reproducible: Always

Steps to Reproduce:




testcase and patch coming up

Comment 1

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

Attachment: testcase

This testcase is from a different bug so please ignore the extension name /
description / etc. After installing look in your profile's
extensions/{5DB6B72E-5C55-47a0-AB3B-945B4023E398} directory. I see a components
file instead of a components directory. I also have the patch from bug 291572
applied in order to avoid other issues and when testing without that patch you
may experience other issues not covered by this bug.

Comment 2

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

Attachment: patch

Ben - I suspect that with how new this code is and that since it is fallout
from bug 286034 that you would be the appropriate person to review.

This patch first checks if the parent of the file that is to be created is a
directory, if it isn't it deletes the file / creates a directory in its place.

I suspect the reason for this section of new code is to fix bug 189905 for
Firefox?

Comment 3

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

As a side note, it may be a better approach to enumerate the parents creating
directorys along the way until a directory is reached and I'll try to create a
testcase that shows whether this would be better.

Comment 4

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

Correction regarding the testcase... the components directory created as a file
can only be seen while debugging otherwise all that will be seen is the
installation failing and rolling back in the console.

Comment 5

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

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

It is as I thought. getNext() returns the files in what appears to be when it
was added. New patch coming up.

Comment 6

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

Attachment: evil testcase

I've added some random directory structures with files in them and one empty
directory in the root which will always get added as a file.

Comment 7

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

Attachment: better patch

This patch covers everything but the case where a directory is empty in the
extension in which case it gets added as a file

Comment 8

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

Attachment: finished patch

This patch first enumerates and creates all of the directories stored in the
extension and then only unzips the files. It uses "*/" as the param to
findEntries to find just the directories which appears to be cross platform
according to this specification
http://www.pkware.com/company/standards/appnote/ where it states:
file name: (Variable)

	  The name of the file, with optional relative path.
	  The path stored should not contain a drive or
	  device letter, or a leading slash.  All slashes
	  should be forward slashes '/' as opposed to
	  backwards slashes '\' for compatibility with Amiga
	  and Unix file systems etc.

BTW: a good test is to install either testcase and then the other testcase
since they both use the same GUID for em:id

Comment 9

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

To see part of the reason this patch is needed the following is the order in
which the directories are returned when installing the evil testcase
*** entry.name: components/
*** entry.name: components/another1/
*** entry.name: components/bogus/bogus2/another1/another2/
*** entry.name: another1/
*** entry.name: components/bogus/
*** entry.name: another1/another2/
*** entry.name: Help/
*** entry.name: components/another1/another2/
*** entry.name: components/bogus/bogus2/another1/
*** entry.name: components/bogus/bogus2/

So in the above we find
components/bogus/bogus2/another1/another2/
before
components/bogus/bogus2/
etc.

Comment 10

[Benjamin Smedberg]

Comment on attachment 181690 [details] [diff] [review]
finished patch

Interesting. Is there some way to get all the zip entries that are not
directories?

Comment 11

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

(In reply to comment #10)
> (From update of attachment 181690 [details] [diff] [review] [edit])
> Interesting. Is there some way to get all the zip entries that are not
> directories?

Not that I have been able to find... I have seen "*.*" used but that would of
course exclude files without a "." in them. It appears the original intent was
to be able to extract all of the files / directories in one fell swoop. This
leaves the permissions intact (see bug 189905) which is what I believe Ben was
attempting to resolve with this change by first creating the file and then
extracting over it. The trouble is that the order of the files / directories is
pertinent and can change depending on when the file / directory were added (iirc
the extension signing bug had a similar issue where the order the files were
added during signing had to be re-coded).

The symptoms of this bug for anyone that reads this are:
Newly installed extensions alwats state they will be installed on restart in the
Extensions Manager.
The JavaScript console reports:
Error: [Exception... "Component returned failure code: 0x80520012
(NS_ERROR_FILE_NOT_FOUND) [nsIFile.copyTo]"  nsresult: "0x80520012
(NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame ::
file:///C:/Program%20Files/Mozilla%20Firefox/components/nsExtensionManager.js ::
ensureExtensionsFiles :: line 543"  data: no]

Comment 12

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

I attempted to figure out what shell expression would return only files without
any success. *.* does in fact return only the entries with a . in them which
isn't sufficient since it doesn't return files without a . and it does return
directories with a . - it looks like the current patch is probably the way to go
since it covers all the bases.

I was wrong re: the js console message... that one was for bug 291675 and the
missing extensions.rdf. The message I just got is
Install Location returned an item path that does not exist! etc.

Comment 13

[Ben Goodger (use ben at mozilla dot org for email)]

So, does this patch work properly?

Comment 14

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

I tested around 30 extensions using the patch and it worked for me. My only
concern is with the behavior of findEntries("*/") on different OS's.

Comment 15

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

Attachment: patch

Updated patch to fix bitrot

Comment 16

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

First, the patch creates all directories stored in the zip file with permissions
set to 0755 (e.g. PERMS_DIRECTORY) in order to avoid directories being created
using the permissions that are set in the zip file by enumerating just the
directories in the zip file (e.g. findEntries("*/")). Since the order the
directories and files are found when using findEntries is dependent on when the
directory was added and not the extracted directory structure the code uses an
array to store the names of all directories that don't already exist in the
filesystem then enumerates the array backwards creating these directories.
+      // create directories first
+      var entries = zipReader.findEntries("*/");
       while (entries.hasMoreElements()) {
         var entry =
entries.getNext().QueryInterface(Components.interfaces.nsIZipEntry);
+        var target = installLocation.getItemFile(extensionID, entry.name);
+        var pathArray = [];
+
+        while (!target.exists()) {
+          pathArray.push(target.leafName);
+          target = target.parent;
+        }
+
+        for (var i = pathArray.length -1; i >= 0; --i) {
+          target.append(pathArray[i]);
+          target.create(nsILocalFile.DIRECTORY_TYPE, PERMS_DIRECTORY);
         }
+      }
+

Second, the patch extracts only the files in the zip file. It accomplishes this
by enumerating all entries in the zip file (e.g. findEntries("*")) and if the
file doesn't already exist in the filesystem (e.g. a directory created in the
previous step) it creates a file with permissions set to 0644 (e.g. PERMS_FILE)
as the previous code did and then extracts the file over this file as the
previous code did except in this case we don't create files for directory
entries in the zip file.
+      entries = zipReader.findEntries("*");
+      while (entries.hasMoreElements()) {
+        entry =
entries.getNext().QueryInterface(Components.interfaces.nsIZipEntry);
+        target = installLocation.getItemFile(extensionID, entry.name);
+        if (target.exists())
+          continue;
+
+        target.create(nsILocalFile.NORMAL_FILE_TYPE, PERMS_FILE);
+        zipReader.extract(entry.name, target);

Comment 17

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050427
Firefox/1.0+

this patch WFM

there's one other thing (i'm not sure it should be in this bug though)
repro
1.disable an extension and close FF
2.open FF and enable the same extension
3.close and open FF
4.open EM , message ->"the extension will be enabled the next time you start
Firefox" despite that the fact that the extension does work !

Comment 18

[Jaime Mitchell (use [e-mail] for email)]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050427
Firefox/1.0+

The patch also wfm, although if you tried to install extensions before the patch
some manual clean up is needed or a new profile (only an issue for those that
ran the trunk builds in the past few days)

However, when installing a theme I get the message "the extension will be
enabled the next time you start" but restarting does not enable. 

Restarting, selecting use theme and restarting does not use the theme but still
gives the same message. So perhaps this has a different cause to the error Peter
van der Woude is seeing with extensions.

Bug 291946 says this patch should also fix installation of themes.

Comment 19

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

The new EM code throws an error intentionally if the theme does not have a
chrome.manifest file and most themes at this time don't. If this happens you end
up in the state you describe. If the theme is installed and has a
chrome.manifest after this patch is applied it will install but not necessarily
properly. When a theme is installed a copy of the theme is place in the
profile's chrome directory (see bug 286525). If the theme is installed again the
jar file has a -x appended to the jar file's name (e.g. theme-2.jar) and this
appears to be the file that is copied to the theme's chrome directory. Since the
internalName and the jar file's name no longer match at this point the ui is
unusable. It is possible to recover in -safe-mode since that still works with
themes (see bug 285544) though not extensions last I checked.

To workaround this with this patch applied delete the theme jar files for the
theme you are installing from the chrome directory before installing.

Comment 20

[Mike Connor [:mconnor]]

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

if forward slashes work on Windows, I'm not so worried.  Zip is a
cross-platform format, so I would expect that entries will be the same
regardless of platform.  

>+        var target = installLocation.getItemFile(extensionID, entry.name);
>+        var pathArray = [];
>+
>+        while (!target.exists()) {
>+          pathArray.push(target.leafName);
>+          target = target.parent;
>+        }
>+
>+        for (var i = pathArray.length -1; i >= 0; --i) {
>+          target.append(pathArray[i]);

this seems wrong, pathArray should never have more than one element.  We have a
single target here, and we're already in a loop.  The following code should be
fine, afaict.  Please explain what I'm missing if you think I'm wrong.

if (!target.exists()) {
  target.create(nsILocalFile.DIRECTORY_TYPE, PERMS_DIRECTORY);
}

If we're logging everything else, there's no reason to not log here and below. 
Removing this makes the logging a lot less useful, since the actual point of
failure is unclear.

Comment 21

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

I don't think there is a problem with the */ in regards to enumerating the
directories in a zip. If I am not mistaken though with the original and this
code an entry from the zip in the format of directory/file and then used with
nsILocalFile will be using an incorrect path separator for some OS's... true? I
haven't had a chance to debug this on another system but it appears the patch
doesn't work on a Mac OS X system I tried it on.

Take a look at comment #9 and then consider the case of
defaults/preferences/prefs.js which is pretty standard in extensions. Since
there is no guarantee of the order that findEntries will return the entries as
shown previously and with the testcases we will end up at times creating a
defaults or preferences file instead of a directory. This is how I originally
discovered this issue and I have seen it at times create files for the chrome
and components directories as well.

This section of code didn't have logging originally but I am more than happy to
add some. It is important to note that this section of code runs during the
first startup with another following it due to the install so it doesn't show in
the console except with a debug build... though I am not so familiar with
Mozilla to know if there is another way... is there?

Comment 22

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

Disregard the comment about the path separators - getItemFile should take care
of that.

Comment 23

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

Attachment: patch

This addresses the comments and no longer uses the array, etc. I should have
just created the directory if it didn't exist since I already had a valid OS
specific path from the call to getItemFile. Much cleaner

Comment 24

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

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

wrong patch

Comment 25

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

Attachment: correct patch

Sorry about shooting that blank

I've tested this patch with the testcase and other extensions that were failing
on Firefox and they work with this patch applied. I have also applied this
patch to the latest nightly Thunderbird and I was able to install an extension
and a theme (after I added a chrome.manifest for the theme as stated earlier)
from UMO.

Comment 26

[Mike Pesce (:By-Tor)]

Shouldn't the chrome.manifest be created for themes automatically the way they
are for extensions?  Or is that a different bug altogether?

Comment 27

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

(In reply to comment #26)
> Shouldn't the chrome.manifest be created for themes automatically the way they
> are for extensions?  Or is that a different bug altogether?
I don't know whether Ben consciously wanted to make it mandatory for there to be
a chrome.manifest but it is in the new code and it would be a different bug.

Comment 28

[bugzilla]

This patch is applied to the linux trunk build I have at http://getswiftfox.com
and I can confirm that the extensions I tested work with this patch applied.

Comment 29

[Mike Connor [:mconnor]]

Comment on attachment 182009 [details] [diff] [review]
correct patch

This looks good, and seems to work just fine. r=me

Comment 30

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 182009 [details] [diff] [review]
correct patch

a=caillon for 1.1a

Comment 31

[Mike Connor [:mconnor]]

Checked in.  Thanks!

Comment 32

[Nickolay_Ponomarev]

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

Comment 33

[Asa Dotzler [:asa]]

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

Comment 34

[Hiro]

Attachment: crash log

In the Mac version, it crashes when Extention is installed and Firefox is
re-start. 
And Extention is not installed though it doesn't crash when start is done
again. 
(In Extention Manager, it is a message "When being start next time, it is
installed". )

Comment 35

[Tracy Walker [:tracy]]

verified fixed on Linux Thunderbird trunk build 2005-04-29-04-trunk and Windows
Firefox trunk build 2005-04-29-07-trunk

However, I am seeing the crash and extension not installed on Mac Firefox build
2005-04-29-07-trunk as reported by crot0.

Shall we reopen this or create a new bug for Mac?

Comment 36

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

Please open a new bug and set it as blocking this bug. I am very sure that the
reason the crash wasn't seen before this patch landed is due to the extension
files not being extracted which this bug fixes and therefore never reaching the
point where this crash occurs when working with the libraries / components of an
extension. I will most likely have access to a Mac OS X system late this evening
and will debug it then. Please note what if any extension wa being installed,
etc. in the new bug (one should be enough) and an url to install it.