Last Comment Bug 579178 - Don't enumerate components/*.manifest and chrome/*.manifest
: Don't enumerate components/*.manifest and chrome/*.manifest
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla2.0b4
Assigned To: Benjamin Smedberg [:bsmedberg]
:
Mentors:
: 580044 (view as bug list)
Depends on: 585721 580698 585628 585693 586350 592574
Blocks: 518250 573612 576746 578626 578653 579098 582695 586190
  Show dependency treegraph
 
Reported: 2010-07-15 14:30 PDT by Benjamin Smedberg [:bsmedberg]
Modified: 2011-01-12 14:05 PST (History)
23 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
2.0a1+


Attachments
Part A - XPCOM changes to load submodules and stop enumerating directories, rev. 1 (25.50 KB, patch)
2010-07-15 14:31 PDT, Benjamin Smedberg [:bsmedberg]
dtownsend: review+
bugspam.Callek: feedback+
Details | Diff | Review
Part B - packaging and repackaging changes, rev. 1 (18.26 KB, patch)
2010-07-15 14:32 PDT, Benjamin Smedberg [:bsmedberg]
no flags Details | Diff | Review
Part B - packaging and repackaging changes, rev. 2 (24.89 KB, patch)
2010-07-16 11:41 PDT, Benjamin Smedberg [:bsmedberg]
khuey: review+
bugspam.Callek: feedback+
Details | Diff | Review
Part C - Allow xpcshell to register additional manifests, rev. 1 (3.72 KB, patch)
2010-07-16 11:47 PDT, Benjamin Smedberg [:bsmedberg]
jwalden+bmo: review+
Details | Diff | Review
When checking for sorted-matching, keep version A as the unified version, not the sorted version (3.41 KB, patch)
2010-07-21 10:31 PDT, Benjamin Smedberg [:bsmedberg]
ted: review+
Details | Diff | Review

Description Benjamin Smedberg [:bsmedberg] 2010-07-15 14:30:31 PDT
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 [:bsmedberg] 2010-07-15 14:31:36 PDT
Created attachment 457678 [details] [diff] [review]
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 [:bsmedberg] 2010-07-15 14:32:46 PDT
Created attachment 457679 [details] [diff] [review]
Part B - packaging and repackaging changes, rev. 1
Comment 3 Benjamin Smedberg [:bsmedberg] 2010-07-16 09:37:36 PDT
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 [:bsmedberg] 2010-07-16 11:41:49 PDT
Created attachment 457919 [details] [diff] [review]
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 [:bsmedberg] 2010-07-16 11:47:24 PDT
Created attachment 457921 [details] [diff] [review]
Part C - Allow xpcshell to register additional manifests, rev. 1
Comment 6 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2010-07-16 12:23:20 PDT
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) 2010-07-19 19:52:44 PDT
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) 2010-07-19 20:46:24 PDT
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) 2010-07-20 02:18:53 PDT
(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 10 Mark Finkle (:mfinkle) (use needinfo?) 2010-07-20 12:47:07 PDT
*** Bug 580044 has been marked as a duplicate of this bug. ***
Comment 11 Mark Finkle (:mfinkle) (use needinfo?) 2010-07-20 12:50:15 PDT
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 [:bsmedberg] 2010-07-20 12:54:38 PDT
> 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] 2010-07-20 13:45:00 PDT
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 [:bsmedberg] 2010-07-21 10:31:41 PDT
Created attachment 459065 [details] [diff] [review]
When checking for sorted-matching, keep version A as the unified version, not the sorted version
Comment 15 Ted Mielczarek [:ted.mielczarek] 2010-07-21 13:36:48 PDT
The mochitest-chrome stuff sounds like it overlaps some work Joel was doing.
Comment 16 Joel Maher (:jmaher) 2010-07-21 19:18:05 PDT
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 Ted Mielczarek [:ted.mielczarek] 2010-07-28 08:15:23 PDT
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 [:aki] 2010-08-05 11:01:23 PDT
Will this break multilocale Maemo builds? (jars+manifests in chrome/)
Comment 19 Benjamin Smedberg [:bsmedberg] 2010-08-05 11:02:42 PDT
I don't know, how are they built?
Comment 20 Aki Sasaki [:aki] 2010-08-05 11:15:04 PDT
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 [:bsmedberg] 2010-08-05 12:03:29 PDT
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 [:bsmedberg] 2010-08-05 12:18:41 PDT
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 [:aki] 2010-08-05 13:00:04 PDT
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 [:bsmedberg] 2010-08-05 13:01:39 PDT
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) 2010-08-05 13:23:55 PDT
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 [:bsmedberg] 2010-08-05 13:26:37 PDT
I don't know. Why can't it do what desktop fennec does and install into distribution/bundles?
Comment 27 Benjamin Smedberg [:bsmedberg] 2010-08-09 09:01:58 PDT
http://hg.mozilla.org/mozilla-central/rev/66e79b756b39
Comment 28 Nickolay_Ponomarev 2010-08-12 17:01:13 PDT
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 [:bsmedberg] 2010-08-12 17:06:04 PDT
If there are XULRunner docs which say that manifests may be dropped into chrome/ those should be updated as well.
Comment 31 Eric Shepherd [:sheppy] 2010-12-10 08:56:46 PST
The manifest command has been documented for a while. The other articles mentioned in comment 30 still need updating.

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