291946 - Themes don't get installed.

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Tracy Walker [:tracy]]

seen on Thunderbird linux trunk build 2005-04-26-05-trunk

-download and install on or two Thunderbird profiles using the Theme manager.
notice that on completion of install all theme icons are an open letter. also
note the message "708090-lite will be installed the..." for each new theme
-select a new theme, then click Use theme.
the message changes to "Restart Mozilla Thunderbird to..."
-restart Thunderbird.
The selected default theme remains in use. The message "708090-lite will be
installed the..." for each new theme still exists.

Comment 1

[Tracy Walker [:tracy]]

This bug is also happening with the Extensions Manager.

Comment 2

[Tracy Walker [:tracy]]

This bug is also afflicts Thunderbird Windows trunk build 2005-04-26-08-trunk

Comment 3

[Tracy Walker [:tracy]]

Affects Firefox trunk as well. Seen on Windows Fx build 2005-04-26-07-trunk

Themes do the same thing. However, extensions don't appear at all.

When trying to restart after installing themes and extensions, I got dialogs
about cancelling downloads.

Comment 4

[Scott MacGregor]

Probably fall out from Bug #286034

Comment 5

[Peter van der Woude [:Peter6]]

Fallout from bug 286034 (and probably a dupe)

Comment 6

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

This also occurs with Firefox and this is more than likely fixed by the patch
for bug 291666

Comment 7

[:Gavin Sharp [email: [e-mail]]]

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

Comment 8

[Asa Dotzler [:asa]]

duping against the bug that fixed this.

*** This bug has been marked as a duplicate of 291666 ***

Comment 9

[Tracy Walker [:tracy]]

The extensions part of this bug is fixed by bug 291666 (except on Mac). However,
themes remain broken as originally describe.  Seen on Linux Thunderbird trunk
build 2005-04-29-04-trunk, Windows Firefox trunk build 2005-04-29-07-trunk and
Mac Firefox build 2005-04-29-07-trunk

reopened and adjusted summary to cover just theme installation

Comment 10

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

(In reply to comment #9)
> The extensions part of this bug is fixed by bug 291666 (except on Mac). However,
> themes remain broken as originally describe.  Seen on Linux Thunderbird trunk
> build 2005-04-29-04-trunk, Windows Firefox trunk build 2005-04-29-07-trunk and
> Mac Firefox build 2005-04-29-07-trunk
> 
> reopened and adjusted summary to cover just theme installation
I have a patch for this that does work as stated in bug 292248 and should have
it ready by this evening. Themes currently will install as long as they meet the
requirement of having a chrome.manifest as stated in bug 291666 and there are
several other issues with theme installation the patch fixes.

> taking

Comment 11

[David Lawhon]

I see this bug on the 4/29 trunk Mac build. If it's occurring on both Mac and
Windows Firefox trunk builds, shouldn't the "Hardware" field be changed to "All"?

Comment 12

[Peter van der Woude [:Peter6]]

Tracy does bug 289460 fundamentally differ from this one, or can it be duped (or
visa versa) ?

Comment 13

[Tracy Walker [:tracy]]

bug 289460 should probably have been marked WFM some time ago.  In this bug, the
theme seems to install initially. Then it doesn't take when selected for use and
app restarted.  So I'd say the two bugs are different.

Comment 14

[Asa Dotzler [:asa]]

Ben, is this fallout from your recent changes? We need to get this cleared
before we ship 1.8b2/1.1a

Comment 15

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

Attachment: patch

This is the theme portion of one of the patches in progress from bug 292506. It
includes the patch from bug 251026 that is already r+=bsmedberg & sr+=darin.
There are several other issues fixed in the current patch in bug 292506 and I
was able to apply this patch cleanly after applying the one from bug 292506.

This will allow the installation of themes with or without chrome.manifest
files. One item to note is that it fixes an issue with where Themes with
chrome.manifests will break. Since xpinstall uses CreateUnique when saving a
theme to the profile's chrome dir if there is already a jar file with that name
there the EM has no way of knowing the correct jar file name which is hardcoded
in the chrome.manifest.

Comment 16

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

Benjamin - this is the bug we discussed briefly regarding theme breaking due to
xpinstall renaming the jar files in the case of an upgrade or a reinstall due to
a copy being left behind in the profile's chrome dir. This bug also fixes the EM
code so that it will allow themes that do not have a chrome manifest to install.
It works as is but I would like to also remove the calls to
ProcessContentsManifest and CheckForNewChrome in nsSoftwareUpdateRun.cpp so
there would no longer be a case where a jar file is left behind on theme
install. I don't see a use case where ProcessContentsManifest and
CheckForNewChrome would ever be used here since the EM handles it and thought
you would know of one since you touched that section of code last.

http://lxr.mozilla.org/seamonkey/source/xpinstall/src/nsSoftwareUpdateRun.cpp#678

Also, I don't know if / when Ben is going to have time to review this and
wondered if you would? This bug and bug 292506 take care of a lot of significant
issues with the EM which would be good to get into the alpha.

Comment 17

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

Attachment: patch

Updated to remove the patch from bug 292362 that snuck in here

Comment 18

[Benjamin Smedberg]

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

I never added support for theme chrome.manifests for a reason: the manifests
ought to contain relative URLs, but they are extracted from the JAR file which
means that the relative URLs are wrong. All this stuff with the leafname is a
bad workaround, because you have to assume that the manifest is being extracted
to somewhere in particular. I'm not sure what to do about this.

And yes, it does make sense to remove the if (!installed) block from the
xpinstall code.

Comment 19

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

(In reply to comment #18)
> (From update of attachment 182576 [details] [diff] [review] [edit])
> I never added support for theme chrome.manifests for a reason: the manifests
> ought to contain relative URLs, but they are extracted from the JAR file which
> means that the relative URLs are wrong. All this stuff with the leafname is a
> bad workaround, because you have to assume that the manifest is being extracted
> to somewhere in particular. I'm not sure what to do about this.
I am unsure how this is a problem with an EM initiated install... can you please
explain after taking the following into consideration? The installation
including the directory structure of an extension or a theme is dictated by an
EM installation prior to the new code as well as today. The directory structure
being:
Install-location/{GUID}/chrome.manifest
Install-location/{GUID}/install.rdf
Install-location/{GUID}/chrome/theme.jar
where Install-location is either bin/firefox/ or <profile>
http://wiki.mozilla.org/Firefox:1.1_Extension_Manager_Upgrades

So, the urls are relative and correct in relation to Install-location/GUID/.
There isn't an assumption as to where the chrome.manifest will be extracted to
since it is always extracted to Install-location/{GUID} which is created by the
EM on install. Also, if it is an assumption as to where it is extracted to when
we always extract it to Install-location/{GUID}/ how can it not be an assumption
to always create it in the Install-location/{GUID}/ directory as the code you
wrote to upgrade the chrome does?

The only issue with an EM initiated install then becomes the reliance on
xpinstall in regards to the file name. I am using the same data and method to
get the leafName as xpinstall does in GetDestinationFile::GetDestinationFile...
the only difference is that I am passing this value to installItemFromFile.
http://lxr.mozilla.org/seamonkey/source/xpinstall/src/nsXPInstallManager.cpp#825

A couple of alternatives I considered for dealing with the file being renamed by
xpinstall is to always create a chrome.manifest and to either change the jar
file name in the one provided or create one if one isn't provided. Both of these
seem like a bad workaround once all of the previous points are taken into
consideration.

Comment 20

[Benjamin Smedberg]

When the chrome.manifest is in the JAR file, the relative URIs would be

skin package skinname path/inside/jar/

When the chrome.manifest is extracted, these paths change:

skin package skinname jar:chrome/theme.jar!/path/inside/jar/

I do not particularly want skin authors to go writing manifests that are not
actually correct until you unpack the JAR in a particular way, so I was not
going to support flat manifests in theme JARs for the time being. I could be
convinced to change my mind if an elegant solution to the above could be
found... perhaps a "baseuri" instruction in the flat manifest.

Comment 21

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

(In reply to comment #20)
> When the chrome.manifest is in the JAR file, the relative URIs would be
> 
> skin package skinname path/inside/jar/
> 
> When the chrome.manifest is extracted, these paths change:
> 
> skin package skinname jar:chrome/theme.jar!/path/inside/jar/
I still don't see the problem since the paths that are written in the
chrome.manifest file do not change just because the file is extracted and there
is no code I know of that changes it which has also been confirmed by my
testing. Is there any code that reads a chrome.manifest while it is inside of an
xpi or a jar file in order to get the relative paths that it contains? Whether
the chrome.manifest file is inside of a jar or an xpi file it is always
extracted to Install-location/{GUID}/ and it always contains paths like so when
it is referencing a jar file:

skin package skinname jar:chrome/theme.jar!/path/inside/jar/

In what use case would your first example be applicable to a theme jar file
installation? The only possible case I can see is being able to install a jar
file unpacked which seems like a fringe case and it should be possible to
accomplish this with a theme installation using an xpi. All this is doing is
placing the same format requirements for a theme's chrome.manifest as there is
for an extension's chrome.manifest which I believe has always been the case. The
only significant difference between an extension and a theme in regards to the
chrome.manifest is that with an extension the chrome.manifest is inside of an
xpi. In the case of a theme the chrome.manifest is inside of the jar file
itself. In both cases they are extracted to the exact same location just like
the install.rdf is extracted and in both cases the jar file is placed inside of
a chrome directory inside of the same directory as the chrome.manifest and the
install.rdf.

> I do not particularly want skin authors to go writing manifests that are not
> actually correct until you unpack the JAR in a particular way, so I was not
> going to support flat manifests in theme JARs for the time being. I could be
> convinced to change my mind if an elegant solution to the above could be
> found... perhaps a "baseuri" instruction in the flat manifest.
There is no unpacking of the jar but there is extraction of the chrome.manifest
to Install-location/{GUID}/ just as there is extraction of the install.rdf to
this same location. This is true for both extensions and themes and In both
cases the relative paths are correct when using the format as documented and
provided above.

Comment 22

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

Regarding "baseuri" - inside the extensions-startup.manifest there is already
app-global and app-profile providing the info as to where the Install-location
is as well as the GUID like so.

app-profile	{GUID}	extensions/{GUID}	1115058404	

Inside the Install-location/{GUID}/ there is the chrome.manifest which contains
the relative paths from that location to the jar file, etc. like so:

skin package skinname jar:chrome/theme.jar!/path/inside/jar/

Comment 23

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

Attachment: updated patch

This patch includes the removal of the no longer needed code from
nsSoftwareUpdateRun.cpp to remove another case where a jar file would be left
behind in the profile's chrome dir as well as a simple one liner to
extensions.js to fix a bug with uninstalling a theme that is in use.

In thinking on the comments the only desire I can see for being able to specify
relative paths for theme jar files in the format of
'skin package skinname path/inside/jar/'
is during theme development or possibly when actually packaging a theme but I
don't believe it is any more of a hardship for theme developers as it is for
extension developers while keeping the two formats consistent. You just have to
specify the name of the jar file in the chrome.manifest that you add to the
root of the jar file as you have to specify the name of the jar file in the
chrome.manifest that you add to the root of the xpi file.

Comment 24

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

If the general process used for installing themes prior to 4/26 or in the
attached patch is not acceptable then I suggest the following process.
 
Themes with a chrome.manifest:
If the theme has a chrome.manifest in the root of the archive during the
installation the chrome.manifest would be extracted then parsed and if it has
any lines that specify anything other than a skin package an alert would be
displayed similar to the malformed rdf alert and the theme would not be installed.
 
The directory structure would be similar to what extensions have. This will
allow extracting the jar directly into the extensions directory to install it as
can be done with extensions. This would also allow for the contents of the
chrome directory to be flat file instead of archive based. It would be simple
enough to limit the extracted directory structure for themes to that as outlined
below and thereby prevent a components directory or other additional files
besides those located in the chrome directory if there are any concerns of this
happening. I would also suggest placing the preview and icon images in the root
in order to lessen the hassles caused by zip readers holding the jar file open
when an upgrade or an uninstall occurs.
package.jar
    chrome.manifest
    chrome/theme.jar
    icon.png
    install.rdf
    preview.png
 
Themes without a chrome.manifest:
The chrome can either be upgraded on install as is done with builds before 4/26
as well as the patch and thereby provide backwards compatibility or the
packaging requirements as outlined above could be mandatory.

I have already coded the above process and I'm very sure that I can provide a
patch to meet the requirements outlined above or a variation thereof in less
than a day and if I am not busy in a few hours. I think the above should address
Benjamin's concerns about relative paths in the chrome.manifest since it would
bring themes up to parity with extensions in regards to chrome.manifests.
Regarding the initial concern of the manifest being extracted to somewhere in
particular I believe this is a non-issue since the code always extracts it to
the same location when using any of the methods proposed in the same manner that
it already does this with the install.rdf.

Comment 25

[Benjamin Smedberg]

ok, that sounds like a decent proposal. Let me talk with Ben, but you can
presume that this is ok unless you hear otherwise. We definitely want to
maintain compatibility with the existing structure. It would be good to write up
some docs about this for one of the wikis; I'm not sure where, exactly.

Comment 26

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

Comment on attachment 182605 [details] [diff] [review]
updated patch

>+  upgradeThemeChrome: function() {
>+    var manifestFile = this._installLocation.getItemFile(this._id, FILE_CHROME_MANIFEST);
<..>

What's all of this? Are you fully up to date?

Comment 27

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

Currently, if you install a theme that does not have a chrome.manifest file
using the theme manager or by dropping a theme jar file in the extensions
directory it tries to install the theme but fails with the following assertions
and no alert to the user. The theme's display name is listed in the theme
manager though the theme cannot be used.
*** installTheme: failed to extract install manifest to:
<path>/{2A10B180-05EF-11D9-8C50-444553540001}
*** safeInstallOperation: install operation (caller-supplied callback) failed,
rolling back file moves and aborting inst
allation.
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Component returned failure code: 0x80520006
(NS_ERROR_FILE_TARGET_DOES_NOT_EXIST) [nsIZipReader.extract]"
  nsresult: "0x80520006 (NS_ERROR_FILE_TARGET_DOES_NOT_EXIST)"  location: "JS
frame :: file:///C:/moz/mozilla-source/moz
illa/ff-debug/dist/bin/components/nsExtensionManager.js :: extractThemeFiles ::
line 1267"  data: no]
************************************************************

This error is not caused by not being able to extract the install manifest... it
is caused by there being no chrome.manifest file. The upgradeThemeChrome
function provides a path to upgrade the theme's chrome in this situation.

The current patch is not up to date with the process as described. I will submit
an updated patch this evening that provides the new structure as outlined and
maintains compatibility with the existing structure.

Comment 28

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

Attachment: patch

If the theme has a chrome.manifest it requires the package structure as
outlined and if it doesn't it attempts to upgrade the contents.rdf to a
chrome.manifest. For the new structure it only extracts the install.rdf,
chrome.manifest, icon.png, and preview.png if present to the root and it
extracts everything under the chrome entry into the chrome dir. It doesn't care
if any of these files are present since the check for an install.rdf occurs
well beforehand and exits silently if not present. Perhaps a bug should be
opened for this?

The one thing I am not happy with is the way I shimmed in the error code when a
theme has a chrome.manifest that has entries other than skin. Without this
check at the start an upgrade isn't rolled back to the previous version in part
due to the extensions.rdf already having new data and for new installs some
data is left behind in the extensions.rdf as well.

I have successfully tested installing by dropping in a file, dropping in an
extracted new structure theme dir, and with the themes manager with the new
structure and by dropping in a file and with the themes manager with the old
structure as well as several failure conditions. So far, I haven't seen any
issues within the scope of this bug that the patch doesn't address.

It appears that by using the extracted icon.png and preview.png the need to
notify chrome-flush-skin-caches has been removed so it isn't included in this
patch. If you prefer the method where the icon and preview images are opened
from inside the jar I can put it back that way and add the
chrome-flush-skin-caches back to the patch... either way works though I am
leery of the hold on them that a zip reader can have.

Comment 29

[Benjamin Smedberg]

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

This is getting a lot closer, but I'm still concerned about a few things:

1) Why do we need leafname now? Can't we just use whatever filename xpinstall
decides, for old-style themes? New-style themes obviously don't care.

2) You don't need isValidChromeManifest... the chrome registry checks for skin
packages at runtime.

Other than that, this looks good.

Comment 30

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

Attachment: patch

Addresses the comments... thanks for the review.

Comment 31

[Benjamin Smedberg]

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

Beautiful. Lemme find a driver for quick approval.

Comment 32

[Asa Dotzler [:asa]]

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

a=asa

Comment 33

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

patch checked in - thanks db48x - resolving fixed

Comment 34

[AKIHIRO Misaki (a.k.a Kuden)]

Firefox beast-trunk 05/05/07 07:19:00
Theme install and select
OK -> install.rdf+contents.rdf
Fail -> install.rdf+chrome.manifest
Fail -> install.rdf+contents.rdf+chrome.manifest

Theme's specification is decided to not using chrome.manifest?
http://www.mozilla.org/projects/firefox/extensions/packaging/themes.html

Comment 35

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

The current theme specification can't have a chrome.manifest in the root of the
jar after this patch. This is in part due to that a theme's jar file name can
change during install, etc. which would then break the chrome registration of
the theme when using a theme supplied chrome.manifest. With the current
specification it was decided that the safest method is to dynamically create a
chrome.manifest from a theme's contents.rdf and I believe this is why there is
no mention of using a chrome.manifest on the link you provided for the current
specification.

A new package structure for themes is described in comment #24 that is based on
the same package structure used for extensions. This new structure is not
required as you found with your successful install of a theme that had an
install.rdf and a contents.rdf. I will hopefully have docs for this finished by
the end of this weekend to further describe the structure but I believe the
information in the comment #24 should be sufficient to be able to package a
theme using the new structure.

Comment 36

[Kevin Brosnan]

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

Comment 37

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

I put together 
http://exchangecode.com/robert/themes.html which is based on
http://www.mozilla.org/projects/firefox/extensions/packaging/themes.html

Comment 38

[AKIHIRO Misaki (a.k.a Kuden)]

(In reply to comment #37)
> I put together 
> http://exchangecode.com/robert/themes.html which is based on
> http://www.mozilla.org/projects/firefox/extensions/packaging/themes.html
Thank you. This is comprehensible. I have already experimentally opened the
Theme of this packaging to the public. And, two Theme creators in Japan have
released the Themes by this packaging. These will be tested until 1.1 release. 

Comment 39

[Tracy Walker [:tracy]]

Is it possible the last checkin caused bug 293514?

Comment 40

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

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