Last Comment Bug 533038 - (packedxpi) Extensions should not be extracted into the profile directory, but installed/stored as XPI file
(packedxpi)
: Extensions should not be extracted into the profile directory, but installed/...
Status: VERIFIED FIXED
[ts]
: dev-doc-complete
Product: Toolkit
Classification: Components
Component: Add-ons Manager (show other bugs)
: Trunk
: All All
: -- normal with 8 votes (vote)
: mozilla2.0b7
Assigned To: Michael Wu [:mwu]
:
Mentors:
Depends on: 598697 550011 595462 595573 596607 597509 597620 599921 603611 610156 610180 612088 612393 616628 617055 620357 645970 712789
Blocks: 447581 459117 550242 595392 595398 608593 531886 593058 594058 597702
  Show dependency treegraph
 
Reported: 2009-12-04 18:24 PST by (dormant account)
Modified: 2016-07-23 13:54 PDT (History)
59 users (show)
hskupin: in‑testsuite+
hskupin: in‑litmus?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta7+


Attachments
1. Generalize manifest reading code to read from any jar (19.31 KB, patch)
2010-08-19 23:06 PDT, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
2. Add api for reading manifests from jars (8.08 KB, patch)
2010-08-19 23:11 PDT, Michael Wu [:mwu]
benjamin: review-
Details | Diff | Splinter Review
3. Support reading preferences from arbitrary jars (3.45 KB, patch)
2010-08-19 23:15 PDT, Michael Wu [:mwu]
dwitte: review+
Details | Diff | Splinter Review
4. Support non-extracting extensions in extension manager (10.67 KB, patch)
2010-08-19 23:24 PDT, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
Adblock Plus with noUnpack support (487.57 KB, application/x-xpinstall)
2010-08-19 23:33 PDT, Michael Wu [:mwu]
no flags Details
1. Generalize manifest reading code to read from any jar, v2 (1.17 KB, patch)
2010-08-20 15:27 PDT, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
1. Generalize manifest reading code to read from any jar, v3 (21.88 KB, patch)
2010-08-23 14:12 PDT, Michael Wu [:mwu]
benjamin: review-
Details | Diff | Splinter Review
5. Allow reading jars within jars (17.74 KB, patch)
2010-08-23 18:57 PDT, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
4. Support non-extracting extensions in extension manager, v2 (12.96 KB, patch)
2010-08-24 11:57 PDT, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
5. Allow reading jars within jars, v2 (29.14 KB, patch)
2010-08-24 13:48 PDT, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
5. Allow reading jars within jars, v3 (28.10 KB, patch)
2010-08-24 13:58 PDT, Michael Wu [:mwu]
taras.mozilla: review-
Details | Diff | Splinter Review
5a. nsZipArchive changes for reading jars within jars (8.67 KB, patch)
2010-08-24 17:39 PDT, Michael Wu [:mwu]
taras.mozilla: review+
Details | Diff | Splinter Review
6. Fixes for patches 1 and 2 (15.22 KB, patch)
2010-08-25 14:35 PDT, Michael Wu [:mwu]
benjamin: review+
Details | Diff | Splinter Review
5a. nsZipArchive changes for reading jars within jars, v2 (8.52 KB, patch)
2010-08-26 11:22 PDT, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
5b. Allow reading jars within jars, v4 (10.47 KB, patch)
2010-08-26 12:15 PDT, Michael Wu [:mwu]
taras.mozilla: review+
Details | Diff | Splinter Review
3a. Interdiff with review fixes (4.68 KB, patch)
2010-08-26 13:50 PDT, Michael Wu [:mwu]
dwitte: review+
Details | Diff | Splinter Review
3. Support reading preferences from arbitrary jars, v2 (3.45 KB, patch)
2010-08-26 15:32 PDT, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
3. Support reading preferences from arbitrary jars, v3 (4.97 KB, patch)
2010-08-26 16:13 PDT, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
4. Support non-extracting extensions in extension manager, v3 (9.04 KB, patch)
2010-08-26 16:14 PDT, Michael Wu [:mwu]
dtownsend: review-
Details | Diff | Splinter Review
4. Support non-extracting extensions in extension manager, v4 WIP (22.39 KB, patch)
2010-09-03 18:18 PDT, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
7. Add support for loading jar uris in the js subscript loader (1.17 KB, patch)
2010-09-07 12:04 PDT, Michael Wu [:mwu]
benjamin: review-
Details | Diff | Splinter Review
8. Add flush-cache-entry topic (1.26 KB, patch)
2010-09-07 12:10 PDT, Michael Wu [:mwu]
taras.mozilla: review+
Details | Diff | Splinter Review
7. Add support for loading jar uris in the js subscript loader, v2 (763 bytes, patch)
2010-09-07 13:34 PDT, Michael Wu [:mwu]
benjamin: review+
Details | Diff | Splinter Review
4. Support non-extracting extensions in extension manager, v5 (32.92 KB, patch)
2010-09-07 16:12 PDT, Michael Wu [:mwu]
dtownsend: review-
Details | Diff | Splinter Review
9. Update comments/variable names to reflect support for files as extensions (30.71 KB, patch)
2010-09-07 18:29 PDT, Michael Wu [:mwu]
dtownsend: review+
Details | Diff | Splinter Review
10. Update tests to use writeInstallRDFForExtension (40.53 KB, patch)
2010-09-08 19:58 PDT, Michael Wu [:mwu]
dtownsend: review-
Details | Diff | Splinter Review
11. Support unpacked xpi extensions referenced in the windows registry (1.83 KB, patch)
2010-09-09 15:33 PDT, Michael Wu [:mwu]
dtownsend: review+
Details | Diff | Splinter Review
12. Add unpack to the two tests that need it (4.34 KB, patch)
2010-09-09 16:30 PDT, Michael Wu [:mwu]
dtownsend: review-
Details | Diff | Splinter Review
4. Support non-extracting extensions in extension manager, v6 (23.18 KB, patch)
2010-09-09 16:52 PDT, Michael Wu [:mwu]
dtownsend: review-
Details | Diff | Splinter Review
10. Update tests to use writeInstallRDFForExtension, v2 (43.47 KB, patch)
2010-09-09 16:54 PDT, Michael Wu [:mwu]
dtownsend: review-
Details | Diff | Splinter Review
4 + 9 + 10 + 11 + 12: megapatch (89.50 KB, patch)
2010-09-09 16:58 PDT, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
13. Return the extension file instead of modifying the argument (36.34 KB, patch)
2010-09-09 17:55 PDT, Michael Wu [:mwu]
dtownsend: review-
Details | Diff | Splinter Review
Interdiff between version 5 and 6 of patch 4 (12.37 KB, patch)
2010-09-09 18:43 PDT, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
13. Return the extension file instead of modifying the argument, v2 (53.18 KB, patch)
2010-09-09 18:48 PDT, Michael Wu [:mwu]
dtownsend: review+
Details | Diff | Splinter Review
Interdiff between version 1 and 2 of patch 13 (10.41 KB, patch)
2010-09-09 18:50 PDT, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
14. More fixes according to review comments (5.76 KB, patch)
2010-09-09 21:01 PDT, Michael Wu [:mwu]
dtownsend: review+
Details | Diff | Splinter Review
15. Be explicit about what we're testing (724 bytes, patch)
2010-09-10 11:10 PDT, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
4 + 9 + 10 + 11 + 12 + 13 + 14: megapatch (96.59 KB, patch)
2010-09-10 11:33 PDT, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review

Description (dormant account) 2009-12-04 18:24:48 PST
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.
Comment 1 Robert Strong [:rstrong] (use needinfo to contact me) 2009-12-04 18:28:48 PST
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?
Comment 2 Robert Strong [:rstrong] (use needinfo to contact me) 2009-12-04 18:36:31 PST
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.
Comment 3 (dormant account) 2009-12-04 18:36:48 PST
(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 :(
Comment 4 Robert Strong [:rstrong] (use needinfo to contact me) 2009-12-04 18:46:32 PST
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.
Comment 5 (dormant account) 2009-12-08 17:37:18 PST
I asked Ben to get some numbers on this
Comment 6 Dave Townsend [:mossop] 2010-03-02 16:00:27 PST
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?
Comment 7 (dormant account) 2010-03-02 16:22:58 PST
(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.
Comment 8 Dave Townsend [:mossop] 2010-03-02 16:31:02 PST
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.
Comment 9 Dave Townsend [:mossop] 2010-03-02 16:38:16 PST
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)
Comment 10 (dormant account) 2010-03-02 16:46:00 PST
(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.
Comment 11 Ted Mielczarek [:ted.mielczarek] 2010-03-02 16:48:53 PST
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
Comment 12 (dormant account) 2010-03-02 16:51:04 PST
(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.
Comment 13 Benjamin Smedberg [:bsmedberg] 2010-03-02 16:59:09 PST
Well yeah, but unless you extract the extension when you install it there aren't any files to poke at.
Comment 14 Dave Townsend [:mossop] 2010-03-02 18:22:14 PST
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.
Comment 15 Dietrich Ayala (:dietrich) 2010-03-02 23:52:25 PST
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.
Comment 16 Ted Mielczarek [:ted.mielczarek] 2010-03-03 04:41:46 PST
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.
Comment 17 Dave Townsend [:mossop] 2010-03-03 08:09:42 PST
(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?
Comment 18 Dietrich Ayala (:dietrich) 2010-03-03 09:28:27 PST
(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
Comment 19 Dietrich Ayala (:dietrich) 2010-03-03 12:11:04 PST
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
Comment 20 Dave Townsend [:mossop] 2010-03-03 12:22:57 PST
(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.
Comment 21 Benjamin Smedberg [:bsmedberg] 2010-03-03 12:40:20 PST
We *should* be cacheing XPT files in xpti.dat and only reloading them when we re-register!
Comment 22 (dormant account) 2010-03-04 09:04:08 PST
(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.
Comment 23 Dave Townsend [:mossop] 2010-03-04 09:33:27 PST
(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.
Comment 24 (dormant account) 2010-03-04 09:59:52 PST
(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.
Comment 25 Dave Townsend [:mossop] 2010-03-04 10:03:11 PST
(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.
Comment 26 (dormant account) 2010-03-04 10:20:29 PST
(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.
Comment 27 John J. Barton 2010-03-05 22:14:01 PST
(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.
Comment 28 (dormant account) 2010-03-05 23:05:45 PST
(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.
Comment 29 John J. Barton 2010-03-06 08:46:15 PST
(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.
Comment 30 Alfred Kayser 2010-03-10 23:54:57 PST
What about letting AMO check that extensions and themes have their chrome contents packed in a jar within the extension xpi?
Comment 31 Manoj 2010-03-12 12:50:16 PST
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?
Comment 32 Dave Townsend [:mossop] 2010-03-12 13:01:25 PST
(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.
Comment 33 Dietrich Ayala (:dietrich) 2010-03-12 13:32:56 PST
(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?
Comment 34 Dietrich Ayala (:dietrich) 2010-03-12 13:37:48 PST
(In reply to comment #33)
> What cannot currently be loaded from a jar?

Nm, comments 7 and 8 cover this.
Comment 35 Dietrich Ayala (:dietrich) 2010-03-16 18:45:28 PDT
(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.
Comment 36 Steve Fink [:sfink] [:s:] 2010-06-10 16:27:25 PDT
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.
Comment 37 Ben Bucksch (:BenB) 2010-07-26 14:02:46 PDT
> 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.
Comment 38 Michael Wu [:mwu] 2010-08-19 23:06:33 PDT
Created attachment 467684 [details] [diff] [review]
1. Generalize manifest reading code to read from any jar

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(-)
Comment 39 Michael Wu [:mwu] 2010-08-19 23:11:39 PDT
Created attachment 467686 [details] [diff] [review]
2. Add api for reading manifests from jars

Some care was taken to make sure this api works in non-omnijar non-libxul builds.
Comment 40 Michael Wu [:mwu] 2010-08-19 23:15:13 PDT
Created attachment 467688 [details] [diff] [review]
3. Support reading preferences from arbitrary jars

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..
Comment 41 Michael Wu [:mwu] 2010-08-19 23:24:26 PDT
Created attachment 467690 [details] [diff] [review]
4. Support non-extracting extensions in extension manager

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.
Comment 42 Michael Wu [:mwu] 2010-08-19 23:33:11 PDT
Created attachment 467691 [details]
Adblock Plus with noUnpack support

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.
Comment 43 Michael Wu [:mwu] 2010-08-20 15:27:05 PDT
Created attachment 467925 [details] [diff] [review]
1. Generalize manifest reading code to read from any jar, v2

Fix the key used for mKnownJARModules.
Comment 44 Michael Wu [:mwu] 2010-08-23 14:12:02 PDT
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.
Comment 45 Michael Wu [:mwu] 2010-08-23 14:19:54 PDT
(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)
Comment 46 Michael Wu [:mwu] 2010-08-23 18:57:34 PDT
Created attachment 468568 [details] [diff] [review]
5. Allow reading jars within jars

This is a pretty terrible hack but it seems to work..
Comment 47 Michael Wu [:mwu] 2010-08-24 11:57:51 PDT
Created attachment 468751 [details] [diff] [review]
4. Support non-extracting extensions in extension manager, v2

This makes the addon manager not extract extensions by default. From my limited testing, adblock plus, javascript debugger, and noscript work fine.
Comment 48 (dormant account) 2010-08-24 13:48:22 PDT
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.
Comment 49 Michael Wu [:mwu] 2010-08-24 13:48:49 PDT
Created attachment 468798 [details] [diff] [review]
5. Allow reading jars within jars, v2

yo dawg i herd you like jars
Comment 50 Michael Wu [:mwu] 2010-08-24 13:58:24 PDT
Created attachment 468803 [details] [diff] [review]
5. Allow reading jars within jars, v3

bitrot fixed
Comment 51 (dormant account) 2010-08-24 14:03:56 PDT
(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.
Comment 52 (dormant account) 2010-08-24 15:22:54 PDT
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.
Comment 53 Michael Wu [:mwu] 2010-08-24 16:21:52 PDT
This bug will be necessary to fix bug 531886 (by stat'ing everything again).
Comment 54 Michael Wu [:mwu] 2010-08-24 17:39:20 PDT
Created attachment 468877 [details] [diff] [review]
5a. nsZipArchive changes for reading jars within jars
Comment 55 Benjamin Smedberg [:bsmedberg] 2010-08-25 06:00:44 PDT
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?
Comment 56 Benjamin Smedberg [:bsmedberg] 2010-08-25 07:04:13 PDT
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).
Comment 57 Benjamin Smedberg [:bsmedberg] 2010-08-25 07:08:08 PDT
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!
Comment 58 Benjamin Smedberg [:bsmedberg] 2010-08-25 07:11:44 PDT
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.
Comment 59 dwitte@gmail.com 2010-08-25 09:32:20 PDT
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.)
Comment 60 Ben Bucksch (:BenB) 2010-08-25 09:39:24 PDT
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.
Comment 61 Ben Bucksch (:BenB) 2010-08-25 09:40:47 PDT
> ReadExtensionPrefs() would be used from extension manager, not the extension,
> right?

FWIW, that is missing from the documentation comment. Add "typically", if you must.
Comment 62 dwitte@gmail.com 2010-08-25 09:50:16 PDT
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.
Comment 63 Benjamin Smedberg [:bsmedberg] 2010-08-25 09:53:09 PDT
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.
Comment 64 dwitte@gmail.com 2010-08-25 09:54:15 PDT
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.
Comment 65 Ben Bucksch (:BenB) 2010-08-25 09:55:25 PDT
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.
Comment 66 Ben Bucksch (:BenB) 2010-08-25 09:56:25 PDT
Thanks. (My last comment was in reply to comment 62.)
Comment 67 (dormant account) 2010-08-25 09:58:51 PDT
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.
Comment 68 dwitte@gmail.com 2010-08-25 10:08:01 PDT
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.
Comment 69 dwitte@gmail.com 2010-08-25 10:17:48 PDT
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.
Comment 70 Michael Wu [:mwu] 2010-08-25 11:25:51 PDT
(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
Comment 71 Michael Wu [:mwu] 2010-08-25 11:28:50 PDT
(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.
Comment 72 Michael Wu [:mwu] 2010-08-25 14:35:49 PDT
Created attachment 469194 [details] [diff] [review]
6. Fixes for patches 1 and 2

- 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.
Comment 73 Alfred Kayser 2010-08-26 00:57:23 PDT
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.
Comment 74 Michael Wu [:mwu] 2010-08-26 01:59:19 PDT
(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.
Comment 75 Michael Wu [:mwu] 2010-08-26 11:22:37 PDT
Created attachment 469536 [details] [diff] [review]
5a. nsZipArchive changes for reading jars within jars, v2

Review comments addressed.
Comment 76 Michael Wu [:mwu] 2010-08-26 12:15:15 PDT
Created attachment 469556 [details] [diff] [review]
5b. Allow reading jars within jars, v4

mInnerZips eliminated.
Comment 77 Michael Wu [:mwu] 2010-08-26 13:50:13 PDT
Created attachment 469597 [details] [diff] [review]
3a. Interdiff with review fixes

How does this look?
Comment 78 dwitte@gmail.com 2010-08-26 14:11:19 PDT
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
Comment 79 (dormant account) 2010-08-26 14:31:12 PDT
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?
Comment 80 Michael Wu [:mwu] 2010-08-26 15:32:41 PDT
Created attachment 469642 [details] [diff] [review]
3. Support reading preferences from arbitrary jars, v2
Comment 81 Michael Wu [:mwu] 2010-08-26 16:13:32 PDT
Created attachment 469665 [details] [diff] [review]
3. Support reading preferences from arbitrary jars, v3
Comment 82 Michael Wu [:mwu] 2010-08-26 16:14:09 PDT
Created attachment 469666 [details] [diff] [review]
4. Support non-extracting extensions in extension manager, v3
Comment 83 Dave Townsend [:mossop] 2010-08-30 10:21:18 PDT
Really not convinced that we should take this this late but apparently it is needed.
Comment 84 Dave Townsend [:mossop] 2010-08-31 16:44:41 PDT
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.
Comment 85 Jorge Villalobos [:jorgev] 2010-08-31 16:54:25 PDT
(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.
Comment 86 Michael Wu [:mwu] 2010-08-31 17:53:32 PDT
(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.
Comment 87 Dave Townsend [:mossop] 2010-08-31 18:12:44 PDT
(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.
Comment 88 Michael Wu [:mwu] 2010-09-03 18:18:17 PDT
Created attachment 472065 [details] [diff] [review]
4. Support non-extracting extensions in extension manager, v4 WIP

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.
Comment 89 Pardal Freudenthal (:ShareBird) 2010-09-04 21:13:36 PDT
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.
Comment 90 Michael Wu [:mwu] 2010-09-04 21:21:18 PDT
(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.
Comment 91 Pardal Freudenthal (:ShareBird) 2010-09-04 21:55:47 PDT
Thank you Michael for clarifying this.
Comment 92 Michael Wu [:mwu] 2010-09-07 12:04:53 PDT
Created attachment 472711 [details] [diff] [review]
7. Add support for loading jar uris in the js subscript loader

Necessary for no-unpack no-restart extensions.
Comment 93 Benjamin Smedberg [:bsmedberg] 2010-09-07 12:09:22 PDT
Comment on attachment 472711 [details] [diff] [review]
7. Add support for loading jar uris in the js subscript loader

Use NS_GetInnermostURI.
Comment 94 Michael Wu [:mwu] 2010-09-07 12:10:41 PDT
Created attachment 472717 [details] [diff] [review]
8. Add flush-cache-entry topic

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.
Comment 95 Michael Wu [:mwu] 2010-09-07 13:34:48 PDT
Created attachment 472742 [details] [diff] [review]
7. Add support for loading jar uris in the js subscript loader, v2

Patch greatly simplified by NS_GetInnermostURI.
Comment 96 (dormant account) 2010-09-07 13:45:48 PDT
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;
> }
>
Comment 97 Michael Wu [:mwu] 2010-09-07 16:12:04 PDT
Created attachment 472819 [details] [diff] [review]
4. Support non-extracting extensions in extension manager, v5

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>.
Comment 98 Michael Wu [:mwu] 2010-09-07 18:29:49 PDT
Created attachment 472879 [details] [diff] [review]
9. Update comments/variable names to reflect support for files as extensions
Comment 99 Dave Townsend [:mossop] 2010-09-08 14:52:41 PDT
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.
Comment 100 Dave Townsend [:mossop] 2010-09-08 16:38:29 PDT
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.
Comment 101 Dave Townsend [:mossop] 2010-09-08 16:57:15 PDT
(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.
Comment 102 Dave Townsend [:mossop] 2010-09-08 17:04:28 PDT
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
Comment 103 Michael Wu [:mwu] 2010-09-08 19:58:28 PDT
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.
Comment 104 Dave Townsend [:mossop] 2010-09-08 20:01:56 PDT
(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
Comment 105 Michael Wu [:mwu] 2010-09-08 20:03:49 PDT
(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.
Comment 107 Jesper Kristensen 2010-09-09 01:03:09 PDT
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.
Comment 108 Michael Wu [:mwu] 2010-09-09 01:11:30 PDT
(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.
Comment 109 Dave Townsend [:mossop] 2010-09-09 15:19:22 PDT
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
Comment 110 Dave Townsend [:mossop] 2010-09-09 15:23:36 PDT
How does performance vary on whether the extension's XPI is compressed or not?
Comment 111 (dormant account) 2010-09-09 15:24:36 PDT
(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.
Comment 112 Michael Wu [:mwu] 2010-09-09 15:33:18 PDT
Created attachment 473775 [details] [diff] [review]
11. Support unpacked xpi extensions referenced in the windows registry

I'll be rolling everything up into a megapatch at one point.. just doing this to give smaller updates to review.
Comment 113 Michael Wu [:mwu] 2010-09-09 16:30:26 PDT
Created attachment 473806 [details] [diff] [review]
12. Add unpack to the two tests that need it
Comment 114 Michael Wu [:mwu] 2010-09-09 16:52:51 PDT
Created attachment 473817 [details] [diff] [review]
4. Support non-extracting extensions in extension manager, v6

Most review comments addressed here or in one of the follow up patches.
Comment 115 Michael Wu [:mwu] 2010-09-09 16:54:56 PDT
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.
Comment 116 Michael Wu [:mwu] 2010-09-09 16:58:24 PDT
Created attachment 473822 [details] [diff] [review]
4 + 9 + 10 + 11 + 12: megapatch
Comment 117 Dave Townsend [:mossop] 2010-09-09 16:59:46 PDT
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
Comment 118 Michael Wu [:mwu] 2010-09-09 17:55:42 PDT
Created attachment 473846 [details] [diff] [review]
13. Return the extension file instead of modifying the argument
Comment 119 Dave Townsend [:mossop] 2010-09-09 18:15:13 PDT
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.
Comment 120 Michael Wu [:mwu] 2010-09-09 18:18:48 PDT
I left in dest = when the nearby code used it to keep things looking consistent. I can trim all of it.
Comment 121 Dave Townsend [:mossop] 2010-09-09 18:19:06 PDT
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.
Comment 122 Michael Wu [:mwu] 2010-09-09 18:43:17 PDT
Created attachment 473868 [details] [diff] [review]
Interdiff between version 5 and 6 of patch 4
Comment 123 Michael Wu [:mwu] 2010-09-09 18:48:54 PDT
Created attachment 473869 [details] [diff] [review]
13. Return the extension file instead of modifying the argument, v2

Use of the return value trimmed to only the callers that use it.
Comment 124 Michael Wu [:mwu] 2010-09-09 18:50:03 PDT
Created attachment 473870 [details] [diff] [review]
Interdiff between version 1 and 2 of patch 13
Comment 125 Dave Townsend [:mossop] 2010-09-09 19:32:11 PDT
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".
Comment 126 Dave Townsend [:mossop] 2010-09-09 19:36:10 PDT
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.
Comment 127 Dave Townsend [:mossop] 2010-09-09 19:42:36 PDT
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.
Comment 128 Dave Townsend [:mossop] 2010-09-09 19:43:22 PDT
Comment on attachment 473822 [details] [diff] [review]
4 + 9 + 10 + 11 + 12: megapatch

Is there anything in this patch not in the others?
Comment 129 Michael Wu [:mwu] 2010-09-09 20:23:04 PDT
(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.
Comment 130 Michael Wu [:mwu] 2010-09-09 20:28:15 PDT
(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?
Comment 131 Dave Townsend [:mossop] 2010-09-09 20:30:48 PDT
Comment on attachment 473822 [details] [diff] [review]
4 + 9 + 10 + 11 + 12: megapatch

There is no need for me to review this then
Comment 132 Dave Townsend [:mossop] 2010-09-09 20:41:28 PDT
(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.
Comment 133 Michael Wu [:mwu] 2010-09-09 21:01:32 PDT
Created attachment 473917 [details] [diff] [review]
14. More fixes according to review comments
Comment 134 Michael Wu [:mwu] 2010-09-09 21:06:12 PDT
(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?
Comment 135 Dave Townsend [:mossop] 2010-09-10 10:20:01 PDT
(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.
Comment 136 Dave Townsend [:mossop] 2010-09-10 10:22:38 PDT
(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.
Comment 137 Michael Wu [:mwu] 2010-09-10 11:10:59 PDT
Created attachment 474129 [details] [diff] [review]
15. Be explicit about what we're testing

I suspect the default extensions.alwaysUnpack pref isn't being loaded correctly right now in xpcshell, so this patch makes it explicit.
Comment 138 Dave Townsend [:mossop] 2010-09-10 11:27:00 PDT
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
Comment 139 Michael Wu [:mwu] 2010-09-10 11:32:11 PDT
(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..
Comment 140 Michael Wu [:mwu] 2010-09-10 11:33:23 PDT
Created attachment 474137 [details] [diff] [review]
4 + 9 + 10 + 11 + 12 + 13 + 14: megapatch
Comment 141 Michael Wu [:mwu] 2010-09-10 16:04:04 PDT
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
Comment 142 Michael Wu [:mwu] 2010-09-10 23:00:59 PDT
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__.
Comment 143 Justin Dolske [:Dolske] 2010-09-12 22:29:17 PDT
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
Comment 144 Dave Townsend [:mossop] 2010-09-15 11:09:07 PDT
(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.
Comment 145 Dave Townsend [:mossop] 2010-09-17 12:27:07 PDT
(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.
Comment 146 Michael Wu [:mwu] 2010-09-17 12:33:22 PDT
(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.
Comment 147 Ben Bucksch (:BenB) 2010-09-23 02:58:57 PDT
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.)
Comment 148 Jesper Kristensen 2010-09-23 09:53:01 PDT
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.
Comment 149 Dave Townsend [:mossop] 2010-09-23 09:57:03 PDT
(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/
Comment 150 Eric Shepherd [:sheppy] 2010-09-27 08:54:44 PDT
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
Comment 151 Eric Shepherd [:sheppy] 2010-09-27 09:14:38 PDT
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. :)
Comment 152 Ben Bucksch (:BenB) 2010-09-27 09:59:01 PDT
sheppy, I don't think this discussion belongs here, but on the talk page. I gave the my reasons there.
Comment 153 Ben Bucksch (:BenB) 2010-09-27 10:02:07 PDT
(FWIW, this will not just break existing extensions, but new ones as well.)
Comment 154 Sergey «Mithgol the Webmaster» Sokoloff 2010-09-27 18:09:29 PDT
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.
Comment 155 Henrik Skupin (:whimboo) 2010-10-12 07:05:59 PDT
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?
Comment 156 Michael Wu [:mwu] 2010-10-12 08:01:02 PDT
(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.
Comment 157 Henrik Skupin (:whimboo) 2010-10-12 10:27:56 PDT
(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.
Comment 158 Dave Townsend [:mossop] 2010-10-12 10:35:06 PDT
(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
Comment 159 Michael Wu [:mwu] 2010-10-12 10:50:21 PDT
(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.
Comment 160 Pardal Freudenthal (:ShareBird) 2010-10-12 12:21:17 PDT
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?
Comment 161 Dave Townsend [:mossop] 2010-10-12 13:02:43 PDT
(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
Comment 162 Pardal Freudenthal (:ShareBird) 2010-10-12 16:28:13 PDT
(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?
Comment 163 Dave Townsend [:mossop] 2010-10-12 16:30:20 PDT
(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
Comment 164 Rick Alther 2010-10-21 21:58:08 PDT
(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.
Comment 165 Wladimir Palant 2010-10-21 23:16:41 PDT
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.
Comment 166 Ed Morley [:emorley] 2010-11-13 10:46:36 PST
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...)
Comment 167 Robert Kaiser 2010-11-13 12:52:58 PST
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.

Note You need to log in before you can comment on or make changes to this bug.