(dormant account) Reporter
	Description • 15 years ago

Extensions ship in jar files, but we unpack them(from looking at my firefox installs). 
Opening individual files and directories is very inefficient on hard drives and mobile devices. 
Ideally we'd be able to repack all installed extensions into a single jar in the order of their invocation. Alternatively, keeping extensions in their jars will also be efficient.

	Robert Strong (they/them - no direct email)
	Comment 1 • 15 years ago

We don't unpack the chrome jar's but I know some extension authors distribute their extensions without packaging their chrome in a jar. I know we used to recommend that extension authors ship their extension chrome in a jar file and it is up to the author to choose how they want to distribute their extension.

Perhaps you are asking to put everything (e.g. components, libraries, chrome, prefs, etc.) in a single jar?

	Robert Strong (they/them - no direct email)
	Comment 2 • 15 years ago

forgot to add locale and theme to the list.

btw: the EM manages the install, uninstall, update, enable, disable, etc. for extensions and the extensions follow the formats required by core code so even if this were implemented the EM portion of it is extremely small compared to the changes required to the core.

	(dormant account) Reporter
	Comment 3 • 15 years ago

(In reply to comment #1)
> We don't unpack the chrome jar's but I know some extension authors distribute
> their extensions without packaging their chrome in a jar. I know we used to
> recommend that extension authors ship their extension chrome in a jar file and
> it is up to the author to choose how they want to distribute their extension.

Ok. All of  my extensions seem to unpack files. There are jars in there too, but there are manifest/config/icon/js files outside of jars.

Perhaps better AMO warnings will help there?

> 
> Perhaps you are asking to put everything (e.g. components, libraries, chrome,
> prefs, etc.) in a single jar?

Would be tricky to do native executables/libraries, but yes, I believe everything else could be stuck into one big file. Unfortunately that's a hard sell.
Alternatively we could pray that the OSes will develop filesystems that lay out files/directories in a startup-optimized order :(

	Robert Strong (they/them - no direct email)
	Comment 4 • 15 years ago

The chrome.manifest is a requirement we have placed and I know there has been discussion about storing all of the active extension chrome.manifest files into a single file in the root of the profile directory.

The install.rdf is only read on install / update so that shouldn't be an issue and it is likely to change in the future anyways.

Might be good to figure out a better way to handle defaults/preferences.

install.js in the root of the ext dir is the old style xpinstall which shouldn't ever be read.

components can contain js and / or binary components.

modules contain js modules

chrome, locale, skin can all contain one jar each though there is often just a chrome dir.

Perhaps as a starting point an alternative jar format where all three can be contained within one jar? I suspect more could be done as well.

	(dormant account) Reporter
	Comment 5 • 15 years ago

I asked Ben to get some numbers on this

	Dave Townsend [:mossop]
	Comment 6 • 14 years ago

This bug needs some clarification. The summary does not make sense since as said extensions aren't unpacked on startup so what actually is the problem and how do you propose it could be improved?

	(dormant account) Reporter
	Comment 7 • 14 years ago

(In reply to comment #6)
> This bug needs some clarification. The summary does not make sense since as
> said extensions aren't unpacked on startup so what actually is the problem and
> how do you propose it could be improved?

We need to put an end to dumping files in the app/profile dirs. I would like all possible extension files(save for OS stuff such as .dlls) to live in a single jar. We can provide a virtual filesystem to extensions.

	Dave Townsend [:mossop]
	Comment 8 • 14 years ago

So the goal is that installed extensions are just the xpi file as distributed, which means we need to add support for I think the following as a minimum:

Loading binary and JS components from out of the zip file.
Loading preferences from out of the zip file.
Loading plugins from the zip file.
Loading window icons from the zip file.

The chrome registry is more problematic, can we support loading the chrome out of a zip file within a zip file? Either way we either need to make the chrome registry munge the paths in the chrome manifests to adjust to loading everything out of the zip file regardless or break every single extension for the sake of this.

Are these bugs already filed? If so lets mark them as blocking this, if not lets get them filed.

	Dave Townsend [:mossop]
	Comment 9 • 14 years ago

Note, either way I don't see how we can make this change without breaking any extension that tried to refer to its own files directly (this looks like around 150-200 extensions based on a quick grep of extensions on AMO)

	(dormant account) Reporter
	Comment 10 • 14 years ago

(In reply to comment #8)
> So the goal is that installed extensions are just the xpi file as distributed,
> which means we need to add support for I think the following as a minimum:

That's not quite my goal. I'd like to have multiple extensions flattened into a single jar file.

> Loading binary and JS components from out of the zip file.
sounds like bug 517569
> Loading preferences from out of the zip file.
No bug yet

> Loading plugins from the zip file.
> Loading window icons from the zip file.
Not sure if plugins are feasible. Icons may be troublesome too.

> 
> The chrome registry is more problematic, can we support loading the chrome out
> of a zip file within a zip file? Either way we either need to make the chrome
> registry munge the paths in the chrome manifests to adjust to loading
> everything out of the zip file regardless or break every single extension for
> the sake of this.

I think this is doable.

> 
> Are these bugs already filed? If so lets mark them as blocking this, if not
> lets get them filed.

I'm not too clear on how extension loading currently works, I'd appreciate if you filed the relevant bugs.

	(not currently active) Ted Mielczarek
	Comment 11 • 14 years ago

It is a pretty common idiom to poke at files from the extension install dir. My Extension Developer's Extension, for example, pokes in there to find batch files that it uses to run zip programs (this predates nsIZipWriter):
http://code.google.com/p/extensiondev/source/browse/trunk/content/extensionbuilder.js#1069

	(dormant account) Reporter
	Comment 12 • 14 years ago

(In reply to comment #11)
> It is a pretty common idiom to poke at files from the extension install dir. 

It's ok to poke at files. It's not ok to do so during startup. Would be nice to flag filesystem activities like that as warnings.

	Benjamin Smedberg
	Comment 13 • 14 years ago

Well yeah, but unless you extract the extension when you install it there aren't any files to poke at.

	Dave Townsend [:mossop]
	Comment 14 • 14 years ago

So now we need numbers. I don't think we can consider doing this until we have an idea of how much time is spent statting and reading files in an extension's directory during startup for common extensions and have a realistic estimate of how much we might expect to save by moving them all into a zip file.

Given that (AIUI) chrome files and javascript components are all loaded from the fastload cache rather than from the extension's files after the first use, my gut feeling is that the reward here doesn't warrant the amount of development time for platform and extension authors to implement this.

	Dietrich Ayala (:dietrich)
	Comment 15 • 14 years ago

strace says this:

firefox clean profile, no extensions: 1280 open() calls
firefox clean profile, AMO top 10 extensions: 2973 open() calls

that's a lot.

	(not currently active) Ted Mielczarek
	Comment 16 • 14 years ago

We could do some sort of compromise, where the extension files get unpacked like they normally do, but we instead load everything from a big JAR file, and treat it like fastload. That way the extension's files would be available for poking, but we wouldn't have to touch them in normal startup.

If we added an API to get a stream from a filename in the extension's package, then we could provide that as a migration path to existing extensions, who could use it to read files directly from our new JAR instead. There are still probably uses where this wouldn't suffice, like my extensiondev example above, where we want to execute a batch file shipped with the extension.

Also, I'm curious as to how your "stick all extensions in one JAR" plan would interact with the "install extensions without a restart" plan of Jetpack. We'd have to rewrite a JAR that we have files loaded from, which seems scary.

	Dave Townsend [:mossop]
	Comment 17 • 14 years ago

(In reply to comment #15)
> strace says this:
> 
> firefox clean profile, no extensions: 1280 open() calls
> firefox clean profile, AMO top 10 extensions: 2973 open() calls
> 
> that's a lot.

These numbers are meaningless to me, what do they mean in terms of actual time spent and how do we expect that to change if they all came from a jar?

When you say clean profile do you mean nothing will be in the fastload?

Can we identify what files are actually being touched by all these calls?

	Dietrich Ayala (:dietrich)
	Comment 18 • 14 years ago

(In reply to comment #17)
> These numbers are meaningless to me, what do they mean in terms of actual time
> spent and how do we expect that to change if they all came from a jar?

1. the times i have are useless, since i did the trace on an ssd. i can do it on a slowdrive when i get home, or someone else can do this: "strace -ff firefox-3.6 -no-remote -P yourprofile 2>&1 | grep 'open(' > open.txt"

2. i wasn't removing misses from the log. however, i don't know if i should... it *is* time spent attempting to open files.

> When you say clean profile do you mean nothing will be in the fastload?

no, just noting that i'm not measuring profile/extension setup work. this log should be just non-fastloaded opens.

> Can we identify what files are actually being touched by all these calls?

log, no extensions: http://pastebin.mozilla.org/705877
log, with extensions: http://pastebin.mozilla.org/705876

	Dietrich Ayala (:dietrich)
	Comment 19 • 14 years ago

pastebin truncated the logs, here's the whole thing:

http://people.mozilla.com/~dietrich/open.empty.txt
http://people.mozilla.com/~dietrich/open.extensions.txt

	Dave Townsend [:mossop]
	Comment 20 • 14 years ago

(In reply to comment #19)
> pastebin truncated the logs, here's the whole thing:
> 
> http://people.mozilla.com/~dietrich/open.empty.txt
> http://people.mozilla.com/~dietrich/open.extensions.txt

Looking at the actual files that are being touched for extensions I can see them split into three groups:

XPT files.
This makes sense, we don't cache the actual contents of the xpt files anywhere so we have to either load them on startup or when needed. It looks from the log like we might be doing that on startup so we can either only load when needed (should be possible based on the xpti.dat cache of names), or we could cache their contents somewhere in a single file. From what I recall the xpt loading stuff did support loading out of a zip file or something at one point.

Preferences.
Again we don't cache the default prefs that extensions provide anywhere so these all have to be read on startup right now. There is probably nothing stopping us writing out all of the default preferences into a single canonical file in the profile so we only need to read that, flushing it whenever the set of extensions changes.

Chrome files.
A couple of these seem to be in the logs though not many. It may be worth investigating why they aren't being fastloaded.

I think it is going to far easier to handle each of those cases with better caching or whatever than trying to put all extension files into a single zip.

	Benjamin Smedberg
	Comment 21 • 14 years ago

We *should* be cacheing XPT files in xpti.dat and only reloading them when we re-register!

	(dormant account) Reporter
	Comment 22 • 14 years ago

(In reply to comment #17)

> These numbers are meaningless to me, what do they mean in terms of actual time
> spent and how do we expect that to change if they all came from a jar?
> 

After my blog post I met with a user suffering terrible startup times. He is running firefox from network drives on a corporate network, so any latency from file io is magnified.

I asked him to measure startup with/without extensions, results are pretty telling:
a) startup with extensions 18s
b) statup without extensions 10s
c) warm start without extensions 2s

As to what the expected effect from moving extensions into a single jar would be; I'd expect extension-filled-startup to be pretty much as fast as extension-less.

	Dave Townsend [:mossop]
	Comment 23 • 14 years ago

(In reply to comment #22)
> As to what the expected effect from moving extensions into a single jar would
> be; I'd expect extension-filled-startup to be pretty much as fast as
> extension-less.

I don't think we can make that assumption without knowing how much of those 8 seconds is related to file accesses in the extensions directory. There is certainly other things happening when an extension is installed that this bug wouldn't solve.

	(dormant account) Reporter
	Comment 24 • 14 years ago

(In reply to comment #23)
> 
> I don't think we can make that assumption without knowing how much of those 8
> seconds is related to file accesses in the extensions directory. There is
> certainly other things happening when an extension is installed that this bug
> wouldn't solve.

Like? 

Dietrich's open()-counting script and Rich's startup timing correlate pretty strongly. In Dietrich's case a near doubling of fs-rummaging and in Rich's case a near doubling in startup time. We can do more testing, but I'm pretty sure that a significant proportion of the slowdown is caused by not having extensions contained in a single file.
With the current model we can offer either fast startup or extensions, that sucks.

	Dave Townsend [:mossop]
	Comment 25 • 14 years ago

(In reply to comment #24)
> (In reply to comment #23)
> > 
> > I don't think we can make that assumption without knowing how much of those 8
> > seconds is related to file accesses in the extensions directory. There is
> > certainly other things happening when an extension is installed that this bug
> > wouldn't solve.
> 
> Like? 

Most extension run code on startup, alter the UI, etc. Assuming the fastload cache is working for chrome then by your rationale an extension that contains nothing but chrome should cause no performance impact. I find that hard to believe.

	(dormant account) Reporter
	Comment 26 • 14 years ago

(In reply to comment #25)

> Most extension run code on startup, alter the UI, etc. Assuming the fastload
> cache is working for chrome then by your rationale an extension that contains
> nothing but chrome should cause no performance impact. I find that hard to
> believe.

Let me clarify. By startup in the context of this bug, I mean cold startup. Cold startup is dominated by io. It ranges from 4x to 10x slower than warm in degenerate cases. 
I'm not completely sure what you mean by "nothing but chrome", I know we fastload XUL files only, but I'm not sure if we open extension's jars to get to the chrome stuff there.
Assuming that a "nothing but chrome" extension is ideally fastloaded such that it causes no extra files to be accessed, then yes, since cpus are so much faster than storage media, it will have no significant impact on cold startup. Ie any slowdowns will be measured in milliseconds instead of seconds.

	John J. Barton
	Comment 27 • 14 years ago

(In reply to comment #19)
> pastebin truncated the logs, here's the whole thing:
> 
> http://people.mozilla.com/~dietrich/open.empty.txt
> http://people.mozilla.com/~dietrich/open.extensions.txt

I was surprised to see that the list does not include javascript files.

	(dormant account) Reporter
	Comment 28 • 14 years ago

(In reply to comment #27)
> (In reply to comment #19)
> > pastebin truncated the logs, here's the whole thing:
> > 
> > http://people.mozilla.com/~dietrich/open.empty.txt
> > http://people.mozilla.com/~dietrich/open.extensions.txt
> 
> I was surprised to see that the list does not include javascript files.

Those are fastloaded, though in 3.6 we still stat() them, so they still slow us down on startup. That's fixed on trunk.

	John J. Barton
	Comment 29 • 14 years ago

(In reply to comment #28)
> (In reply to comment #27)
> > (In reply to comment #19)
> > > pastebin truncated the logs, here's the whole thing:
> > > 
> > > http://people.mozilla.com/~dietrich/open.empty.txt
> > > http://people.mozilla.com/~dietrich/open.extensions.txt
> > 
> > I was surprised to see that the list does not include javascript files.
> 
> Those are fastloaded, though in 3.6 we still stat() them, so they still slow us
> down on startup. That's fixed on trunk.

Some browser javascript files are loaded in a precompiled form (curiously denying your argument about infinitely fast CPU). Is that what you mean by 'fastload'? Firebug js files being read when I work on firebug.

	Alfred Kayser
	Comment 30 • 14 years ago

What about letting AMO check that extensions and themes have their chrome contents packed in a jar within the extension xpi?

	Manoj
	Comment 31 • 14 years ago

One quick way to validate Taras's claim about startup being sped up by having all extensions in one jar is to do just that - measure startup with n extensions as independent jars; measure startup with all the extension files in one jar. Is the lack of infrastructure precluding the measurement of startup time with all extensions in a jar?

	Dave Townsend [:mossop]
	Comment 32 • 14 years ago

(In reply to comment #30)
> What about letting AMO check that extensions and themes have their chrome
> contents packed in a jar within the extension xpi?

Doesn't look like this will help a great deal. Everything in the logs are things that cannot currently reside in a jar like the chrome related files can.

(In reply to comment #31)
> One quick way to validate Taras's claim about startup being sped up by having
> all extensions in one jar is to do just that - measure startup with n
> extensions as independent jars; measure startup with all the extension files in
> one jar. Is the lack of infrastructure precluding the measurement of startup
> time with all extensions in a jar?

We cannot currently load a single extension out of a jar let alone all extensions in a single jar.

	Dietrich Ayala (:dietrich)
	Comment 33 • 14 years ago

(In reply to comment #32)
> Everything in the logs are
> things that cannot currently reside in a jar like the chrome related files can.

What cannot currently be loaded from a jar?

	Dietrich Ayala (:dietrich)
	Comment 34 • 14 years ago

(In reply to comment #33)
> What cannot currently be loaded from a jar?

Nm, comments 7 and 8 cover this.

	Dietrich Ayala (:dietrich)
	Comment 35 • 14 years ago

(In reply to comment #10)
> > Are these bugs already filed? If so lets mark them as blocking this, if not
> > lets get them filed.
> 
> I'm not too clear on how extension loading currently works, I'd appreciate if
> you filed the relevant bugs.

I filed a tracker bug, and the dependent bugs for getting all parts of an extension able to be read from the zip: bug 552855.

	Steve Fink [:sfink] [:s:]
	Comment 36 • 14 years ago

As a test proxy for loading all extensions from a single jar, could you put all extensions into a single disk image file and loopback mount it? The direct open() calls would still happen, but from the rotating storage's perspective everything is adjacent and comes from a single file.

	Ben Bucksch (:BenB)
	Comment 37 • 14 years ago

> I'd like to have multiple extensions flattened into a single jar file.

As a user, I want the ability to manually delete the extension.
dietrich counted 1700 open()s for 10 extensions. If we get that down to 10 (one file per extension), that would already be great enough.

	Michael Wu [:mwu] Assignee
	Comment 38 • 14 years ago

This patch generalizes the manifest reading code to read from arbitrary JARs instead of just the omnijar. As a pleasant side-effect, we remove 151 lines of code:

 b/chrome/src/nsChromeRegistry.h           |    6 --
 b/chrome/src/nsChromeRegistryChrome.cpp   |    4 -
 b/modules/libjar/nsZipArchive.h           |    2 
 b/xpcom/components/Makefile.in            |    4 -
 b/xpcom/components/ManifestParser.cpp     |   10 +--
 b/xpcom/components/ManifestParser.h       |    7 +-
 b/xpcom/components/nsComponentManager.cpp |   80 +++++++++++++++---------------
 b/xpcom/components/nsComponentManager.h   |   43 +++++-----------
 xpcom/components/nsManifestZIPLoader.cpp  |   69 -------------------------
 xpcom/components/nsManifestZIPLoader.h    |   56 ---------------------
 10 files changed, 65 insertions(+), 216 deletions(-)

	Michael Wu [:mwu] Assignee
	Comment 39 • 14 years ago

Some care was taken to make sure this api works in non-omnijar non-libxul builds.

	Michael Wu [:mwu] Assignee
	Comment 40 • 14 years ago

I'm actually not a big fan of this api, but it seemed better than requiring the caller to enumerate the jar. Also should probably find an appropriate define for the buffer size..

	Michael Wu [:mwu] Assignee
	Comment 41 • 14 years ago

This adds support for noUnpack in install.rdf. Setting noUnpack to 1 means your extension supports running directly out of its xpi with no extraction required. I'm not a fan of "noUnpack".. a better name would be nice. Also, no extraction isn't supported with extensions found via the windows registry though it should be possible. Extensions installed without unpacking are moved to [profile dir]/extensions/[extension id].xpi. The .xpi on files distinguishes unpacked extensions from extension pointers. 

More polish is required but this has been enough for me to run an unpacked extension.

	Michael Wu [:mwu] Assignee
	Comment 42 • 14 years ago

I downloaded a development version of ABP, converted its chrome packaging to flat packaging, added noUnpack to its install.rdf, and zip'd it back up. Installation results in a file named {d10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d}.xpi in the profile extensions directory. This is backwards compatible with older versions of firefox and simplifies development for extension authors by eliminating the need to pack/unpack jars in the extension chrome directory.

	Michael Wu [:mwu] Assignee
	Comment 43 • 14 years ago

Fix the key used for mKnownJARModules.

	Michael Wu [:mwu] Assignee
	Comment 44 • 14 years ago

Apparently I am terrible about attaching the right patch.

	Michael Wu [:mwu] Assignee
	Comment 45 • 14 years ago

(In reply to comment #44)
> Created attachment 468448 [details] [diff] [review]
> 1. Generalize manifest reading code to read from any jar, v3
> 
> Apparently I am terrible about attaching the right patch.
Also, I missed the two chunks that remove nsManifestZIPLoader.* in this patch. (but it's in my patch queue patch)

	Michael Wu [:mwu] Assignee
	Comment 46 • 14 years ago

This is a pretty terrible hack but it seems to work..

	Michael Wu [:mwu] Assignee
	Comment 47 • 14 years ago

This makes the addon manager not extract extensions by default. From my limited testing, adblock plus, javascript debugger, and noscript work fine.

	(dormant account) Reporter
	Comment 48 • 14 years ago

Comment on attachment 468568 [details] [diff] [review]
5. Allow reading jars within jars


>+  nsJAR* zip = static_cast<nsJAR*>(static_cast<nsIZipReader*>(mInnerZips.Get(&stringKey))); // AddRefs
>+  if (zip) {
>+#ifdef ZIP_CACHE_HIT_RATE
>+    mZipCacheHits++;
>+#endif
>+    zip->ClearReleaseTime();
>+  }
>+  else {
>+    zip = new nsJAR();
>+    if (zip == nsnull)
>+        return NS_ERROR_OUT_OF_MEMORY;

I don't think we are supposed to check for OOM anymore. Use a smart pointer + .forget() instead of this. Still reading through the rest of this hack.

	Michael Wu [:mwu] Assignee
	Comment 49 • 14 years ago

yo dawg i herd you like jars

	Michael Wu [:mwu] Assignee
	Comment 50 • 14 years ago

bitrot fixed

	(dormant account) Reporter
	Comment 51 • 14 years ago

(In reply to comment #50)
> Created attachment 468803 [details] [diff] [review]
> 5. Allow reading jars within jars, v3

This needs at least one testcase for the jar side and probly another few for extension manager.

	(dormant account) Reporter
	Comment 52 • 14 years ago

Comment on attachment 468803 [details] [diff] [review]
5. Allow reading jars within jars, v3

To follow up our irc discussion. Enhancements I'd like to see:
* Port extensionmanager testcases so there is test coverage
* Change nsZipReader to be able to open arbitrary buffers via a hybrid nsZipHandle/nsZipPtr mechanism
* Instead of making a giant/invasive distinction between inner/outer jars key the jars based on urls.(I hope this is feasible)
* No raw pointers on stack
* OOM checks for operator new

The rest of the code looks good.

	Michael Wu [:mwu] Assignee
	Comment 53 • 14 years ago

This bug will be necessary to fix bug 531886 (by stat'ing everything again).

	Benjamin Smedberg
	Comment 55 • 14 years ago

I'm not sure I understand the jars-within-jars bit. We already support nested JARs using URIs:

jar:jar:some.jar!/other.jar!/inner.path

Why can't we just use URIs?

	Benjamin Smedberg
	Comment 56 • 14 years ago

Comment on attachment 468448 [details] [diff] [review]
1. Generalize manifest reading code to read from any jar, v3

>diff --git a/xpcom/components/nsComponentManager.cpp b/xpcom/components/nsComponentManager.cpp

> #ifdef MOZ_OMNIJAR
> void
>-nsComponentManagerImpl::RegisterOmnijar(const char* aPath, bool aChromeOnly)
>+nsComponentManagerImpl::RegisterOmnijar(nsIZipReader* aReader,
>+                                        const char* aPath, bool aChromeOnly)

I don't understand why you changed the signature of this method. Why not continue to get the omnijar nsIZipReader within this method?

> void
> nsComponentManagerImpl::ManifestManifest(ManifestProcessingContext& cx, int lineno, char *const * argv)
> {
>     char* file = argv[0];
> 
>-#ifdef MOZ_OMNIJAR
>     if (cx.mPath) {
>         nsCAutoString manifest(cx.mPath);
>         AppendFileToManifestPath(manifest, file);
> 
>-        RegisterOmnijar(manifest.get(), cx.mChromeOnly);
>+        RegisterOmnijar(cx.mReader, manifest.get(), cx.mChromeOnly);

This doesn't look right. RegisterOmnijar is still a MOZ_OMNIJAR-only. If you mean to use it for extension JARs also, you need to rename it to RegisterJARRedManifest and remove the MOZ_OMNIJAR ifdefs.


>     }
>     else
>-#endif
>     {

Here and elsewhere, please fix the bracing to use standard style, e.g. "else {". It was only nonstandard because of the ifdef.

>-        km = mKnownJARModules.Get(manifest);
>+        nsCAutoString hash;
>+        cx.mFile->GetNativePath(hash);
>+        hash.Append(manifest);

Please insert a "|" between the two parts of the hash. Also, please document the format of the hash in nsComponentManager.h

> nsCString
> nsComponentManagerImpl::KnownModule::Description() const
> {
>     nsCString s;
>-#ifdef MOZ_OMNIJAR
>     if (!mPath.IsEmpty()) {
>         s.AssignLiteral("omnijar:");
>         s.Append(mPath);
>     }
>     else
>-#endif

This isn't right any more. I know it's just debugging code, but please have the description include the JAR filename as well as the path, instead of hardcoding "omnijar:".

I'd like to review an interdiff of the requested changes (a new MQ patch on top of this one).

	Benjamin Smedberg
	Comment 57 • 14 years ago

Comment on attachment 467686 [details] [diff] [review]
2. Add api for reading manifests from jars

I don't think we should have separate sJarModuleLocations and sModuleLocations: they are supposed to be registered in the order that XRE_Add* was called. Instead, I would just add a boolean flag on to ModuleLocation indicating whether it's a "regular" location or a JAR location.

This needs tests!

	Benjamin Smedberg
	Comment 58 • 14 years ago

Comment on attachment 467688 [details] [diff] [review]
3. Support reading preferences from arbitrary jars

dwitte, can you prioritize this review? I'm not sure we want to be changing nsIPrefService now, so maybe we could leave that method in and make it a no-op.

	dwitte@gmail.com
	Comment 59 • 14 years ago

Comment on attachment 467688 [details] [diff] [review]
3. Support reading preferences from arbitrary jars

>diff --git a/modules/libpref/public/nsIPrefService.idl b/modules/libpref/public/nsIPrefService.idl
>@@ -105,6 +106,19 @@ interface nsIPrefService : nsISupports
>    */
>   void savePrefFile(in nsIFile aFile);
> 
>+  /**
>+   * Called to read the preferences in the defaults/preferences/
>+   * directory of a zip file
>+   *
>+   * @param aFile The zip file to be read.
>+   *
>+   * @return NS_OK The file was read and processed.
>+   * @return Other The file failed to read or contained invalid data.
>+   *
>+   * @see readPrefFile
>+   * @see nsIInputStream

Don't need to mention nsIInputStream here. Also, s/readPrefFile/readUserPrefs/?

>diff --git a/modules/libpref/src/nsPrefService.cpp b/modules/libpref/src/nsPrefService.cpp

>+NS_IMETHODIMP nsPrefService::ReadExtensionPrefs(nsILocalFile *aFile)
>+{

Need a content process guard here. See other mutation methods in this file.

>+  nsAutoArrayPtr<char> buffer(new char[4096]);

Could declare this on the stack.

>+    while (NS_SUCCEEDED(rv = stream->Available(&avail)) && avail) {
>+      rv = stream->Read(buffer, 4096, &read);
>+      if (NS_FAILED(rv)) {
>+        NS_WARNING("Pref stream read failed");
>+        break;
>+      }
>+
>+      rv = PREF_ParseBuf(&ps, buffer, read);
>+      if (NS_FAILED(rv)) {
>+        NS_WARNING("Pref stream parse failed");
>+        break;
>+      }
>+    }
>+    PREF_FinalizeParseState(&ps);
>+  }
>+  return NS_OK;

ITYM 'return rv' here.

Holding off on r+ to get clarification on bsmedberg's comment. (You're not removing a method afaict!, and I think adding a method is OK, since it won't affect script.)

	Ben Bucksch (:BenB)
	Comment 60 • 14 years ago

ReadExtensionPrefs() would be used from extension manager, not the extension, right? Isn't that an internal API, then, which could be put on nsIPrefServiceInternal or some other IDL? nsIPrefService is very often used, and changing C++ ABI means means breaking a lot of components. Even if they need only a recompile, still that's serious hassle for some extension authors, and I think unnecessary here, esp. given the core IDL this is.

	Ben Bucksch (:BenB)
	Comment 61 • 14 years ago

> ReadExtensionPrefs() would be used from extension manager, not the extension,
> right?

FWIW, that is missing from the documentation comment. Add "typically", if you must.

	dwitte@gmail.com
	Comment 62 • 14 years ago

Well, they're playing with fire if they don't rebuild for 2.0, because we've made it pretty clear that we're breaking binary compat. I'm really not sure how much we're caring about pure binary compat. (Obviously script is a different story!)

Didn't we already change the way components are registered? That alone guarantees a rebuild.

	Benjamin Smedberg
	Comment 63 • 14 years ago

If we were in normal trunk development, changing the IDL would be a no-brainer. But we've already released 3 betas with the new registration scheme, and I bet that most binary components use nsIPrefService at some point. I think we should try to be compatible here and avoid any change to nsIPrefService. As an alternative, that can be sent through nsIObserver.

	dwitte@gmail.com
	Comment 64 • 14 years ago

Comment on attachment 467688 [details] [diff] [review]
3. Support reading preferences from arbitrary jars

bsmedberg made the argument that many extensions have probably built based on betas already, so changing the IID at this point isn't smart. r=me for adding an nsIPrefServiceInternal.

	Ben Bucksch (:BenB)
	Comment 65 • 14 years ago

OK, but this really looks like an internal API. So, if you want to change this function later, you'd break script compat as well. I think prefservice/branch is such an often-used API that it should be geared towards the public users and designed to be stable.

	Ben Bucksch (:BenB)
	Comment 66 • 14 years ago

Thanks. (My last comment was in reply to comment 62.)

	(dormant account) Reporter
	Comment 67 • 14 years ago

Comment on attachment 468877 [details] [diff] [review]
5a. nsZipArchive changes for reading jars within jars

># HG changeset patch
># Parent 971f04e8aee36af522e302ff3d74258c06d62f21
>
>diff --git a/modules/libjar/nsJAR.cpp b/modules/libjar/nsJAR.cpp
>--- a/modules/libjar/nsJAR.cpp
>+++ b/modules/libjar/nsJAR.cpp
>@@ -170,7 +170,17 @@ nsJAR::Open(nsIFile* zipFile)
>   mLock = PR_NewLock();
>   NS_ENSURE_TRUE(mLock, NS_ERROR_OUT_OF_MEMORY);
> 
>-  return mZip.OpenArchive(zipFile);
>+  nsresult rv;
>+  nsCOMPtr<nsILocalFile> localFile = do_QueryInterface(zipFile, &rv);
>+  if (NS_FAILED(rv))
>+    return rv;
>+
>+  nsRefPtr<nsZipHandle> handle;
>+  rv = nsZipHandle::Init(localFile, getter_AddRefs(handle));
>+  if (NS_FAILED(rv))
>+    return rv;
>+
>+  return mZip.OpenArchive(handle);

For convenience and to reduce code duplication, there should still be an nsZipArchive::OpenArchive(nsIFile), but it should call nsZipArchive::OpenArchive(nsZipHandle)

> 
>+nsresult nsZipHandle::Init(nsZipArchive *zip, const char *entry,
>+                           nsZipHandle **ret)
>+{
>+  nsZipHandle *handle = new nsZipHandle();
>+  if (!handle)
>+    return NS_ERROR_OUT_OF_MEMORY;
>+
>+  handle->mBuf = new nsZipItemPtr<PRUint8>(zip, entry);
>+  if (!handle->mBuf) {
>+    delete handle;
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  }
>+
>+  handle->mMap = nsnull;
>+  handle->mLen = handle->mBuf->Length();
>+  handle->mFileData = handle->mBuf->Buffer();
>+  handle->AddRef();
>+  *ret = handle;

Should use nsRefPtr + .forget for handle here(this bad habit is copied from my code)
> 
>-    if (NS_FAILED(zipReader->OpenArchive(sOmnijarPath))) {
>+    nsRefPtr<nsZipHandle> handle;
>+    nsZipHandle::Init(sOmnijarPath, getter_AddRefs(handle));
>+    if (!handle) {
>+        NS_IF_RELEASE(sOmnijarPath);
>+        return;
>+    }

Should change this to the way it was before once you add the convenience OpenArchive mentioned above.

Overall this is very nice. r+ with comments addressed.

	dwitte@gmail.com
	Comment 68 • 14 years ago

Also, I just noticed we're not guarding against content process in the Observe method. Sigh. I'm going to file a bug to just fork the prefservice between content and parent processes, to make all this stuff more robust and clear.

	dwitte@gmail.com
	Comment 69 • 14 years ago

Hmm, actually, let's just keep this simple (there's a lot of code that needs to be shared). While you're in there, can you stick a content process guard at the top of nsPrefService::Observe? No need to NS_ERROR, just silently return failure.

	Michael Wu [:mwu] Assignee
	Comment 70 • 14 years ago

(In reply to comment #55)
> I'm not sure I understand the jars-within-jars bit. We already support nested
> JARs using URIs:
> 
> jar:jar:some.jar!/other.jar!/inner.path
> 
> Why can't we just use URIs?

Nested jars are only supported when doing non-blocking IO. This fails if someone tries to get an nsIInputStream. See http://hg.mozilla.org/mozilla-central/file/1f9925cb9bf8/modules/libjar/nsJARChannel.cpp#l352

	Michael Wu [:mwu] Assignee
	Comment 71 • 14 years ago

(In reply to comment #56)
> Comment on attachment 468448 [details] [diff] [review]
> 1. Generalize manifest reading code to read from any jar, v3
> 
> >diff --git a/xpcom/components/nsComponentManager.cpp b/xpcom/components/nsComponentManager.cpp
> 
> > #ifdef MOZ_OMNIJAR
> > void
> >-nsComponentManagerImpl::RegisterOmnijar(const char* aPath, bool aChromeOnly)
> >+nsComponentManagerImpl::RegisterOmnijar(nsIZipReader* aReader,
> >+                                        const char* aPath, bool aChromeOnly)
> 
> I don't understand why you changed the signature of this method. Why not
> continue to get the omnijar nsIZipReader within this method?
> 
Because in a later patch, this function will be used to load manifests from arbitrary jars.

> > void
> > nsComponentManagerImpl::ManifestManifest(ManifestProcessingContext& cx, int lineno, char *const * argv)
> > {
> >     char* file = argv[0];
> > 
> >-#ifdef MOZ_OMNIJAR
> >     if (cx.mPath) {
> >         nsCAutoString manifest(cx.mPath);
> >         AppendFileToManifestPath(manifest, file);
> > 
> >-        RegisterOmnijar(manifest.get(), cx.mChromeOnly);
> >+        RegisterOmnijar(cx.mReader, manifest.get(), cx.mChromeOnly);
> 
> This doesn't look right. RegisterOmnijar is still a MOZ_OMNIJAR-only. If you
> mean to use it for extension JARs also, you need to rename it to
> RegisterJARRedManifest and remove the MOZ_OMNIJAR ifdefs.
> 
Done in the second patch.

> 
> >     }
> >     else
> >-#endif
> >     {
> 
> Here and elsewhere, please fix the bracing to use standard style, e.g. "else
> {". It was only nonstandard because of the ifdef.
> 
Ok.

> >-        km = mKnownJARModules.Get(manifest);
> >+        nsCAutoString hash;
> >+        cx.mFile->GetNativePath(hash);
> >+        hash.Append(manifest);
> 
> Please insert a "|" between the two parts of the hash. Also, please document
> the format of the hash in nsComponentManager.h
> 
Ok.

> > nsCString
> > nsComponentManagerImpl::KnownModule::Description() const
> > {
> >     nsCString s;
> >-#ifdef MOZ_OMNIJAR
> >     if (!mPath.IsEmpty()) {
> >         s.AssignLiteral("omnijar:");
> >         s.Append(mPath);
> >     }
> >     else
> >-#endif
> 
> This isn't right any more. I know it's just debugging code, but please have the
> description include the JAR filename as well as the path, instead of hardcoding
> "omnijar:".
> 
Done in the second patch.

	Michael Wu [:mwu] Assignee
	Comment 72 • 14 years ago

- s/else$/else {/
- sJarModuleLocations removed
- bool jar added to ComponentLocation
- comment about the hash used for mKnownJARModules added
- TestRegistrationOrder modified to test XRE_AddJarManifestLocation. extension2.jar is basically the files in the regorder/extension directory modified to use @mozilla.org/RegTestServiceB;1 (and the appropriate UUID) plus a chrome.manifest to load the extComponent.manifest.

	Alfred Kayser
	Comment 73 • 14 years ago

Note, be careful not to keep the themes in their original jar/xpi file.
What happens generally, if a user as multiple themes, only one theme will be active (always), so when opening the theme list in the Addons manager, the addons manager code will try to get the icons from each theme from the jar files, even the ones that are not used. This stuffs the zipcache...

This also applies to installed but disabled extensions.
Keeping the metadata outside the jarfile of the extension/theme, prevents opening a jarfile for disabled extensions.

	Michael Wu [:mwu] Assignee
	Comment 74 • 14 years ago

(In reply to comment #73)
> Note, be careful not to keep the themes in their original jar/xpi file.
> What happens generally, if a user as multiple themes, only one theme will be
> active (always), so when opening the theme list in the Addons manager, the
> addons manager code will try to get the icons from each theme from the jar
> files, even the ones that are not used. This stuffs the zipcache...
> 
Thanks for the heads up. If icons are the only problem with keeping themes in their xpis, we can fix it.

> This also applies to installed but disabled extensions.
> Keeping the metadata outside the jarfile of the extension/theme, prevents
> opening a jarfile for disabled extensions.
Disabled extensions are probably ok. The extension manager caches most of the metadata so we don't need to read jars of disabled extensions to grab the install.rdf metadata.

	Michael Wu [:mwu] Assignee
	Comment 75 • 14 years ago

Review comments addressed.

	Michael Wu [:mwu] Assignee
	Comment 76 • 14 years ago

mInnerZips eliminated.

	Michael Wu [:mwu] Assignee
	Comment 77 • 14 years ago

How does this look?

	dwitte@gmail.com
	Comment 78 • 14 years ago

Comment on attachment 469597 [details] [diff] [review]
3a. Interdiff with review fixes

>diff --git a/modules/libpref/public/nsIPrefService.idl b/modules/libpref/public/nsIPrefService.idl

>+[scriptable, uuid(c47fabd4-bd59-47f6-bc1f-aefc9de0fcb7)]
>+interface nsIPrefServiceInternal : nsIPrefService

I'd rather this be a separate interface inheriting from nsISupports.

r=dwitte

	(dormant account) Reporter
	Comment 79 • 14 years ago

Comment on attachment 469556 [details] [diff] [review]
5b. Allow reading jars within jars, v4

This approach is a bit neater. 
As I mentioned before, please don't do naked new nsJar() allocation. comptr+forget there.

Alfred can you take a look at this too?

	Dave Townsend [:mossop]
	Comment 83 • 14 years ago

Really not convinced that we should take this this late but apparently it is needed.

	Dave Townsend [:mossop]
	Comment 84 • 14 years ago

Comment on attachment 469666 [details] [diff] [review]
4. Support non-extracting extensions in extension manager, v3

wtb more context

I'm not sure that storing the xpi files for these in the extensions directory is a great idea, it makes for a pretty terrible downgrade user experience.

This needs a bunch of tests, I'd be interested to see if we could just duplicate the xpcshell test suite and run the tests once for unpacking stuff and once for not.

We support using icon.png from the root of the extension. The way this patch stands at the moment doing so will use the jar: protocol which locks the xpi meaning we'd no longer be able to uninstall those extensions without a restart.

Actually it looks like restartless extensions are completely ignored here and have to be unpacked anyway. Given that we want to move most extensions to be restartless not supporting it would seem to make this patch a waste of time.

Please fix comments throughout where it is talking about the add-on's directory and could in fact mean the add-on's XPI.

How does this code handling finding both .xpi and non-.xpi forms in the staging directory and in the install location?

># HG changeset patch
># Parent 07ce65b6c3534f53fa924a6c08c192c1e9bdf011
>
>diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm

>   PROP_METADATA.forEach(function(aProp) {
>     addon[aProp] = getRDFProperty(ds, root, aProp);
>   });
>+  addon.unpack = getRDFProperty(ds, root, "unpack");
>+  if (addon.unpack != 1) {
>+    addon.unpack = 0;
>+  }

Please use true/false as string values in the rdf and boolean in the JS.
Looks like you're defaulting to not unpacking extensions, is that really the right way to go?

>       while (entries.hasMoreElements()) {
>         let stageDirEntry = entries.getNext().QueryInterface(Ci.nsILocalFile);
> 
>-        // Only directories are important. Files may be updated manifests.
>+        let id = stageDirEntry.leafName;
>+        let directLoad = false;
>         if (!stageDirEntry.isDirectory()) {
>-          WARN("Ignoring file: " + stageDirEntry.path);
>-          continue;
>+          if (id.substring(id.length - 4, id.length).toLowerCase() == ".xpi") {
>+            id = id.substring(0, id.length - 4);
>+            directLoad = true;
>+          }
>+          else {
>+            WARN("Ignoring file: " + stageDirEntry.path);
>+            continue;
>+          }

Can you add a test here that makes it ignore .json files too please.

>         entry = newEntry;
>       }
>-      else if (!entry.isDirectory()) {
>-        LOG("Ignoring entry which isn't a directory: " + entry.path);
>+      else if (entry.isDirectory() && directLoad) {
>+        LOG("Ignoring xpi which isn't a file: " + entry.path);
>         continue;
>       }

This looks like it will break any regular add-on with an ID that ends in ".xpi". Please make sure to add tests for unpacked extensions with IDs like that.

>     if (dir.exists())
>       dir.remove(true);
> 
>+    dir = this._directory.clone().QueryInterface(Ci.nsILocalFile);
>+    dir.append(aId + ".xpi");

Just do dir.leafName += ".xpi"

>diff --git a/toolkit/xre/nsXREDirProvider.cpp b/toolkit/xre/nsXREDirProvider.cpp

>-    aDirectories.AppendObject(dir);
>+    if (Substring(path, path.Length() - 4).Equals(NS_LITERAL_CSTRING(".xpi"))) {
>+      XRE_AddJarManifestLocation(aType, dir);
>+      if (!prefs)
>+        continue;
>+      prefs->ReadExtensionPrefs(dir);
>+    }
>+    else {
>+      aDirectories.AppendObject(dir);

It seems wrong to me that we read extension prefs from different places depending on how they are installed.

	Jorge Villalobos [:jorgev] (he/him)
	Comment 85 • 14 years ago

(In reply to comment #84)
> Comment on attachment 469666 [details] [diff] [review] 
> Actually it looks like restartless extensions are completely ignored here and
> have to be unpacked anyway. Given that we want to move most extensions to be
> restartless not supporting it would seem to make this patch a waste of time.

I don't think it's realistic to expect that restartless add-ons will be a significant portion of add-on numbers in the near future. Most add-ons require an overlay and the move to Jetpack/restartless extensions will be very slow.

	Michael Wu [:mwu] Assignee
	Comment 86 • 14 years ago

(In reply to comment #84)
> Comment on attachment 469666 [details] [diff] [review]
> 4. Support non-extracting extensions in extension manager, v3
> 
> wtb more context
> 
Will do.

> I'm not sure that storing the xpi files for these in the extensions directory
> is a great idea, it makes for a pretty terrible downgrade user experience.
> 
Actually, it was done for a good downgrade experience. Older versions of firefox install xpis found in the extension directory. There's just two problems: The first is that installation via the extension directory prompts the user. The second is that the extension manager doesn't try to install xpis with an @ in the file name. We should be able to work around the second problem.

> This needs a bunch of tests, I'd be interested to see if we could just
> duplicate the xpcshell test suite and run the tests once for unpacking stuff
> and once for not.
> 
Hoping this will be the case. Will be too much of a pain if we can't do that.

> We support using icon.png from the root of the extension. The way this patch
> stands at the moment doing so will use the jar: protocol which locks the xpi
> meaning we'd no longer be able to uninstall those extensions without a restart.
> 
I'll look into caching the icons elsewhere. This will let us always show the icon.

> Actually it looks like restartless extensions are completely ignored here and
> have to be unpacked anyway. Given that we want to move most extensions to be
> restartless not supporting it would seem to make this patch a waste of time.
> 
I'll take a closer look. We should be able to load those without unpacking.

> Please fix comments throughout where it is talking about the add-on's directory
> and could in fact mean the add-on's XPI.
> 
Ok,

> How does this code handling finding both .xpi and non-.xpi forms in the staging
> directory and in the install location?
> 
The logic should be: if the entry is a directory, load it as usual. If not, check if it ends with .xpi and install it if it does.

> ># HG changeset patch
> ># Parent 07ce65b6c3534f53fa924a6c08c192c1e9bdf011
> >
> >diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm
> 
> >   PROP_METADATA.forEach(function(aProp) {
> >     addon[aProp] = getRDFProperty(ds, root, aProp);
> >   });
> >+  addon.unpack = getRDFProperty(ds, root, "unpack");
> >+  if (addon.unpack != 1) {
> >+    addon.unpack = 0;
> >+  }
> 
> Please use true/false as string values in the rdf and boolean in the JS.
> Looks like you're defaulting to not unpacking extensions, is that really the
> right way to go?
> 
bsmedberg suggested doing opt out. With the addition of being able to read jars from jars, most extensions without binary components are automatically compatible.

> >       while (entries.hasMoreElements()) {
> >         let stageDirEntry = entries.getNext().QueryInterface(Ci.nsILocalFile);
> > 
> >-        // Only directories are important. Files may be updated manifests.
> >+        let id = stageDirEntry.leafName;
> >+        let directLoad = false;
> >         if (!stageDirEntry.isDirectory()) {
> >-          WARN("Ignoring file: " + stageDirEntry.path);
> >-          continue;
> >+          if (id.substring(id.length - 4, id.length).toLowerCase() == ".xpi") {
> >+            id = id.substring(0, id.length - 4);
> >+            directLoad = true;
> >+          }
> >+          else {
> >+            WARN("Ignoring file: " + stageDirEntry.path);
> >+            continue;
> >+          }
> 
> Can you add a test here that makes it ignore .json files too please.
> 
Ok.

> >         entry = newEntry;
> >       }
> >-      else if (!entry.isDirectory()) {
> >-        LOG("Ignoring entry which isn't a directory: " + entry.path);
> >+      else if (entry.isDirectory() && directLoad) {
> >+        LOG("Ignoring xpi which isn't a file: " + entry.path);
> >         continue;
> >       }
> 
> This looks like it will break any regular add-on with an ID that ends in
> ".xpi". Please make sure to add tests for unpacked extensions with IDs like
> that.
> 
Ok. That's definitely a mistake.. the logic should be to check for a .xpi iff the entry isn't a directory.

> >     if (dir.exists())
> >       dir.remove(true);
> > 
> >+    dir = this._directory.clone().QueryInterface(Ci.nsILocalFile);
> >+    dir.append(aId + ".xpi");
> 
> Just do dir.leafName += ".xpi"
> 
> >diff --git a/toolkit/xre/nsXREDirProvider.cpp b/toolkit/xre/nsXREDirProvider.cpp
> 
> >-    aDirectories.AppendObject(dir);
> >+    if (Substring(path, path.Length() - 4).Equals(NS_LITERAL_CSTRING(".xpi"))) {
> >+      XRE_AddJarManifestLocation(aType, dir);
> >+      if (!prefs)
> >+        continue;
> >+      prefs->ReadExtensionPrefs(dir);
> >+    }
> >+    else {
> >+      aDirectories.AppendObject(dir);
> 
> It seems wrong to me that we read extension prefs from different places
> depending on how they are installed.

We could let the pref service figure it out I think. I'll take a look. Should be a bit more consistent.

	Dave Townsend [:mossop]
	Comment 87 • 14 years ago

(In reply to comment #86)
> (In reply to comment #84)
> > Comment on attachment 469666 [details] [diff] [review] [details]
> > 4. Support non-extracting extensions in extension manager, v3
> > 
> > wtb more context
> > 
> Will do.
> 
> > I'm not sure that storing the xpi files for these in the extensions directory
> > is a great idea, it makes for a pretty terrible downgrade user experience.
> > 
> Actually, it was done for a good downgrade experience. Older versions of
> firefox install xpis found in the extension directory. There's just two
> problems: The first is that installation via the extension directory prompts
> the user. The second is that the extension manager doesn't try to install xpis
> with an @ in the file name. We should be able to work around the second
> problem.

The first problem is pretty ugly as it is but the third problem is I think the older version will reject incompatible add-ons meaning you can downgrade then upgrade and lose extensions.

> > How does this code handling finding both .xpi and non-.xpi forms in the staging
> > directory and in the install location?
> > 
> The logic should be: if the entry is a directory, load it as usual. If not,
> check if it ends with .xpi and install it if it does.

I don't think that was matching the code I was seeing but I'll take a closer look on the next pass.

	Michael Wu [:mwu] Assignee
	Comment 88 • 14 years ago

All but three tests pass now on linux/osx. Those three tests are uninteresting when not unpacking extensions.

Remaining things to do:
1. Rerun addon manager tests without packing, probably with a pref.
2. Add a <em:unpack>true</em:unpack> test.
3. Adjust comments to reflect that extensions may just be xpis now.
4. Add another api for flushing entries in the zip cache without spinning the event loop.

	Pardal Freudenthal (:ShareBird)
	Comment 89 • 14 years ago

I don't know if I understand this bug correctly... If the issue is that some extensions have no jars inside their xpis (or inside jars in the case of some themes) it would be easier to just make AMO check for it as proposed on comment #30. I'm concerned about how this will impact development's workflow since developers use to work in an unpacked installation of their extensions and only compress them to ship them.

	Michael Wu [:mwu] Assignee
	Comment 90 • 14 years ago

(In reply to comment #89)
> I don't know if I understand this bug correctly... If the issue is that some
> extensions have no jars inside their xpis (or inside jars in the case of some
> themes) it would be easier to just make AMO check for it as proposed on comment
> #30. I'm concerned about how this will impact development's workflow since
> developers use to work in an unpacked installation of their extensions and only
> compress them to ship them.

Unpacked extension directories are still fully supported so development workflows aren't affected. These patches only affect the installation/update of new extensions. It also simplifies the development workflow by making the creation of jars in the chrome directory optional when shipping.

	Pardal Freudenthal (:ShareBird)
	Comment 91 • 14 years ago

Thank you Michael for clarifying this.

	Michael Wu [:mwu] Assignee
	Comment 92 • 14 years ago

Necessary for no-unpack no-restart extensions.

	Benjamin Smedberg
	Comment 93 • 14 years ago

Comment on attachment 472711 [details] [diff] [review]
7. Add support for loading jar uris in the js subscript loader

Use NS_GetInnermostURI.

	Michael Wu [:mwu] Assignee
	Comment 94 • 14 years ago

This allows the addons code to drop handles to specific files in case the file was updated (linux/osx) or we need to delete it (windows). As a bonus, the addons code will be able to refer to icons/images within uninstalled xpis and be able to delete the xpis later on windows.

	Michael Wu [:mwu] Assignee
	Comment 95 • 14 years ago

Patch greatly simplified by NS_GetInnermostURI.

	(dormant account) Reporter
	Comment 96 • 14 years ago

Comment on attachment 472717 [details] [diff] [review]
8. Add flush-cache-entry topic

># HG changeset patch
># Parent 03d88fbb750400b814474b22907449bb4a83d866
>
>diff --git a/modules/libjar/nsJAR.cpp b/modules/libjar/nsJAR.cpp
>--- a/modules/libjar/nsJAR.cpp
>+++ b/modules/libjar/nsJAR.cpp
>@@ -1050,6 +1050,7 @@ nsZipReaderCache::Init(PRUint32 cacheSiz
>   {
>     os->AddObserver(this, "memory-pressure", PR_TRUE);
>     os->AddObserver(this, "chrome-flush-caches", PR_TRUE);
>+    os->AddObserver(this, "flush-cache-entry", PR_TRUE);
>   }
> // ignore failure of the observer registration.
> 
>@@ -1317,6 +1318,32 @@ nsZipReaderCache::Observe(nsISupports *a
>     mZips.Enumerate(DropZipReaderCache, nsnull);
>     mZips.Reset();
>   }
>+  else if (strcmp(aTopic, "flush-cache-entry") == 0) {
>+    nsCOMPtr<nsIFile> file = do_QueryInterface(aSubject);
>+    if (!file)
>+      return NS_OK;
>+
>+    nsCAutoString uri;
>+    if (NS_FAILED(file->GetNativePath(uri)))
>+      return NS_OK;
>+
>+    uri.Insert(NS_LITERAL_CSTRING("file:"), 0);
>+    nsCStringKey key(uri);
>+
>+    nsAutoLock lock(mLock);    
>+    nsJAR* zip = static_cast<nsJAR*>(static_cast<nsIZipReader*>(mZips.Get(&key)));
>+    if (!zip)
>+      return NS_OK;
>+
>+#ifdef ZIP_CACHE_HIT_RATE
>+    mZipCacheFlushes++;
>+#endif
>+
>+    zip->SetZipReaderCache(nsnull);
>+
>+    mZips.Remove(&key);
>+    NS_RELEASE(zip);
>+  }
>   return NS_OK;
> }
>

	Michael Wu [:mwu] Assignee
	Comment 97 • 14 years ago

Tests are run with and without unpacking now. Two tests are disabled when testing extensions without unpacking. Will follow up with a test to test <em:unpack>true</em:unpack>.

	Dave Townsend [:mossop]
	Comment 99 • 14 years ago

Comment on attachment 472819 [details] [diff] [review]
4. Support non-extracting extensions in extension manager, v5

Going through this in detail now but it still isn't well tested. Though you are running all of the tests twice a pretty sizable proportion of the tests do their work by writing extensions directly into the profile extensions directory in unpacked form using writeInstallRDFToDir. These tests need to also be run for the packed cases.

	Dave Townsend [:mossop]
	Comment 100 • 14 years ago

Comment on attachment 472819 [details] [diff] [review]
4. Support non-extracting extensions in extension manager, v5

>diff --git a/modules/libpref/src/init/all.js b/modules/libpref/src/init/all.js
>--- a/modules/libpref/src/init/all.js
>+++ b/modules/libpref/src/init/all.js
>@@ -3217,11 +3217,12 @@ pref("html5.flushtimer.subsequentdelay",
> // Push/Pop/Replace State prefs
> pref("browser.history.allowPushState", true);
> pref("browser.history.allowReplaceState", true);
> pref("browser.history.allowPopState", true);
> pref("browser.history.maxStateObjectSize", 655360);
> 
> // XPInstall prefs
> pref("xpinstall.whitelist.required", true);
>+pref("xpinstall.unpack", false);

This preference should be extensions.alwaysUnpack I think.

>diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm

>-        // Only directories are important. Files may be updated manifests.
>+        let id = stageDirEntry.leafName;
>+        let directLoad = false;

This directLoad variable seems unnecessary when you can just use isFile()

>         if (!stageDirEntry.isDirectory()) {
>-          WARN("Ignoring file: " + stageDirEntry.path);
>-          continue;
>+          if (id.substring(id.length - 4, id.length).toLowerCase() == ".xpi") {

You don't need the second argument to substring here.

>+            id = id.substring(0, id.length - 4);
>+            directLoad = true;
>+          }
>+          else {
>+            if (id.substring(id.length - 5, id.length).toLowerCase() != ".json")

Ditto

>     };
>     this.addAddonsToCrashReporter();
> 
>     let principal = Cc["@mozilla.org/systemprincipal;1"].
>                     createInstance(Ci.nsIPrincipal);
>     this.bootstrapScopes[aId] = new Components.utils.Sandbox(principal);
> 
>     let bootstrap = aDir.clone();
>-    bootstrap.append("bootstrap.js");
>+    let name = aDir.leafName;
>+    let spec;
>+
>+    if (bootstrap.isDirectory()) {
>+      bootstrap.append("bootstrap.js");
>+      let uri = Services.io.newFileURI(bootstrap);
>+      spec = uri.spec;
>+    } else if (name.substring(name.length - 4, name.length).toLowerCase() ==

Don't know why you're bothering to test the file extension since you're assuming it will always be a directory or an xpi file.

>+               ".xpi") {
>+      let uri = Services.io.newFileURI(bootstrap);
>+      spec = "jar:" + uri.spec + "!/bootstrap.js";

Please use buildJarURI

>-      stagedAddon.append(this.addon.id);
>-      if (stagedAddon.exists())
>-        stagedAddon.remove(true);
>-      stagedAddon.create(Ci.nsIFile.DIRECTORY_TYPE, FileUtils.PERMS_DIRECTORY);
>-      extractFiles(this.file, stagedAddon);
>+      if (this.addon.unpack || Prefs.getBoolPref(PREF_XPI_UNPACK, false)) {
>+        LOG(this.sourceURI.spec + " will be unpacked");

Change this to "Add-on <id> will be installed as an unpacked directory" and add a matching log to the other side.

>+      if (!entry.isDirectory() &&
>+          id.substring(id.length - 4, id.length).toLowerCase() == ".xpi") {
>+        id = id.substring(0, id.length - 4);
>+        directLoad = true;
>+      }

You can move this down below the ID test since any ID with .xpi tagged on the end will also be a valid ID. You can then simplify the if conditions and get rid of the directLoad variable.

>       if (!gIDTest.test(id)) {
>         LOG("Ignoring file entry whose name is not a valid add-on ID: " +
>              entry.path);
>         continue;
>       }
> 
>-      // XXX Bug 530188 requires us to clone this entry
>-      entry = this._directory.clone().QueryInterface(Ci.nsILocalFile);
>-      entry.append(id);
>-      if (entry.isFile()) {
>+      if (entry.isFile() && !directLoad) {
>         newEntry = this._readDirectoryFromFile(entry);
>         if (!newEntry)
>           continue;
>         entry = newEntry;
>       }
>-      else if (!entry.isDirectory()) {
>-        LOG("Ignoring entry which isn't a directory: " + entry.path);
>-        continue;
>-      }

Please keep this

>   installAddon: function DirInstallLocation_installAddon(aId, aSource) {
>     let dir = this._directory.clone().QueryInterface(Ci.nsILocalFile);
>     dir.append(aId);
>     if (dir.exists())
>       dir.remove(true);
> 
>-    aSource = aSource.clone();
>-    aSource.moveTo(this._directory, aId);
>-    this._DirToIDMap[dir.path] = aId;
>-    this._IDToDirMap[aId] = dir;
>-
>-    return dir;
>+    dir = this._directory.clone().QueryInterface(Ci.nsILocalFile);
>+    dir.append(aId + ".xpi");
>+    if (dir.exists()) {
>+      Services.obs.notifyObservers(dir, "flush-cache-entry", null);
>+      dir.remove(true);
>+    }
>+
>+    aSource = aSource.clone().QueryInterface(Ci.nsILocalFile);
>+    Services.obs.notifyObservers(aSource, "flush-cache-entry", null);

It's only worth doing this if it is an XPI file.

>diff --git a/toolkit/mozapps/extensions/test/xpcshell/head_addons.js b/toolkit/mozapps/extensions/test/xpcshell/head_addons.js
>--- a/toolkit/mozapps/extensions/test/xpcshell/head_addons.js
>+++ b/toolkit/mozapps/extensions/test/xpcshell/head_addons.js
>@@ -115,16 +115,31 @@ function do_check_not_in_crash_annotatio
>  *         The name of the testcase (without extension)
>  * @return an nsILocalFile pointing to the testcase xpi
>  */
> function do_get_addon(aName) {
>   return do_get_file("addons/" + aName + ".xpi");
> }
> 
> /**
>+ * Returns uri pointing to the root of the extension

Document the parameters and return

>+ */
>+function do_get_addon_root_uri(aProfileDir, aId) {
>+  let path = aProfileDir.clone();
>+  path.append(aId);
>+  if (!path.exists()) {
>+    path = aProfileDir.clone();
>+    path.append(aId + ".xpi");

Just do path.leafName += ".xpi"

>+    return "jar:" + Services.io.newFileURI(path).spec + "!/";
>+  }
>+  else
>+    return Services.io.newFileURI(path).spec;

Braces around the else statement please.

>diff --git a/toolkit/mozapps/extensions/test/xpcshell/test_AddonRepository_cache.js b/toolkit/mozapps/extensions/test/xpcshell/test_AddonRepository_cache.js

> function get_subfile_uri(aId, aFilename) {
>   let file = gProfD.clone();
>   file.append("extensions");
>-  file.append(aId);
>-  if (aFilename)
>-    file.append(aFilename);
>-  return NetUtil.newURI(file).spec;
>+  let spec = do_get_addon_root_uri(file, aId);
>+  return do_get_addon_root_uri(file, aId) + aFilename;

spec seems to be unused here

>diff --git a/toolkit/mozapps/extensions/test/xpcshell/test_bug541420.js b/toolkit/mozapps/extensions/test/xpcshell/test_bug541420.js
>--- a/toolkit/mozapps/extensions/test/xpcshell/test_bug541420.js
>+++ b/toolkit/mozapps/extensions/test/xpcshell/test_bug541420.js
>@@ -33,16 +33,20 @@
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL
>  *
>  * ***** END LICENSE BLOCK *****
>  */
> 
> function run_test() {
>+  // This test isn't applicable to packed extensions
>+  if (!Services.prefs.getBoolPref("xpinstall.unpack"))
>+    return;

I disagree, leave this test running and mark its extensions as needing to be unpacked.

>diff --git a/toolkit/mozapps/extensions/test/xpcshell/test_getresource.js b/toolkit/mozapps/extensions/test/xpcshell/test_getresource.js
>--- a/toolkit/mozapps/extensions/test/xpcshell/test_getresource.js
>+++ b/toolkit/mozapps/extensions/test/xpcshell/test_getresource.js
>@@ -35,45 +35,37 @@ function run_test() {
> 
>     completeAllInstalls([aInstall], function() {
>       restartManager();
>       AddonManager.getAddonByID("addon1@tests.mozilla.org", function(a1) {
>         do_check_neq(a1, null);
> 
>         let addonDir = gProfD.clone();
>         addonDir.append("extensions");
>-        addonDir.append("addon1@tests.mozilla.org");
>+        let rootUri = do_get_addon_root_uri(addonDir, "addon1@tests.mozilla.org");
> 
>-        let uri = a1.getResourceURI();
>-        do_check_true(uri instanceof AM_Ci.nsIFileURL);
>-        do_check_eq(uri.file.path, addonDir.path);
>+        let uri = a1.getResourceURI("/");
>+        do_check_eq(uri.spec, rootUri);
> 
>-        let file = addonDir.clone();
>-        file.append("install.rdf");
>+        let file = rootUri + "install.rdf";
>         do_check_true(a1.hasResource("install.rdf"));
>         uri = a1.getResourceURI("install.rdf")
>-        do_check_true(uri instanceof AM_Ci.nsIFileURL);
>-        do_check_eq(uri.file.path, file.path);
>+        do_check_eq(uri.spec, file);
> 
>-        file = addonDir.clone();
>-        file.append("icon.png");
>+        file = rootUri + "icon.png";
>         do_check_true(a1.hasResource("icon.png"));
>         uri = a1.getResourceURI("icon.png")
>-        do_check_true(uri instanceof AM_Ci.nsIFileURL);
>-        do_check_eq(uri.file.path, file.path);
>+        do_check_eq(uri.spec, file);
> 
>         do_check_false(a1.hasResource("missing.txt"));
> 
>-        file = addonDir.clone();
>-        file.append("subdir");
>-        file.append("subfile.txt");
>+        file = rootUri + "subdir/subfile.txt";
>         do_check_true(a1.hasResource("subdir/subfile.txt"));
>         uri = a1.getResourceURI("subdir/subfile.txt")
>-        do_check_true(uri instanceof AM_Ci.nsIFileURL);
>-        do_check_eq(uri.file.path, file.path);
>+        do_check_eq(uri.spec, file);
> 
>         do_check_false(a1.hasResource("subdir/missing.txt"));
> 
>         do_check_eq(a1.size, ADDON_SIZE);
> 
>         a1.uninstall();
> 
>         restartManager();
>diff --git a/toolkit/mozapps/extensions/test/xpcshell/test_install.js b/toolkit/mozapps/extensions/test/xpcshell/test_install.js
>--- a/toolkit/mozapps/extensions/test/xpcshell/test_install.js
>+++ b/toolkit/mozapps/extensions/test/xpcshell/test_install.js
>@@ -108,19 +108,41 @@ function run_test_1() {
> function check_test_1() {
>   ensure_test_completed();
>   AddonManager.getAddonByID("addon1@tests.mozilla.org", function(olda1) {
>     do_check_eq(olda1, null);
> 
>     AddonManager.getAddonsWithOperationsByTypes(null, function(pendingAddons) {
>       do_check_eq(pendingAddons.length, 1);
>       do_check_eq(pendingAddons[0].id, "addon1@tests.mozilla.org");
>-      let iconFile = NetUtil.newURI(pendingAddons[0].iconURL)
>-                            .QueryInterface(AM_Ci.nsIFileURL).file;
>-      do_check_true(iconFile.exists());
>+      let jarURI;
>+      try {
>+        jarURI = NetUtil.newURI(pendingAddons[0].iconURL)
>+                        .QueryInterface(AM_Ci.nsIJARURI);
>+      }
>+      catch (e) {
>+      }

Use instanceof AM_Ci.nsIJARURI etc to avoid the tray catch stuff.

	Dave Townsend [:mossop]
	Comment 101 • 14 years ago

(In reply to comment #87)
> (In reply to comment #86)
> > (In reply to comment #84)
> > > Comment on attachment 469666 [details] [diff] [review] [details] [details]
> > > 4. Support non-extracting extensions in extension manager, v3
> > > 
> > > wtb more context
> > > 
> > Will do.
> > 
> > > I'm not sure that storing the xpi files for these in the extensions directory
> > > is a great idea, it makes for a pretty terrible downgrade user experience.
> > > 
> > Actually, it was done for a good downgrade experience. Older versions of
> > firefox install xpis found in the extension directory. There's just two
> > problems: The first is that installation via the extension directory prompts
> > the user. The second is that the extension manager doesn't try to install xpis
> > with an @ in the file name. We should be able to work around the second
> > problem.
> 
> The first problem is pretty ugly as it is but the third problem is I think the
> older version will reject incompatible add-ons meaning you can downgrade then
> upgrade and lose extensions.
> 
> > > How does this code handling finding both .xpi and non-.xpi forms in the staging
> > > directory and in the install location?
> > > 
> > The logic should be: if the entry is a directory, load it as usual. If not,
> > check if it ends with .xpi and install it if it does.
> 
> I don't think that was matching the code I was seeing but I'll take a closer
> look on the next pass.

The code as-is does the following:

If both xpi and unpackaged directory exist in the staging directory then both will get installed into the install location. The ordering of directoryEntries determines which ends up installed. Presumably directoryEntries gives us a fixed ordering (maybe based on filesystem type?) but I think we should add a test to this to ensure it matches across platforms.

The same is basically true of the install location itself. _readAddons doesn't do any checking to see if both types were found so whichever is seen last wins. Another test should be added.

	Dave Townsend [:mossop]
	Comment 102 • 14 years ago

Comment on attachment 472879 [details] [diff] [review]
9. Update comments/variable names to reflect support for files as extensions

Actually only really meant the comments but this is awesome

	Michael Wu [:mwu] Assignee
	Comment 103 • 14 years ago

Only test_registry.js wasn't converted, since I don't think the current patches actually support unpacked extensions referenced by the windows registry.

	Dave Townsend [:mossop]
	Comment 104 • 14 years ago

(In reply to comment #103)
> Created attachment 473367 [details] [diff] [review]
> 10. Update tests to use writeInstallRDFForExtension
> 
> Only test_registry.js wasn't converted, since I don't think the current patches
> actually support unpacked extensions referenced by the windows registry.

I presume you mean we don't support packed extensions referenced in the windows registry. That is potentially a perf hit given the plans in bug 594058

	Michael Wu [:mwu] Assignee
	Comment 105 • 14 years ago

(In reply to comment #104)
> (In reply to comment #103)
> > Created attachment 473367 [details] [diff] [review] [details]
> > 10. Update tests to use writeInstallRDFForExtension
> > 
> > Only test_registry.js wasn't converted, since I don't think the current patches
> > actually support unpacked extensions referenced by the windows registry.
> 
> I presume you mean we don't support packed extensions referenced in the windows
> registry. That is potentially a perf hit given the plans in bug 594058
Yeah that's what I meant. Don't remember why I didn't implement it though.. need to check.

	Michael Wu [:mwu] Assignee
	Comment 106 • 14 years ago

http://hg.mozilla.org/mozilla-central/rev/203419a57194 patch 1, 2, and 6
http://hg.mozilla.org/mozilla-central/rev/4958e6add3c9 patch 5a
http://hg.mozilla.org/mozilla-central/rev/b32655a79234 patch 3
http://hg.mozilla.org/mozilla-central/rev/3e6d29c4ab61 patch 5b
http://hg.mozilla.org/mozilla-central/rev/04f642fa69a2 patch 7
http://hg.mozilla.org/mozilla-central/rev/48cf59ded8dd patch 8

All that remains is the addon manager change that actually takes advantage of these backend changes.

	Jesper Kristensen
	Comment 107 • 14 years ago

Which addons does this affect? I have seen comments that it affects addons with binary components and addons which access their files directly. Are there others? Does it affect dictionary addons? As far as I know the spell check engine expects dictionaries to be actual files, not entries in an xpi archive.

	Michael Wu [:mwu] Assignee
	Comment 108 • 14 years ago

(In reply to comment #107)
> Which addons does this affect? I have seen comments that it affects addons with
> binary components and addons which access their files directly. Are there
> others? Does it affect dictionary addons? As far as I know the spell check
> engine expects dictionaries to be actual files, not entries in an xpi archive.

Yes, dictionaries also need to be unpacked. We want the dictionary loading code to support loading from arbitrary URIs but it's currently not supported. I think this will also affect the loading of window icons.

	Dave Townsend [:mossop]
	Comment 109 • 14 years ago

Comment on attachment 473367 [details] [diff] [review]
10. Update tests to use writeInstallRDFForExtension

Awesome.

>diff --git a/toolkit/mozapps/extensions/test/xpcshell/head_addons.js b/toolkit/mozapps/extensions/test/xpcshell/head_addons.js

>+function do_get_expected_addon_name(aId) {
>+  if (Services.prefs.getBoolPref("xpinstall.unpack"))
>+    return aId;
>+  else
>+    return aId + ".xpi";

The else is unnecessary here.

>+function writeInstallRDFForExtension(aData, aDir, aId, aExtraFile) {

Since this function modifies the aDir passed in it should clone it, which also means you don't need all the cloning in the tests.

>+  var id = aId;
>+  if (!id)
>+    id = aData.id;

Just do id = aId ? aId : aData.id

	Dave Townsend [:mossop]
	Comment 110 • 14 years ago

How does performance vary on whether the extension's XPI is compressed or not?

	(dormant account) Reporter
	Comment 111 • 14 years ago

(In reply to comment #110)
> How does performance vary on whether the extension's XPI is compressed or not?

Faster cold start, slower warm start.

	Michael Wu [:mwu] Assignee
	Comment 112 • 14 years ago

I'll be rolling everything up into a megapatch at one point.. just doing this to give smaller updates to review.

	Michael Wu [:mwu] Assignee
	Comment 114 • 14 years ago

Most review comments addressed here or in one of the follow up patches.

	Michael Wu [:mwu] Assignee
	Comment 115 • 14 years ago

The cloning of the file in writeInstallRDFForExtension wasn't done as some tests like to do things like change the modified date of the extension after installing. Not cloning the file effectively gives back the path that the extension was installed to.

	Dave Townsend [:mossop]
	Comment 117 • 14 years ago

Comment on attachment 473819 [details] [diff] [review]
10. Update tests to use writeInstallRDFForExtension, v2

(In reply to comment #115)
> Created attachment 473819 [details] [diff] [review]
> 10. Update tests to use writeInstallRDFForExtension, v2
> 
> The cloning of the file in writeInstallRDFForExtension wasn't done as some
> tests like to do things like change the modified date of the extension after
> installing. Not cloning the file effectively gives back the path that the
> extension was installed to.

Please still do the cloning and return the resulting file for those cases that want it

	Dave Townsend [:mossop]
	Comment 119 • 14 years ago

Comment on attachment 473846 [details] [diff] [review]
13. Return the extension file instead of modifying the argument

wtb more context

>diff --git a/toolkit/mozapps/extensions/test/xpcshell/test_bug542391.js b/toolkit/mozapps/extensions/test/xpcshell/test_bug542391.js
>--- a/toolkit/mozapps/extensions/test/xpcshell/test_bug542391.js
>+++ b/toolkit/mozapps/extensions/test/xpcshell/test_bug542391.js
>@@ -205,8 +205,8 @@ function run_test() {
> 
>   // Add an extension to the profile to make sure the dialog doesn't show up
>   // on new profiles
>-  var dest = profileDir.clone();
>-  writeInstallRDFForExtension({
>+  var dest;
>+  dest = writeInstallRDFForExtension({

var dest = ...

>diff --git a/toolkit/mozapps/extensions/test/xpcshell/test_startup.js b/toolkit/mozapps/extensions/test/xpcshell/test_startup.js
>--- a/toolkit/mozapps/extensions/test/xpcshell/test_startup.js
>+++ b/toolkit/mozapps/extensions/test/xpcshell/test_startup.js
>@@ -116,19 +116,15 @@ function end_test() {
> 
> // Try to install all the items into the profile
> function run_test_1() {
>-  var dest = profileDir.clone();
>-  writeInstallRDFForExtension(addon1, dest);
>-  dest = profileDir.clone();
>-  writeInstallRDFForExtension(addon2, dest);
>+  var dest;
>+  dest = writeInstallRDFForExtension(addon1, profileDir);
>+  dest = writeInstallRDFForExtension(addon2, profileDir);
>   // Attempt to make this look like it was added some time in the past so
>   // the change in run_test_2 makes the last modified time change.
>   dest.lastModifiedTime -= 5000;
>-  dest = profileDir.clone();
>-  writeInstallRDFForExtension(addon3, dest);
>-  dest = profileDir.clone();
>-  writeInstallRDFForExtension(addon4, dest);
>-  dest = profileDir.clone();
>-  writeInstallRDFForExtension(addon5, dest);
>+  dest = writeInstallRDFForExtension(addon3, profileDir);
>+  dest = writeInstallRDFForExtension(addon4, profileDir);
>+  dest = writeInstallRDFForExtension(addon5, profileDir);

Only one of these actually needs dest it seems?

> // Test that modified items are detected and items in other install locations
> // are ignored
> function run_test_2() {
>-  var dest = userDir.clone();
>+  var dest;
>   addon1.version = "1.1";
>-  writeInstallRDFForExtension(addon1, dest);
>-  dest = profileDir.clone();
>+  dest = writeInstallRDFForExtension(addon1, userDir);
>   addon2.version="2.1";
>-  writeInstallRDFForExtension(addon2, dest);
>-  dest = globalDir.clone();
>+  dest = writeInstallRDFForExtension(addon2, profileDir);
>   addon2.version="2.2";
>-  writeInstallRDFForExtension(addon2, dest);
>-  dest = userDir.clone();
>+  dest = writeInstallRDFForExtension(addon2, globalDir);
>   addon2.version="2.3";
>-  writeInstallRDFForExtension(addon2, dest);
>+  dest = writeInstallRDFForExtension(addon2, userDir);

And again, in fact just this whole file assigns to dest when it is mostly unnecessary

>diff --git a/toolkit/mozapps/extensions/test/xpcshell/test_uninstall.js b/toolkit/mozapps/extensions/test/xpcshell/test_uninstall.js

>     var dest = profileDir.clone();
>     dest.append(do_get_expected_addon_name("addon1@tests.mozilla.org"));
>     do_check_false(dest.exists());
>-    dest = profileDir.clone();
>-    writeInstallRDFForExtension(addon1, dest);
>+    dest = writeInstallRDFForExtension(addon1, profileDir);

dest goes unused here.

>diff --git a/toolkit/mozapps/extensions/test/xpcshell/test_upgrade.js b/toolkit/mozapps/extensions/test/xpcshell/test_upgrade.js
>--- a/toolkit/mozapps/extensions/test/xpcshell/test_upgrade.js
>+++ b/toolkit/mozapps/extensions/test/xpcshell/test_upgrade.js
>@@ -22,8 +22,8 @@ function run_test() {
>   createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "1", "1.9.2");
> 
>   // Will be enabled in the first version and disabled in subsequent versions
>-  var dest = profileDir.clone();
>-  writeInstallRDFForExtension({
>+  var dest;
>+  dest = writeInstallRDFForExtension({

var dest = ...

But is that really necessary? Doesn't look like it here or for most of this file.

	Michael Wu [:mwu] Assignee
	Comment 120 • 14 years ago

I left in dest = when the nearby code used it to keep things looking consistent. I can trim all of it.

	Dave Townsend [:mossop]
	Comment 121 • 14 years ago

Comment on attachment 473817 [details] [diff] [review]
4. Support non-extracting extensions in extension manager, v6

Can I get an interdiff between this and the previous version with 8 lines of context please.

	Michael Wu [:mwu] Assignee
	Comment 123 • 14 years ago

Use of the return value trimmed to only the callers that use it.

	Dave Townsend [:mossop]
	Comment 125 • 14 years ago

Comment on attachment 473817 [details] [diff] [review]
4. Support non-extracting extensions in extension manager, v6

>diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm
>-        // Only directories are important. Files may be updated manifests.
>+        let id = stageDirEntry.leafName;
>         if (!stageDirEntry.isDirectory()) {
>-          WARN("Ignoring file: " + stageDirEntry.path);
>-          continue;
>+          if (id.substring(id.length - 4).toLowerCase() == ".xpi")
>+            id = id.substring(0, id.length - 4);
>+          else {
>+            if (id.substring(id.length - 5).toLowerCase() != ".json")
>+              WARN("Ignoring file: " + stageDirEntry.path);
>+            continue;
>+          }

Braces around the if part here please.

>-        LOG("Ignoring entry which isn't a directory: " + entry.path);
>-        continue;
>+        if (id.substring(id.length - 4).toLowerCase() == ".xpi")
>+          id = id.substring(0, id.length - 4);
>+        else {
>+          newEntry = this._readDirectoryFromFile(entry);
>+          if (!newEntry)
>+            continue;
>+          entry = newEntry;
>+        }

Braces around the if part here please.

>@@ -5950,12 +5984,21 @@ DirectoryInstallLocation.prototype = {
>     if (dir.exists())
>       dir.remove(true);
> 
>-    aSource = aSource.clone();
>-    aSource.moveTo(this._directory, aId);
>-    this._DirToIDMap[dir.path] = aId;
>-    this._IDToDirMap[aId] = dir;
>-
>-    return dir;
>+    dir = this._directory.clone().QueryInterface(Ci.nsILocalFile);
>+    dir.append(aId + ".xpi");
>+    if (dir.exists()) {
>+      Services.obs.notifyObservers(dir, "flush-cache-entry", null);
>+      dir.remove(true);
>+    }
>+
>+    aSource = aSource.clone().QueryInterface(Ci.nsILocalFile);
>+    Services.obs.notifyObservers(aSource, "flush-cache-entry", null);

As I said previously, it's only worth doing this if it is an XPI file.

>diff --git a/toolkit/mozapps/extensions/test/xpcshell/head_addons.js b/toolkit/mozapps/extensions/test/xpcshell/head_addons.js
>--- a/toolkit/mozapps/extensions/test/xpcshell/head_addons.js
>+++ b/toolkit/mozapps/extensions/test/xpcshell/head_addons.js
>@@ -120,6 +120,26 @@ function do_get_addon(aName) {
> }
> 
> /**
>+ * Returns an extension uri spec
>+ *
>+ * @param  aProfileDir
>+ *         The extension install directory
>+ * @return a uri spec pointing to the root of the extension
>+ */
>+function do_get_addon_root_uri(aProfileDir, aId) {
>+  let path = aProfileDir.clone();
>+  path.append(aId);
>+  if (!path.exists()) {
>+    path = aProfileDir.clone();
>+    path.append(aId + ".xpi");

As I said earlier, just do path.leafName += ".xpi".

	Dave Townsend [:mossop]
	Comment 126 • 14 years ago

Comment on attachment 473869 [details] [diff] [review]
13. Return the extension file instead of modifying the argument, v2

>diff --git a/toolkit/mozapps/extensions/test/xpcshell/test_bug542391.js b/toolkit/mozapps/extensions/test/xpcshell/test_bug542391.js
>--- a/toolkit/mozapps/extensions/test/xpcshell/test_bug542391.js
>+++ b/toolkit/mozapps/extensions/test/xpcshell/test_bug542391.js
>@@ -200,27 +200,27 @@ function check_state_v3_2([a1, a2, a3, a
> function run_test() {
>   do_test_pending();
>   createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "1", "1");
> 
>   Services.prefs.setBoolPref(PREF_EM_SHOW_MISMATCH_UI, true);
> 
>   // Add an extension to the profile to make sure the dialog doesn't show up
>   // on new profiles
>-  var dest = profileDir.clone();
>-  writeInstallRDFForExtension({
>+  var dest;
>+  dest = writeInstallRDFForExtension({

Combine these to one line.

	Dave Townsend [:mossop]
	Comment 127 • 14 years ago

Comment on attachment 473806 [details] [diff] [review]
12. Add unpack to the two tests that need it

>diff --git a/toolkit/mozapps/extensions/test/addons/test_bug541420/binary b/toolkit/mozapps/extensions/test/addons/test_bug541420/binary
>new file mode 100755
>diff --git a/toolkit/mozapps/extensions/test/addons/test_bug541420/install.rdf b/toolkit/mozapps/extensions/test/addons/test_bug541420/install.rdf
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/mozapps/extensions/test/addons/test_bug541420/install.rdf
>@@ -0,0 +1,23 @@
>+<?xml version="1.0"?>
>+
>+<RDF xmlns="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
>+     xmlns:em="http://www.mozilla.org/2004/em-rdf#">
>+
>+  <Description about="urn:mozilla:install-manifest">
>+    <em:id>bug541420@tests.mozilla.org">bug541420@tests.mozilla.org</em:id>
>+    <em:version>1.0</em:version>
>+    <em:unpack>true</em:unpack>
>+
>+    <em:targetApplication>
>+      <Description>
>+        <em:id>xpcshell@tests.mozilla.org</em:id>
>+        <em:minVersion>1</em:minVersion>
>+        <em:maxVersion>1</em:maxVersion>
>+      </Description>
>+    </em:targetApplication>
>+
>+    <!-- Front End MetaData -->
>+    <em:name>Bug 541420</em:name>
>+
>+  </Description>
>+</RDF>

I'd rather that this remained as an xpi in the tree so we don't have to rely on the zip tool marking the file as executable properly.

	Dave Townsend [:mossop]
	Comment 128 • 14 years ago

Comment on attachment 473822 [details] [diff] [review]
4 + 9 + 10 + 11 + 12: megapatch

Is there anything in this patch not in the others?

	Michael Wu [:mwu] Assignee
	Comment 129 • 14 years ago

(In reply to comment #128)
> Comment on attachment 473822 [details] [diff] [review]
> 4 + 9 + 10 + 11 + 12: megapatch
> 
> Is there anything in this patch not in the others?

There shouldn't be. I just qfolded everything to make this.

	Michael Wu [:mwu] Assignee
	Comment 130 • 14 years ago

(In reply to comment #125)
> >@@ -5950,12 +5984,21 @@ DirectoryInstallLocation.prototype = {
> >     if (dir.exists())
> >       dir.remove(true);
> > 
> >-    aSource = aSource.clone();
> >-    aSource.moveTo(this._directory, aId);
> >-    this._DirToIDMap[dir.path] = aId;
> >-    this._IDToDirMap[aId] = dir;
> >-
> >-    return dir;
> >+    dir = this._directory.clone().QueryInterface(Ci.nsILocalFile);
> >+    dir.append(aId + ".xpi");
> >+    if (dir.exists()) {
> >+      Services.obs.notifyObservers(dir, "flush-cache-entry", null);
> >+      dir.remove(true);
> >+    }
> >+
> >+    aSource = aSource.clone().QueryInterface(Ci.nsILocalFile);
> >+    Services.obs.notifyObservers(aSource, "flush-cache-entry", null);
> 
> As I said previously, it's only worth doing this if it is an XPI file.
> 
I'm not sure that's a worthwhile optimization and there are other places that this comment could apply to. Would you want it in the uninstall path too?

	Dave Townsend [:mossop]
	Comment 131 • 14 years ago

Comment on attachment 473822 [details] [diff] [review]
4 + 9 + 10 + 11 + 12: megapatch

There is no need for me to review this then

	Dave Townsend [:mossop]
	Comment 132 • 14 years ago

(In reply to comment #130)
> (In reply to comment #125)
> > >@@ -5950,12 +5984,21 @@ DirectoryInstallLocation.prototype = {
> > >     if (dir.exists())
> > >       dir.remove(true);
> > > 
> > >-    aSource = aSource.clone();
> > >-    aSource.moveTo(this._directory, aId);
> > >-    this._DirToIDMap[dir.path] = aId;
> > >-    this._IDToDirMap[aId] = dir;
> > >-
> > >-    return dir;
> > >+    dir = this._directory.clone().QueryInterface(Ci.nsILocalFile);
> > >+    dir.append(aId + ".xpi");
> > >+    if (dir.exists()) {
> > >+      Services.obs.notifyObservers(dir, "flush-cache-entry", null);
> > >+      dir.remove(true);
> > >+    }
> > >+
> > >+    aSource = aSource.clone().QueryInterface(Ci.nsILocalFile);
> > >+    Services.obs.notifyObservers(aSource, "flush-cache-entry", null);
> > 
> > As I said previously, it's only worth doing this if it is an XPI file.
> > 
> I'm not sure that's a worthwhile optimization and there are other places that
> this comment could apply to. Would you want it in the uninstall path too?

In terms of performance sure, the difference is probably marginal but I'd rather the code was clear that it is only actually doing anything in the XPI case. Probably worth a comment in both cases too. I really wish we hadn't used "flush-cache-entry" for this topic since it is really too generic and confusing given the number of caches we have that apply to add-ons.

	Michael Wu [:mwu] Assignee
	Comment 134 • 14 years ago

(In reply to comment #101)
> The code as-is does the following:
> 
> If both xpi and unpackaged directory exist in the staging directory then both
> will get installed into the install location. The ordering of directoryEntries
> determines which ends up installed. Presumably directoryEntries gives us a
> fixed ordering (maybe based on filesystem type?) but I think we should add a
> test to this to ensure it matches across platforms.
> 
How about uninstalling any previously staged extensions? That would ensure the last staged extension gets installed instead of whatever filesystem/alphabetic order the directory enumerator gives us.

> The same is basically true of the install location itself. _readAddons doesn't
> do any checking to see if both types were found so whichever is seen last wins.
> Another test should be added.
Do we pick the xpi or the directory if that happens? Do we attempt to uninstall the other one?

	Dave Townsend [:mossop]
	Comment 135 • 14 years ago

(In reply to comment #134)
> (In reply to comment #101)
> > The code as-is does the following:
> > 
> > If both xpi and unpackaged directory exist in the staging directory then both
> > will get installed into the install location. The ordering of directoryEntries
> > determines which ends up installed. Presumably directoryEntries gives us a
> > fixed ordering (maybe based on filesystem type?) but I think we should add a
> > test to this to ensure it matches across platforms.
> > 
> How about uninstalling any previously staged extensions? That would ensure the
> last staged extension gets installed instead of whatever filesystem/alphabetic
> order the directory enumerator gives us.

This doesn't make sense to me. My point is if during a single startup both xpi and directories for an extension are detected in the staging directory then it is undefined which actually gets used at this point (ignoring directory ordering)

> > The same is basically true of the install location itself. _readAddons doesn't
> > do any checking to see if both types were found so whichever is seen last wins.
> > Another test should be added.
> Do we pick the xpi or the directory if that happens? Do we attempt to uninstall
> the other one?

I guess the xpi as that seems to be our preferred form right now (and what would get installed on downgrade). I'd probably just leave hte other there, it'll get removed then next time the add-on is upgraded.

	Dave Townsend [:mossop]
	Comment 136 • 14 years ago

(In reply to comment #135)
> (In reply to comment #134)
> > (In reply to comment #101)
> > > The code as-is does the following:
> > > 
> > > If both xpi and unpackaged directory exist in the staging directory then both
> > > will get installed into the install location. The ordering of directoryEntries
> > > determines which ends up installed. Presumably directoryEntries gives us a
> > > fixed ordering (maybe based on filesystem type?) but I think we should add a
> > > test to this to ensure it matches across platforms.
> > > 
> > How about uninstalling any previously staged extensions? That would ensure the
> > last staged extension gets installed instead of whatever filesystem/alphabetic
> > order the directory enumerator gives us.
> 
> This doesn't make sense to me. My point is if during a single startup both xpi
> and directories for an extension are detected in the staging directory then it
> is undefined which actually gets used at this point (ignoring directory
> ordering)
> 
> > > The same is basically true of the install location itself. _readAddons doesn't
> > > do any checking to see if both types were found so whichever is seen last wins.
> > > Another test should be added.
> > Do we pick the xpi or the directory if that happens? Do we attempt to uninstall
> > the other one?
> 
> I guess the xpi as that seems to be our preferred form right now (and what
> would get installed on downgrade). I'd probably just leave hte other there,
> it'll get removed then next time the add-on is upgraded.

This can probably land separately to the rest to make sure we get good bake time.

	Michael Wu [:mwu] Assignee
	Comment 137 • 14 years ago

I suspect the default extensions.alwaysUnpack pref isn't being loaded correctly right now in xpcshell, so this patch makes it explicit.

	Dave Townsend [:mossop]
	Comment 138 • 14 years ago

Comment on attachment 474129 [details] [diff] [review]
15. Be explicit about what we're testing

There shouldn't be any problems with loading the default prefs and besides any use of that preference falls back to a default false value anyway so I don't see why this is needed

	Michael Wu [:mwu] Assignee
	Comment 139 • 14 years ago

(In reply to comment #138)
> Comment on attachment 474129 [details] [diff] [review]
> 15. Be explicit about what we're testing
> 
> There shouldn't be any problems with loading the default prefs and besides any
> use of that preference falls back to a default false value anyway so I don't
> see why this is needed

I was seeing extensions being installed as directories in the xpcshell directory during some testing with check-one. Well, it seems pretty unlikely to me too so I'll drop this..

	Michael Wu [:mwu] Assignee
	Comment 141 • 14 years ago

Hoping for the best. Will file followups to address ambiguous behavior when an unpacked and packed extension are both installed.

http://hg.mozilla.org/mozilla-central/rev/dd0de36fc6f4

	Michael Wu [:mwu] Assignee
	Comment 142 • 14 years ago

Someone just asked about __LOCATION__. Though the change was not in this bug, it will be worth documenting that __LOCATION__ will not be defined in components loaded from jars. There is some documentation on devmo that refer to __LOCATION__.

	Justin Dolske [:Dolske]
	Comment 143 • 14 years ago

I think this may have broken incremental builds:

$ make -C toolkit
...
cp -pPR /Users/dolske-bulk/build/mozilla-central/obj/toolkit/mozapps/extensions/test/../../../../_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell/. /Users/dolske-bulk/build/mozilla-central/obj/toolkit/mozapps/extensions/test/../../../../_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack
cp: cannot overwrite directory /Users/dolske-bulk/build/mozilla-central/obj/toolkit/mozapps/extensions/test/../../../../_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/./data with non-directory /Users/dolske-bulk/build/mozilla-central/obj/toolkit/mozapps/extensions/test/../../../../_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell/./data

"data" is a symlink in both places, tried a quick workaround by adding -f to the cp command but didn't help. 

Workaround (for each incremental make):

$ rm _tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/data

	Dave Townsend [:mossop]
	Comment 144 • 14 years ago

(In reply to comment #143)
> I think this may have broken incremental builds:
> 
> $ make -C toolkit
> ...
> cp -pPR
> /Users/dolske-bulk/build/mozilla-central/obj/toolkit/mozapps/extensions/test/../../../../_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell/.
> /Users/dolske-bulk/build/mozilla-central/obj/toolkit/mozapps/extensions/test/../../../../_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack
> cp: cannot overwrite directory
> /Users/dolske-bulk/build/mozilla-central/obj/toolkit/mozapps/extensions/test/../../../../_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/./data
> with non-directory
> /Users/dolske-bulk/build/mozilla-central/obj/toolkit/mozapps/extensions/test/../../../../_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell/./data
> 
> "data" is a symlink in both places, tried a quick workaround by adding -f to
> the cp command but didn't help. 
> 
> Workaround (for each incremental make):
> 
> $ rm _tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/data

Filed bug 596668 on this.

	Dave Townsend [:mossop]
	Comment 145 • 14 years ago

(In reply to comment #136)
> (In reply to comment #135)
> > (In reply to comment #134)
> > > (In reply to comment #101)
> > > > The code as-is does the following:
> > > > 
> > > > If both xpi and unpackaged directory exist in the staging directory then both
> > > > will get installed into the install location. The ordering of directoryEntries
> > > > determines which ends up installed. Presumably directoryEntries gives us a
> > > > fixed ordering (maybe based on filesystem type?) but I think we should add a
> > > > test to this to ensure it matches across platforms.
> > > > 
> > > How about uninstalling any previously staged extensions? That would ensure the
> > > last staged extension gets installed instead of whatever filesystem/alphabetic
> > > order the directory enumerator gives us.
> > 
> > This doesn't make sense to me. My point is if during a single startup both xpi
> > and directories for an extension are detected in the staging directory then it
> > is undefined which actually gets used at this point (ignoring directory
> > ordering)
> > 
> > > > The same is basically true of the install location itself. _readAddons doesn't
> > > > do any checking to see if both types were found so whichever is seen last wins.
> > > > Another test should be added.
> > > Do we pick the xpi or the directory if that happens? Do we attempt to uninstall
> > > the other one?
> > 
> > I guess the xpi as that seems to be our preferred form right now (and what
> > would get installed on downgrade). I'd probably just leave hte other there,
> > it'll get removed then next time the add-on is upgraded.
> 
> This can probably land separately to the rest to make sure we get good bake
> time.

Did a bug get filed for this?

(In reply to comment #88)
> 2. Add a <em:unpack>true</em:unpack> test.

I don't think this ever happened, it needs to.

	Michael Wu [:mwu] Assignee
	Comment 146 • 14 years ago

(In reply to comment #145)
> (In reply to comment #136)
> > (In reply to comment #135)
> > > (In reply to comment #134)
> > > > (In reply to comment #101)
> > > > > The code as-is does the following:
> > > > > 
> > > > > If both xpi and unpackaged directory exist in the staging directory then both
> > > > > will get installed into the install location. The ordering of directoryEntries
> > > > > determines which ends up installed. Presumably directoryEntries gives us a
> > > > > fixed ordering (maybe based on filesystem type?) but I think we should add a
> > > > > test to this to ensure it matches across platforms.
> > > > > 
> > > > How about uninstalling any previously staged extensions? That would ensure the
> > > > last staged extension gets installed instead of whatever filesystem/alphabetic
> > > > order the directory enumerator gives us.
> > > 
> > > This doesn't make sense to me. My point is if during a single startup both xpi
> > > and directories for an extension are detected in the staging directory then it
> > > is undefined which actually gets used at this point (ignoring directory
> > > ordering)
> > > 
> > > > > The same is basically true of the install location itself. _readAddons doesn't
> > > > > do any checking to see if both types were found so whichever is seen last wins.
> > > > > Another test should be added.
> > > > Do we pick the xpi or the directory if that happens? Do we attempt to uninstall
> > > > the other one?
> > > 
> > > I guess the xpi as that seems to be our preferred form right now (and what
> > > would get installed on downgrade). I'd probably just leave hte other there,
> > > it'll get removed then next time the add-on is upgraded.
> > 
> > This can probably land separately to the rest to make sure we get good bake
> > time.
> 
> Did a bug get filed for this?
> 
bug 595392

> (In reply to comment #88)
> > 2. Add a <em:unpack>true</em:unpack> test.
> 
> I don't think this ever happened, it needs to.
We have two tests which have <em:unpack>. Those two tests explicitly check for files in the extension directory IIRC, so we should be covered.

	Ben Bucksch (:BenB)
	Comment 147 • 14 years ago

Because this cut me badly, even though I was aware of this bug, but not the effects in my case, I added some docs to <https://developer.mozilla.org/en/Firefox_4_for_developers#XPI_unpacking>. Please correct/extend with file types that I missed. This needs a blog post on <http://blog.mozilla.com/addons/>, too, so that ext devs are aware of it. It will break many extensions (including all with binary XPCOM components, any with searchplugins/, any using getInstallLocation() etc.)

	Jesper Kristensen
	Comment 148 • 14 years ago

I think this changes needs a lot of outreach. I think many addon developers test thoroughly when releasing a new version, but the testing may not be as thorough when updating the compatibility information. I typically don't reinstall my extension the same way a user would before updating compatibility information. If you just test an existing install of your extension, it will work fine. Only new installs and updates are affected. I just checked the 96 dictionaries listed on addons.mozilla.org. 18 of them declares compatibility with the latest beta, but only 4 are actually compatible.

	Dave Townsend [:mossop]
	Comment 149 • 14 years ago

(In reply to comment #147)
> Because this cut me badly, even though I was aware of this bug, but not the
> effects in my case, I added some docs to
> <https://developer.mozilla.org/en/Firefox_4_for_developers#XPI_unpacking>.
> Please correct/extend with file types that I missed. This needs a blog post on
> <http://blog.mozilla.com/addons/>, too, so that ext devs are aware of it. It
> will break many extensions (including all with binary XPCOM components, any
> with searchplugins/, any using getInstallLocation() etc.)

Thanks for this, I've made a few minor updates to the notes you added, added some docs on em:unpack to the install manifests page and pushed out a blog post to http://blog.mozilla.com/addons/2010/09/23/changes-to-how-extensions-are-installed-in-firefox-4/

	Eric Shepherd [:sheppy]
	Comment 150 • 14 years ago

I've moved the content you guys put together to:

https://developer.mozilla.org/en/Extensions/Updating_extensions_for_Firefox_4#XPI_unpacking

And done some minor copy-editing. That's linked to from the Fx4 for developers page.

In addition, I added notes to:

https://developer.mozilla.org/en/XPI
https://developer.mozilla.org/en/Extension_Packaging

	Eric Shepherd [:sheppy]
	Comment 151 • 14 years ago

The reason for the Updating extensions page is to call out items that will break compatibility with existing extensions, requiring updates. The XPI unpacking item is one of those. The Fx4 for developers page is intended to be a list of articles, not to include long explanations. I have reverted your reversion. :)

	Ben Bucksch (:BenB)
	Comment 152 • 14 years ago

sheppy, I don't think this discussion belongs here, but on the talk page. I gave the my reasons there.

	Ben Bucksch (:BenB)
	Comment 153 • 14 years ago

(FWIW, this will not just break existing extensions, but new ones as well.)

	Sergey Sokoloff
	Comment 154 • 14 years ago

And, while you're at it, note that the <em:unpack> hyperlink at https://developer.mozilla.org/en/Extensions/Updating_extensions_for_Firefox_4#XPI_unpacking points to the following URL:

mks://localhost/en/Install_Manifests#unpack

Fix it, please.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 155 • 14 years ago

Verified fixed by installing extensions and themes with builds on all platforms like Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b7pre) Gecko/20101006 Firefox/4.0b7pre. Add-ons which were already installed are getting replaced by the XPI package. One issue I have found with older builds of Firefox has been filed as bug 603611 and should be tracked separately.

Michael, what's the coverage of the automated tests? Do we need any manual Litmus tests to verify this new feature?

	Michael Wu [:mwu] Assignee
	Comment 156 • 14 years ago

(In reply to comment #155)
> Michael, what's the coverage of the automated tests? Do we need any manual
> Litmus tests to verify this new feature?

Coverage is relatively good. All the xpcshell tests first run without xpi extraction, and then run again with xpi extraction. <em:unpack> should be tested well. I think that might leave room for a test to ensure that xpis without <em:unpack> aren't installed unpacked.. not sure if we have any xpcshell test for that scenario.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 157 • 14 years ago

(In reply to comment #156)
> Coverage is relatively good. All the xpcshell tests first run without xpi
> extraction, and then run again with xpi extraction. <em:unpack> should be
> tested well. I think that might leave room for a test to ensure that xpis
> without <em:unpack> aren't installed unpacked.. not sure if we have any
> xpcshell test for that scenario.

Can we get a bug on file for such a test implemented? I would rather like to see this automated as having to give complicated steps for users to test this manually.

	Dave Townsend [:mossop]
	Comment 158 • 14 years ago

(In reply to comment #156)
> (In reply to comment #155)
> > Michael, what's the coverage of the automated tests? Do we need any manual
> > Litmus tests to verify this new feature?
> 
> Coverage is relatively good. All the xpcshell tests first run without xpi
> extraction, and then run again with xpi extraction. <em:unpack> should be
> tested well. I think that might leave room for a test to ensure that xpis
> without <em:unpack> aren't installed unpacked.. not sure if we have any
> xpcshell test for that scenario.

Don't the existing tests already do this? Most of the test add-ons don't have em:unpack and we modify files in them

	Michael Wu [:mwu] Assignee
	Comment 159 • 14 years ago

(In reply to comment #158)
> Don't the existing tests already do this? Most of the test add-ons don't have
> em:unpack and we modify files in them

I was thinking of a test that would catch extensions.alwaysUnpack getting turned on or some scenario like that, since the tests try to work in either scenario.

	Pardal Freudenthal (:ShareBird)
	Comment 160 • 14 years ago

Something I've noticed right now: if you place a "some_extension_install_package".xpi inside the extensions directory it doesn't install the extension anymore (it used to do). It will only install the extension if you rename the xpi to some_extension.id.xpi (some_extension.id being the id from that extension).
Is this an intended behavior?

	Dave Townsend [:mossop]
	Comment 161 • 14 years ago

(In reply to comment #160)
> Something I've noticed right now: if you place a
> "some_extension_install_package".xpi inside the extensions directory it doesn't
> install the extension anymore (it used to do). It will only install the
> extension if you rename the xpi to some_extension.id.xpi (some_extension.id
> being the id from that extension).
> Is this an intended behavior?

Yes

	Pardal Freudenthal (:ShareBird)
	Comment 162 • 14 years ago

(In reply to comment #161)
> (In reply to comment #160)
> > Something I've noticed right now: if you place a
> > "some_extension_install_package".xpi inside the extensions directory it doesn't
> > install the extension anymore (it used to do). It will only install the
> > extension if you rename the xpi to some_extension.id.xpi (some_extension.id
> > being the id from that extension).
> > Is this an intended behavior?
> 
> Yes

Oh... This is a pity... Many people use to install previous downloaded extensions in a bunch simply putting them inside the extensions directory. 
Sorry if I'm missing some previous discussion about this, but is there a reason to do this, or is not possible to have it with the new approach?

	Dave Townsend [:mossop]
	Comment 163 • 14 years ago

(In reply to comment #162)
> (In reply to comment #161)
> > (In reply to comment #160)
> > > Something I've noticed right now: if you place a
> > > "some_extension_install_package".xpi inside the extensions directory it doesn't
> > > install the extension anymore (it used to do). It will only install the
> > > extension if you rename the xpi to some_extension.id.xpi (some_extension.id
> > > being the id from that extension).
> > > Is this an intended behavior?
> > 
> > Yes
> 
> Oh... This is a pity... Many people use to install previous downloaded
> extensions in a bunch simply putting them inside the extensions directory. 
> Sorry if I'm missing some previous discussion about this, but is there a reason
> to do this, or is not possible to have it with the new approach?

See bug 566373

	Rick Alther
	Comment 164 • 14 years ago

(In reply to comment #154)
> And, while you're at it, note that the <em:unpack> hyperlink at
> https://developer.mozilla.org/en/Extensions/Updating_extensions_for_Firefox_4#XPI_unpacking
> points to the following URL:
> 
> mks://localhost/en/Install_Manifests#unpack
> 
> Fix it, please.

Bump.  This hyperlink is still not corrected.

	Wladimir Palant
	Comment 165 • 14 years ago

From all I can tell, it's DekiWiki playing up - the link is correct but isn't converted. I "fixed" it by changing it into a regular HTTP link.

	Ed Morley [:emorley]
	Comment 166 • 14 years ago

For reference, this change broke a number of dictionaries, including British English ( https://addons.mozilla.org/en-US/firefox/addon/3366/ ) and German ( https://addons.mozilla.org/en-US/firefox/addon/3077/ ), amongst others.

Is their an addons equivalent of website evangelism that I can inform about the broken dictionaries, so they can chase up the dictionary creators and at least correct those that mistakenly have 4.08pre compatibility listed, when they don't work? (I've already reported the addons some time ago using addons compatibility reporter, but in the case of the British English dictionary it was last updated in 2007, so I'm not holding my breath...)

	Robert Kaiser
	Comment 167 • 14 years ago

Compatibility with 4.0b8pre is not entirely wrong, as already installed dictionary add-ons work fine - but newly installed ones will fail.
For the German dictionaries, that's bug 611420.