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

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
15 years ago
11 years ago

People

(Reporter: rstrong, Assigned: rstrong)

Tracking

({regression})

unspecified
x86
Windows XP
Points:
---
Dependency tree / graph
Bug Flags:
blocking-aviary1.5 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 5 obsolete attachments)

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
Posted file 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.
Posted patch patch (obsolete) — Splinter Review
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?
Assignee: bugs → moz_bugzilla
Status: NEW → ASSIGNED
Attachment #181676 - Flags: review?(bugs)
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.
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 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.
Attachment #181676 - Attachment is obsolete: true
Attachment #181676 - Flags: review?(bugs)
Posted file 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.
Posted patch better patch (obsolete) — Splinter Review
This patch covers everything but the case where a directory is empty in the
extension in which case it gets added as a file
Posted patch finished patch (obsolete) — Splinter Review
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
Attachment #181687 - Attachment is obsolete: true
Attachment #181690 - Flags: review?(bugs)
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 on attachment 181690 [details] [diff] [review]
finished patch

Interesting. Is there some way to get all the zip entries that are not
directories?
Flags: blocking-aviary1.1+
(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]
Depends on: eminstall
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.
So, does this patch work properly?
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.
Attachment #181690 - Attachment is obsolete: true
Attachment #181690 - Flags: review?(bugs)
Posted patch patch (obsolete) — Splinter Review
Updated patch to fix bitrot
Attachment #181838 - Flags: review?(bugs)
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);
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 !
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.
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 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.
Attachment #181838 - Flags: review?(bugs) → review-
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?
Disregard the comment about the path separators - getItemFile should take care
of that.
Posted patch patch (obsolete) — Splinter Review
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
Attachment #181838 - Attachment is obsolete: true
Attachment #182001 - Flags: review?(mconnor)
Comment on attachment 182001 [details] [diff] [review]
patch

wrong patch
Attachment #182001 - Attachment is obsolete: true
Attachment #182001 - Flags: review?(mconnor)
Posted patch correct patchSplinter Review
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.
Attachment #182009 - Flags: review?(mconnor)
Shouldn't the chrome.manifest be created for themes automatically the way they
are for extensions?  Or is that a different bug altogether?
(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.
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 on attachment 182009 [details] [diff] [review]
correct patch

This looks good, and seems to work just fine. r=me
Attachment #182009 - Flags: review?(mconnor) → review+
Comment on attachment 182009 [details] [diff] [review]
correct patch

a=caillon for 1.1a
Attachment #182009 - Flags: approval-aviary1.1a+
Checked in.  Thanks!
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
No longer blocks: 291808
*** Bug 291808 has been marked as a duplicate of this bug. ***
*** Bug 291946 has been marked as a duplicate of this bug. ***
Posted file 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". )
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?
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. 
Depends on: 292362
No longer depends on: 292362
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.