579178 - Don't enumerate components/*.manifest and chrome/*.manifest

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Benjamin Smedberg]

We currently enumerate components/*.manifest and chrome/*.manifest. Enumerating directories is a known source of bad I/O during startup, and also means that manifests may be loaded in any order. Instead, we should just read the root chrome.manifest and load sub-manifests as necessary.

This patch ends up with five manifests total in a packaged build:

chrome.manifest
-> components/interfaces.manifest
-> components/components.manifest
-> chrome/nonlocalized.manifest
-> chrome/localized.manifest

Unpackaged builds keep lots of manifests around.

Oh, and I'm going to need some removed-files for trunk builds... I forgot that, but I'll catch it later.

Comment 1

[Benjamin Smedberg]

Attachment: Part A - XPCOM changes to load submodules and stop enumerating directories, rev. 1

There are a few tests I may need to clean up as well. We'll see.

Comment 2

[Benjamin Smedberg]

Attachment: Part B - packaging and repackaging changes, rev. 1

Comment 3

[Benjamin Smedberg]

Comment on attachment 457679 [details] [diff] [review]
Part B - packaging and repackaging changes, rev. 1

Tests need work, they drop stuff in the appdir.

Comment 4

[Benjamin Smedberg]

Attachment: Part B - packaging and repackaging changes, rev. 2

Changes:

* package mochitest chrome in distribution/bundles instead of dropping a manifest into chrome/
* ship tp-cmdline in release builds

Comment 5

[Benjamin Smedberg]

Attachment: Part C - Allow xpcshell to register additional manifests, rev. 1

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 457919 [details] [diff] [review]
Part B - packaging and repackaging changes, rev. 2

Looks good except that I don't see any reason to move AB_CD.manifest to localized.manifest.  It looks like we can keep it as AB_CD.manifest the whole way through.  If there's no compelling reason not to keep it that way, lets do that.

Comment 7

[Justin Wood (:Callek)]

Comment on attachment 457678 [details] [diff] [review]
Part A - XPCOM changes to load submodules and stop enumerating directories, rev. 1

>diff --git a/xpcom/components/nsIManifestLoader.idl b/xpcom/components/nsIManifestLoader.idl
>deleted file mode 100644

>diff --git a/modules/libjar/nsManifestZIPLoader.h b/modules/libjar/nsManifestZIPLoader.h
> #include "nsIManifestLoader.h"

nsIManifestLoader.idl was removed, and this file has no references left to anything that was in this .h, but it will fail in a clobber, please remove.

>diff --git a/modules/libjar/objs.mk b/modules/libjar/objs.mk
> MODULES_LIBJAR_LEXPORTS = \
> 		zipstruct.h \
> 		nsZipArchive.h \
>-		nsManifestZIPLoader.h \
> 		$(NULL)

>diff --git a/xpcom/components/Makefile.in b/xpcom/components/Makefile.in
>+	-I$(topsrcdir)/modules/libjar \

>diff --git a/xpcom/components/nsComponentManager.h b/xpcom/components/nsComponentManager.h
>--- a/xpcom/components/nsComponentManager.h
>+++ b/xpcom/components/nsComponentManager.h
>-#include "nsIManifestLoader.h"
>+#include "nsManifestZIPLoader.h"

Even by adding libjar to the include path, don't we want to keep LEXPORTS including this if its used outside of libjar? (You did however remove the other use of it in the tree).

From my skim of everything else this is a great cleanup!

Comment 8

[Justin Wood (:Callek)]

Comment on attachment 457919 [details] [diff] [review]
Part B - packaging and repackaging changes, rev. 2

>diff --git a/config/tests/unit-JarMaker.py b/config/tests/unit-JarMaker.py
>-    debug = False # set to True to debug failing tests on disk
>+    debug = True # set to True to debug failing tests on disk

Please remove this hunk for checkin.

>diff --git a/config/rules.mk b/config/rules.mk
>+EXTRA_MANIFESTS = $(filter %.manifest,$(EXTRA_COMPONENTS) $(EXTRA_PP_COMPONENTS))
>+ifneq (,$(EXTRA_MANIFESTS))
>+libs::
>+	$(PYTHON) $(MOZILLA_DIR)/config/buildlist.py $(FINAL_TARGET)/chrome.manifest $(patsubst %,"manifest components/%",$(notdir $(EXTRA_MANIFESTS)))
> endif

Why chrome.manifest here, doesn't it make more sense to use components.manifest instead? [and make sure components.manifest is linked in chrome.manifest as well]

>diff --git a/browser/installer/package-manifest.in b/browser/installer/package-manifest.in
The hunk here looks unrelated to this bug, I always like changes to have their own bug so reasoning/followup Q's etc. can be found easily; That said, I see no reason a separate changeset/bug is needed for that [since it is already here] :-)

Comment 9

[Mark Banner (:standard8)]

(In reply to comment #4)
> Created attachment 457919 [details] [diff] [review]
> Part B - packaging and repackaging changes, rev. 2
> 
...
> * ship tp-cmdline in release builds

Why is this patch planning on shipping what appears to be test-only code in release builds? Surely we should be correcting the test harness to work properly with the new code...

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

I tested these patches on Fennec, and they do fix the component registration ordering issues.

I do agree with comments about using "chrome.manifest" as the name, since chrome/chrome.manifest exists too. Using "components.manifest" sound good to me.

Comment 12

[Benjamin Smedberg]

> Even by adding libjar to the include path, don't we want to keep LEXPORTS
> including this if its used outside of libjar? (You did however remove the other
> use of it in the tree).

No, I don't.

> >diff --git a/config/rules.mk b/config/rules.mk
> >+EXTRA_MANIFESTS = $(filter %.manifest,$(EXTRA_COMPONENTS) $(EXTRA_PP_COMPONENTS))
> >+ifneq (,$(EXTRA_MANIFESTS))
> >+libs::
> >+	$(PYTHON) $(MOZILLA_DIR)/config/buildlist.py $(FINAL_TARGET)/chrome.manifest $(patsubst %,"manifest components/%",$(notdir $(EXTRA_MANIFESTS)))
> > endif
> 
> Why chrome.manifest here, doesn't it make more sense to use components.manifest
> instead? [and make sure components.manifest is linked in chrome.manifest as
> well]

No, why would I create an extra file if I don't need to? There will just be /chrome.manifest and whatever it loads.

> >diff --git a/browser/installer/package-manifest.in b/browser/installer/package-manifest.in
> The hunk here looks unrelated to this bug, I always like changes to have their
> own bug so reasoning/followup Q's etc. can be found easily; That said, I see no
> reason a separate changeset/bug is needed for that [since it is already here]
> :-)

It isn't unrelated at all: because you can no longer drop in manifests or components into the Firefox install directory, Talos will not work unless we either ship the tp-commandline file by default or somehow have special code to allow it to be dropped in. I opted to ship the -tp commandline code by default.

Comment 13

[Dave Townsend [:mossop]]

Comment on attachment 457678 [details] [diff] [review]
Part A - XPCOM changes to load submodules and stop enumerating directories, rev. 1

>diff --git a/modules/libjar/nsManifestZIPLoader.cpp b/modules/libjar/nsManifestZIPLoader.cpp

>-nsManifestZIPLoader::nsManifestZIPLoader() {
>+nsManifestZIPLoader::nsManifestZIPLoader() 
>+    : mZipReader(new nsJAR())
>+{
>+    nsresult rv = reader->Open(mozilla::OmnijarPath());

What is "reader"? Do you mean mZipReader? Is this path tested?

I also wonder if there is any gain in using the nsIZipReaderCache for stuff talking to the omnijar?

>diff --git a/xpcom/components/ManifestParser.cpp b/xpcom/components/ManifestParser.cpp
>--- a/xpcom/components/ManifestParser.cpp
>+++ b/xpcom/components/ManifestParser.cpp
>@@ -83,16 +83,18 @@ struct ManifestDirective
>   void (nsChromeRegistry::*regfunc)
>     (nsChromeRegistry::ManifestProcessingContext& cx,
>      int lineno, char *const *argv,
>      bool platform, bool contentaccessible);
> 
>   bool isContract;
> };
> static const ManifestDirective kParsingTable[] = {
>+  { "manifest", 1, false, true, false,

Can you line this up like the others.

>     if (!ok ||
>         stApp == eBad ||
>         stAppVersion == eBad ||
>         stOs == eBad ||
>         stOsVersion == eBad ||
>         stABI == eBad)
>       continue;
> 
>-    if (directive->ischrome) {
>+    if (directive->regfunc) {
> #ifdef MOZ_IPC
>       if (GeckoProcessType_Default != XRE_GetProcessType())
>         continue;
> #endif
> 

I wonder if it would be better to just put something higher up in the loop, before all the flag checks to the effect of:

if (!directive->ischrome && aChromeOnly)
  continue;

Then just need an if/else here.

Comment 14

[Benjamin Smedberg]

Attachment: When checking for sorted-matching, keep version A as the unified version, not the sorted version

Comment 15

[(not currently active) Ted Mielczarek]

The mochitest-chrome stuff sounds like it overlaps some work Joel was doing.

Comment 16

[Joel Maher ( :jmaher ) (UTC -8)]

Thanks ted, it doesn't look like much overlap.  Actually when I do get around the landing my mochikit.jar stuff, it will be installed in the profile instead of the application directory (a side effect of android)

Comment 17

[(not currently active) Ted Mielczarek]

Comment on attachment 459065 [details] [diff] [review]
When checking for sorted-matching, keep version A as the unified version, not the sorted version

>diff --git a/build/Makefile.in b/build/Makefile.in
>--- a/build/Makefile.in
>+++ b/build/Makefile.in
>@@ -181,17 +181,17 @@ check::
>         else \
>           echo "TEST-PASS | build/ | unify unified a Java class file!"; \
>         fi
> # try unifying some files that differ only in line ordering
> 	rm -rf unify-sort-test
> 	mkdir unify-sort-test unify-sort-test/a unify-sort-test/b
> 	printf "lmn\nabc\nxyz\n" > unify-sort-test/a/file.foo
> 	printf "xyz\nlmn\nabc" > unify-sort-test/b/file.foo
>-	printf "abc\nlmn\nxyz\n" > unify-sort-test/expected-result
>+	printf "lmn\nabc\nxyz\n" > unify-sort-test/expected-result
> 	@if ! $(srcdir)/macosx/universal/unify --unify-with-sort "\.foo$$" \
>           ./unify-sort-test/a ./unify-sort-test/b \
>           ./unify-sort-test/c; then \
>           echo "TEST-UNEXPECTED-FAIL | build/ | unify failed to unify files with differing line ordering!"; \
>           false; \
>         fi
> 	@if ! diff -q ./unify-sort-test/expected-result ./unify-sort-test/c/file.foo; then \

You could instead just pass unify-sort-test/a instead of expected-result here. Doesn't matter much to me. All this shell scriptery is gross, I should have written it in Python.

Comment 18

[Aki Sasaki (not active)]

Will this break multilocale Maemo builds? (jars+manifests in chrome/)

Comment 19

[Benjamin Smedberg]

I don't know, how are they built?

Comment 20

[Aki Sasaki (not active)]

Putting jars and manifests in chrome/, plus some pref changes and maybe some other magic i don't know of.

Pretty sure this'll break jmaher's fennecmark stuff, but that's already currently broken.

Comment 21

[Benjamin Smedberg]

I was looking for a pointer to the code: if it is just dropping stuff in, yes, that will no longer work. You probably want to generate a chrome/localized.manifest which contains the directives for all the locales combined.

Comment 22

[Benjamin Smedberg]

Aki sent a link to http://hg.mozilla.org/mobile-browser/file/55ecfa303d51/locales/Makefile.in#l117

Where and how is this called?

Comment 23

[Aki Sasaki (not active)]

This is called after building we run |make chrome-AB_CD| for every locale we want added to the multi-locale deb, then make package/make deb.

Comment 24

[Benjamin Smedberg]

In that case I suspect this will "just work".

We'll find out when it gets checked in... waiting for tryserver results, and I'm on vacation tomorrow, so ugh.

Comment 25

[Joel Maher ( :jmaher ) (UTC -8)]

for fennecmark (talos test for fennec), what needs to be done?  Should this be installed in the profile instead of the appdir?

Comment 26

[Benjamin Smedberg]

I don't know. Why can't it do what desktop fennec does and install into distribution/bundles?

Comment 27

[Benjamin Smedberg]

http://hg.mozilla.org/mozilla-central/rev/66e79b756b39

Comment 28

[Nickolay_Ponomarev]

dev-doc-needed: for the new "manifest" command to chrome.manifest (to be documented at https://developer.mozilla.org/en/Chrome_Registration ). Anything else documentation-worthy?

Comment 29

[Benjamin Smedberg]

If there are XULRunner docs which say that manifests may be dropped into chrome/ those should be updated as well.

Comment 30

[Nickolay_Ponomarev]

Thanks, I found https://developer.mozilla.org/en/XUL_Tutorial/Manifest_Files#Manifest_Files and possibly https://developer.mozilla.org/en/Creating_XULRunner_Apps_with_the_Mozilla_Build_System#Okay.2c_okay.2c_can_I_build_it_now.3f

Comment 31

[Eric Shepherd [:sheppy]]

The manifest command has been documented for a while. The other articles mentioned in comment 30 still need updating.

Comment 32

[Eric Shepherd [:sheppy]]

Documentation has been updated here:

https://developer.mozilla.org/en/XUL_Tutorial/Manifest_Files#Manifest_files
https://developer.mozilla.org/en/Chrome_Registration

Linked to from:

https://developer.mozilla.org/en/Firefox_4_for_developers#Other_changes