Many of the files in the application/GRE directory can be placed into a single jar.

These patches are a port of some of the packaging changes in the mozilla-droid tree. This patch series packages the chrome, components, res, modules, defaults, and greprefs.js directories/files into a single zip file and makes the code understand how to read everything out of this omnijar. I don't have a very good test setup but on my system, but cold start drops about 200 ms from ~2200ms to ~2000ms. Additional improvement has been seen from tracing and reordering the omnijar but I don't currently trust my numbers enough to say how much better things are with a reordered omnijar.

Michael this is very cool stuff. What relation is this stuff to the droid port? ie is this the suggested format for shipping ff on Android? Reordering benefit depends on how many megabytes of data you read out of the jar. One problem I had with sticking preferences into a jar, is that jar stuff reads preferences to initialize(creating a circular dependency). For future reference, please add showfunc=true to [diff] section of your hdrc, this will provide more context info in your patches.

(In reply to comment #13) > Michael this is very cool stuff. What relation is this stuff to the droid port? > ie is this the suggested format for shipping ff on Android? > It's not strictly required to ship on Android, but it's a lot better than the alternative. Android packages (APKs) are basically JARs and we want to read as much out of the JAR as possible instead of extracting the files and taking more space. This is what normal android apps do. The original APK we extract from cannot be deleted or modified. > Reordering benefit depends on how many megabytes of data you read out of the > jar. > I haven't gotten to measuring exactly how much data is involved, but I figured it should be a significant improvement due to improved locality and sequential read of files from the jar. Out of the ~1000 files in the omnijar, only ~100 are actually read during startup. (if my tracing patch is accurate) > One problem I had with sticking preferences into a jar, is that jar stuff reads > preferences to initialize(creating a circular dependency). > This only appears to happen if using nsJARChannel. The patch here use omnijar doesn't do that. We need to be able to iterate through all files in the default prefs directory. > For future reference, please add showfunc=true to [diff] section of your hdrc, > this will provide more context info in your patches. Ah, will do. I had something like this in another .hgrc but it looks like I didn't bring it over to my new system..

Some bits in patch #4 should be in this patch.

Comment on attachment 432263 [details] [diff] [review] 1. Add omnijar as a configure option Overloading --enable-chrome-format for omnijar doesn't make complete sense, but it's done here since it can only be used with flat chrome. (and it's not entirely flat since it must end up in a jar to work..)

Comment on attachment 432264 [details] [diff] [review] 2. Generate omnijar (WIP) This part of the series is still a WIP. It has a number of issues. 1. I don't know exactly where to put the omnijar packaging phase. 2. Need to find something better than hardcoding files to zip into the omnijar. 3. |make package| doesn't seem to pick up the app binary from the dist/bin directory, so it doesn't work. 4. I don't understand the xptlink script very well. It seems to give me a bin.xpt instead of browser.xpt.

Concatenate all the manifest files together and call it chrome.manifest. This significantly simplifies patch #4 (Point chrome uris to the omnijar) by eliminating the need to enumerate chrome/*.manifest.

Comment on attachment 432270 [details] [diff] [review] 7. Have the expat driver look inside the omnijar for dtds Canceling review after discussion with taras

This is the single-most-startup-improving bug I've seen. I just tested a build with mwu's patches and one without, got 2.5s vs 4s(almost 40%). If we are going to significantly improve our startup speed in Firefox, this extreme filesystem makeover is the way.

Comment on attachment 432270 [details] [diff] [review] 7. Have the expat driver look inside the omnijar for dtds This patch has been moved to bug 553121 .

(In reply to comment #24) > (From update of attachment 432270 [details] [diff] [review]) > This patch has been moved to bug 553121 . Setting this to block on 553121, should use this bug as a tracking bug.

Comment on attachment 432272 [details] [diff] [review] 8. Find xpts inside the omnijar >diff --git a/xpcom/build/nsXPComInit.cpp b/xpcom/build/nsXPComInit.cpp >--- a/xpcom/build/nsXPComInit.cpp >+++ b/xpcom/build/nsXPComInit.cpp >@@ -674,16 +674,11 @@ > nsSimpleUnicharStreamFactory::GetInstance()); > } > >- // Pay the cost at startup time of starting this singleton. >- nsIInterfaceInfoManager* iim = >- xptiInterfaceInfoManager::GetInterfaceInfoManagerNoAddRef(); >- > // "Re-register the world" if compreg.dat doesn't exist > rv = nsComponentManagerImpl::gComponentManager->ReadPersistentRegistry(); > if (NS_FAILED(rv)) { > // If the component registry is out of date, malformed, or incomplete, > // autoregister the default component directories. >- (void) iim->AutoRegisterInterfaces(); > nsComponentManagerImpl::gComponentManager->AutoRegister(nsnull); > } > Is this change in patch #8 intentional?

(In reply to comment #26) > Is this change in patch #8 intentional? Yup. Autoregistering interfaces at this point fails for omnijar since the jar module isn't loaded. Removing this early registration is fine since it gets done later while loading the xpconnect module. This is why the jar module is moved before xpconnect in nsStaticXULComponents.cpp - to ensure that xpts can be loaded from jars.

Comment on attachment 432263 [details] [diff] [review] 1. Add omnijar as a configure option This could probably use a short comment in the block where you set MOZ_CHROME_FILE_FORMAT=flat, because that's not at all intuitive.

Comment on attachment 432687 [details] [diff] [review] 2. Generate omnijar, v2 (WIP) So your plan is just to build with flat chrome in the objdir, then do all the omnijar work at package time? That seems ok, especially since this is targeted at android, which you won't be able to run out of your objdir anyway. That big block in browser/app/Makefile.in, though, that's not gonna fly. You're duplicating a whole bunch of our existing packager logic, which just doesn't make sense. What you want to do here is put some logic in the stage-package target ifdef MOZ_OMNIJAR, but after the packager-copy and xptlink bits have run, so basically right here: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/packager.mk#380 That's inside a block for MOZ_PKG_MANIFEST, but I think it's ok to require a package manifest to get this behavior. Alternately, you can stick it right before this echo: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/packager.mk#435 which will cover both cases. Anyway, either place you put it, at that point $(DIST)/$(STAGEPATH)$(MOZ_PKG_DIR) will have all the bits that are staged for packaging, so you can munge them however you like, leaving the results in that same directory, and then the build system will just package up whatever you leave there into the zip/tar/dmg/whatever. Does that sound workable?

Also, I forgot to mention, but if you're going to do non-trivial stuff, I'd support you writing a small Python script to do the work for you, since it's going to be much clearer than a bunch of shell commands.

(In reply to comment #29) > (From update of attachment 432687 [details] [diff] [review]) > So your plan is just to build with flat chrome in the objdir, then do all the > omnijar work at package time? That seems ok, especially since this is targeted > at android, which you won't be able to run out of your objdir anyway. FWIW, we'll want this for all platforms -- it's a pretty significant startup win.

Comment on attachment 432609 [details] [diff] [review] 3. Allow the directory service to find the omnijar, v2 >+#if defined(MOZ_OMNIJAR) >+ dirProvider.SetOmniJAR(argv[0]); >+#endif Add a comment saying where argv[0] comes from, esp on Android? On non-Android this can also include the full path which I think will screw up SetOmniJAR internals. That can be fixed in a followup, I think. >+ >+void >+nsXREDirProvider::SetOmniJAR(char* aJarPath) Not really a aJarPath, more like a aJARFileName (decide on one capitalization of JAR, too, please :-). Path implies a full pathname to me. Looks fine otherwise.

Comment on attachment 432703 [details] [diff] [review] 9. Load js modules and components from the omnijar (with -U 8 -p) This looks OK to me, except for the fastload issue -- should resolve that one way or the other. It's something that can be fixed after landing in a followup, though. >+ nsCOMPtr<nsIURI> uri; >+ if (aComponentFile) { >+ nsCAutoString spec; >+ NS_GetURLSpecFromActualFile(aComponentFile, spec); >+ >+ rv = NS_NewURI(getter_AddRefs(uri), spec); >+ NS_ENSURE_SUCCESS(rv, rv); >+ } else >+ uri = aComponentURI; Stick some { } around this else block, don't let it dangle. > JSObject* file_jsobj; ^ initialize this to nsnull, for sanity. This means that JS components loaded from omnijars will get null for the NSGetModule's file param, but that's specced to possibly be null anyway. >+ // XXX: should we assign something else to __LOCATION__ for non localfiles A handful of tests seem to use __LOCATION__ but it's unused other than that. I think we may even want to deprecate it, even. > if (fastLoading) { > // We successfully compiled the script, so cache it in fastload. >- rv = WriteScript(flSvc, script, aComponent, nativePath.get(), uri, cx); >+ if (componentFile) >+ rv = WriteScript(flSvc, script, componentFile, nativePath.get(), aURI, cx); >+ else >+ rv = NS_ERROR_FAILURE; errm, we don't get fastload with omnijar? Is this just because we don't have a nativePath? If so I bet you can use the URI spec as the nativePath, which is what the code path at http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/loader/mozJSComponentLoader.cpp#1171 does -- just make it grab the URI spec if we have a URI. That should allow fastload to work fine, I think. >+#ifdef MOZ_OMNIJAR >+#include "nsIZipReader.h" >+static NS_DEFINE_CID(kZipReaderCID, NS_ZIPREADER_CID); >+ >+void >+nsComponentManagerImpl::AutoRegisterJar() >+{ ... >+ >+ nsCOMPtr<nsILocalFile> fakeFile; >+ rv = NS_NewNativeLocalFile(Substring(compName, startIdx), PR_FALSE, getter_AddRefs(fakeFile)); >+ if (NS_FAILED(rv)) { >+ NS_ERROR("Failed to create fake local file for JS component"); >+ continue; >+ } >+ >+ rv = module->RegisterSelf(this, fakeFile, >+ entryspec.get(), >+ jarComponentType); I'm reasonably sure you can just pass nsnull instead of fakeFile here and below. >+ if (NS_ERROR_FACTORY_REGISTER_AGAIN == rv) { >+ DeferredModule *d = deferred.AppendElement(); >+ if (!d) { >+ NS_ERROR("Failed to allocate DeferredModule"); >+ continue; >+ } >+ >+ d->file = fakeFile; >+ d->location = entryspec; >+ d->module = module; >+ } >+ } >+ >+ for (PRInt32 i = 0; i < deferred.Length(); i++) { >+ DeferredModule &d = deferred[i]; >+ rv = d.module->RegisterSelf(this, d.file, >+ d.location.get(), >+ jarComponentType); >+ if (NS_FAILED(rv)) >+ NS_ERROR("js component failed to load on reregistration"); >+ } >+} >+#endif /* MOZ_OMNIJAR */

Comment on attachment 432267 [details] [diff] [review] 5. Point resource uris to the omnijar Straightforward, looks fine to me

(In reply to comment #33) > (From update of attachment 432703 [details] [diff] [review]) > > JSObject* file_jsobj; > > ^ initialize this to nsnull, for sanity. This means that JS components loaded > from omnijars will get null for the NSGetModule's file param, but that's > specced to possibly be null anyway. > There's a special case to use a JSVAL_NULL when this is null. AFAICT, passing null to OBJECT_TO_JSVAL will cause an assertion, so there is a special case to ensure file_jsobj isn't used if it shouldn't be assigned. > > if (fastLoading) { > > // We successfully compiled the script, so cache it in fastload. > >- rv = WriteScript(flSvc, script, aComponent, nativePath.get(), uri, cx); > >+ if (componentFile) > >+ rv = WriteScript(flSvc, script, componentFile, nativePath.get(), aURI, cx); > >+ else > >+ rv = NS_ERROR_FAILURE; > > errm, we don't get fastload with omnijar? Is this just because we don't have a > nativePath? If so I bet you can use the URI spec as the nativePath, which is > what the code path at > http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/loader/mozJSComponentLoader.cpp#1171 > does -- just make it grab the URI spec if we have a URI. That should allow > fastload to work fine, I think. > As far as I can tell, fastload works. I guess this code isn't necessary.. need to check. > >+#ifdef MOZ_OMNIJAR > >+#include "nsIZipReader.h" > >+static NS_DEFINE_CID(kZipReaderCID, NS_ZIPREADER_CID); > >+ > >+void > >+nsComponentManagerImpl::AutoRegisterJar() > >+{ > ... > >+ > >+ nsCOMPtr<nsILocalFile> fakeFile; > >+ rv = NS_NewNativeLocalFile(Substring(compName, startIdx), PR_FALSE, getter_AddRefs(fakeFile)); > >+ if (NS_FAILED(rv)) { > >+ NS_ERROR("Failed to create fake local file for JS component"); > >+ continue; > >+ } > >+ > >+ rv = module->RegisterSelf(this, fakeFile, > >+ entryspec.get(), > >+ jarComponentType); > > I'm reasonably sure you can just pass nsnull instead of fakeFile here and > below. > I'll try removing it but I remember putting this in for a reason.. > >+ if (NS_ERROR_FACTORY_REGISTER_AGAIN == rv) { > >+ DeferredModule *d = deferred.AppendElement(); > >+ if (!d) { > >+ NS_ERROR("Failed to allocate DeferredModule"); > >+ continue; > >+ } > >+ > >+ d->file = fakeFile; > >+ d->location = entryspec; > >+ d->module = module; > >+ } > >+ } > >+ > >+ for (PRInt32 i = 0; i < deferred.Length(); i++) { > >+ DeferredModule &d = deferred[i]; > >+ rv = d.module->RegisterSelf(this, d.file, > >+ d.location.get(), > >+ jarComponentType); > >+ if (NS_FAILED(rv)) > >+ NS_ERROR("js component failed to load on reregistration"); > >+ } > >+} > >+#endif /* MOZ_OMNIJAR */

(In reply to comment #35) > > I'm reasonably sure you can just pass nsnull instead of fakeFile here and > > below. > > > I'll try removing it but I remember putting this in for a reason.. > And here is the reason: ************************************************************ * Call to xpconnect wrapped JSObject produced this error: * [Exception... "'[JavaScript Error: "fileSpec is null" {file: "resource://gre/modules/XPCOMUtils.jsm" line: 178}]' when calling method: [nsIModule::registerSelf]" nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)" location: "<unknown>" data: yes] ************************************************************

Vlad's review comments addressed

AutoRegisterJar probably shouldn't be defined if there is no implementation.

Same patch as before, with more context. The new approach attempted in bug 553428 doesn't work.

Comment on attachment 432609 [details] [diff] [review] 3. Allow the directory service to find the omnijar, v2 >diff --git a/toolkit/xre/nsAppRunner.cpp b/toolkit/xre/nsAppRunner.cpp >+ nsXREDirProvider dirProvider; It looks like this change in scope is ok. It should still be nested inside ScopedLogging via C++ local nesting rules. >+#if defined(MOZ_OMNIJAR) >+ dirProvider.SetOmniJAR(argv[0]); >+#endif I really don't like this API, see below. >diff --git a/toolkit/xre/nsXREDirProvider.cpp b/toolkit/xre/nsXREDirProvider.cpp >+nsXREDirProvider::SetOmniJAR(char* aJarPath) >+{ >+ nsresult rv; >+ rv = NS_NewNativeLocalFile(nsDependentCString(aJarPath), PR_TRUE, getter_AddRefs(mOmniJARFile)); As noted below, "Native" is incorrect on Windows. >+ if (NS_SUCCEEDED(rv) || rv != NS_ERROR_FILE_UNRECOGNIZED_PATH) >+ return; >+ >+ nsCOMPtr<nsIFile> fixedPath; >+ rv = mXULAppDir->Clone(getter_AddRefs(fixedPath)); I don't understand what's going on here. The rv-check (UNRECOGNIZED_PATH) doesn't actually tell you anything about whether the file *exists*, only if the path is malformed. Are we sometimes passing malformed data in aJarPath? All this fallback code doesn't make much sense to me yet. If you're trying to get the path to the argv[0] binary, use XRE_GetBinaryPath instead of this code. >+ fixedPath->QueryInterface(NS_GET_IID(nsILocalFile), getter_AddRefs(mOmniJARFile)); rewrite this as mOmniJARFile = do_QueryInterface(fixedPath); >@@ -702,10 +736,12 @@ nsXREDirProvider::GetFilesInternal(const > else if (!strcmp(aProperty, NS_CHROME_MANIFESTS_FILE_LIST)) { > nsCOMArray<nsIFile> manifests; > >+#ifndef MOZ_OMNIJAR > nsCOMPtr<nsIFile> manifest; > mGREDir->Clone(getter_AddRefs(manifest)); > manifest->AppendNative(NS_LITERAL_CSTRING("chrome")); > manifests.AppendObject(manifest); >+#endif See question below about whether defining MOZ_OMNIJAR implies that we are definitely using it, or only might be using it. And whether that applies to just the GRE. >diff --git a/toolkit/xre/nsXREDirProvider.h b/toolkit/xre/nsXREDirProvider.h >+ void SetOmniJAR(char* aJarPath); This API needs documentation. Currently you're passing in argv[0]. I believe you said that the JAR data is appended to the end of the fennec binary? This all needs to be written down here in more detail. Also, this char* needs some information about what charset we're using. The data which comes in to XRE_main is UTF8 on Windows (so that it can represent the full unicode repertoire) and native (usually UTF8 but not always) on other systems. The handling of this parameter needs to be really precise. >diff --git a/xpcom/io/nsAppDirectoryServiceDefs.h b/xpcom/io/nsAppDirectoryServiceDefs.h >+#define NS_OMNIJAR_FILE "OmniJAR" This needs significantly more documentation. Who is expected to use this key? What exactly does it point to? A JAR file? A JAR file appended to an executable? A directory containing a JAR file? Does defining MOZ_OMNIJAR imply that we're automatically using it? Or does it depend on some runtime configuration? Please document that apprunner provides this key.

Comment on attachment 432688 [details] [diff] [review] 4. Point chrome uris to the omnijar, v3 It really seems that this code would be better off using nsZipReader/nsZipItem directly. Assuming that omnijar is only going to work with a libxul/static build, we should just link and use those APIs directly, rather than going through XPCOM. The only problem with this, perhaps, is that using zipreader directly would skip the JAR cache. If you don't want to do this now, please file a followup. This XPCOM goop is killing me. Also, this code seems to assume that if MOZ_OMNIJAR is defined there will always be an omnijar. Perhaps we can just fall back to the regular codepath if omnijar is not present (e.g. running from dist/bin even in an omnijar build). I do want this code to be moved to a separate method, e.g. nsChromeRegistry::ProcessOmnijarManifest. r=me with that change. Feel free to re-request review if you have questions, I'll be quick this time, I promise ;-)

(In reply to comment #42) > (From update of attachment 432688 [details] [diff] [review]) > It really seems that this code would be better off using nsZipReader/nsZipItem > directly. Assuming that omnijar is only going to work with a libxul/static > build, we should just link and use those APIs directly, rather than going > through XPCOM. The only problem with this, perhaps, is that using zipreader > directly would skip the JAR cache. Good idea. Providing a decomtaminated jar cache should be an easy change.

Comment on attachment 436580 [details] [diff] [review] 9. Load js modules and components from the omnijar, v3 (with -U 12 -p) I don't think the LoadModuleFromURI method makes a lot of sense... it's too general, and requires the module loader to parse the URL. Can we do something simpler like LoadModuleFromJAR(jarpath, internalpath)?

Comment on attachment 436580 [details] [diff] [review] 9. Load js modules and components from the omnijar, v3 (with -U 12 -p) What the code is doing looks reasonable, but it's factored all wrong. Rewrite it such that every function doesn't have a conditional looking for a URI or File object.

(In reply to comment #44) > (From update of attachment 436580 [details] [diff] [review]) > I don't think the LoadModuleFromURI method makes a lot of sense... it's too > general, and requires the module loader to parse the URL. Can we do something > simpler like LoadModuleFromJAR(jarpath, internalpath)? Yes we can. It may even simplify the code a bit. (In reply to comment #45) > (From update of attachment 436580 [details] [diff] [review]) > What the code is doing looks reasonable, but it's factored all wrong. Rewrite > it such that every function doesn't have a conditional looking for a URI or > File object. I'll see what I can do.

Comment on attachment 436621 [details] [diff] [review] 5. Point resource uris to the omnijar (with -U 12 -p) +++ b/netwerk/protocol/res/src/nsResProtocolHandler.cpp + if (host.Length() == 0) + return NS_ERROR_NO_INTERFACE; if (host.IsEmpty()) + rv = mIOService->NewURI(strGRE_URL, nsnull, greURI, + getter_AddRefs(greURI)); why are you passing greURI here? It's null anyway. NS_NAMED_LITERAL_CSTRING(strGRE_DIR, "gre"); rv = AddSpecialDir(NS_GRE_DIR, strGRE_DIR); maybe use kGRE instead of defining this named literal string. Or you could remove the define and move strGRE_DIR to the top of the function and use that.

Does this affect l10n repacks? Didn't go through all patches, but some comments indicate that it'd change both firefox-l10n.js and en-US.jar. If it does affect l10n repacks, how do those changes affect multi-locale builds like the one we're shipping for maemo?

Comment on attachment 436621 [details] [diff] [review] 5. Point resource uris to the omnijar (with -U 12 -p) >diff --git a/netwerk/protocol/res/src/nsResProtocolHandler.cpp b/netwerk/protocol/res/src/nsResProtocolHandler.cpp > > rv = net_GetFileFromURLSpec(spec, getter_AddRefs(mFile)); > #ifdef DEBUG_bsmedberg > if (NS_SUCCEEDED(rv)) { > PRBool exists = PR_TRUE; > mFile->Exists(&exists); > if (!exists) { > printf("resource %s doesn't exist!\n", spec.get()); > } > } > #endif > >@@ -165,43 +173,72 @@ nsResProtocolHandler::AddSpecialDir(cons > > nsresult > nsResProtocolHandler::Init() > { > if (!mSubstitutions.Init(32)) > return NS_ERROR_UNEXPECTED; > > nsresult rv; > > mIOService = do_GetIOService(&rv); > NS_ENSURE_SUCCESS(rv, rv); > >+#ifdef MOZ_OMNIJAR >+ nsCOMPtr<nsIFile> omniJar; >+ rv = NS_GetSpecialDirectory(NS_OMNIJAR_FILE, getter_AddRefs(omniJar)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ nsCAutoString omniJarSpec; >+ NS_GetURLSpecFromActualFile(omniJar, omniJarSpec); >+ >+ nsCAutoString strGRE_URL("jar:"); >+ strGRE_URL += omniJarSpec; >+ strGRE_URL += "!/"; >+#endif >+ > // > // make resource:/// point to the application directory > // >+ nsCOMPtr<nsIURI> greURI; >+#ifndef MOZ_OMNIJAR > rv = AddSpecialDir(NS_OS_CURRENT_PROCESS_DIR, EmptyCString()); > NS_ENSURE_SUCCESS(rv, rv); >+#else >+ rv = mIOService->NewURI(strGRE_URL, nsnull, greURI, >+ getter_AddRefs(greURI)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ SetSubstitution(EmptyCString(), greURI); >+#endif > > // > // make resource://gre/ point to the GRE directory > // >+#ifndef MOZ_OMNIJAR > NS_NAMED_LITERAL_CSTRING(strGRE_DIR, "gre"); > rv = AddSpecialDir(NS_GRE_DIR, strGRE_DIR); > NS_ENSURE_SUCCESS(rv, rv); >+#else >+ SetSubstitution(kGRE, greURI); >+#endif > > // make resource://gre-resources/ point to gre toolkit[.jar]/res >- nsCOMPtr<nsIURI> greURI; > nsCOMPtr<nsIURI> greResURI; >+#ifndef MOZ_OMNIJAR > GetSubstitution(strGRE_DIR, getter_AddRefs(greURI)); >+#endif > #ifdef MOZ_CHROME_FILE_FORMAT_JAR > NS_NAMED_LITERAL_CSTRING(strGRE_RES_URL, "jar:chrome/toolkit.jar!/res/"); >+#elif defined(MOZ_OMNIJAR) >+ nsCAutoString strGRE_RES_URL = strGRE_URL; >+ strGRE_RES_URL += "chrome/toolkit/res/"; > #else > NS_NAMED_LITERAL_CSTRING(strGRE_RES_URL, "chrome/toolkit/res/"); > #endif > rv = mIOService->NewURI(strGRE_RES_URL, nsnull, greURI, > getter_AddRefs(greResURI)); > SetSubstitution(kGRE_RESOURCES, greResURI); > //XXXbsmedberg Neil wants a resource://pchrome/ for the profile chrome dir... > // but once I finish multiple chrome registration I'm not sure that it is needed > > // XXX dveditz: resource://pchrome/ defeats profile directory salting > // if web content can load it. Tread carefully. >

I added mozilla/Omnijar.h and mozilla::OmnijarPath(). OmnijarPath currently just returns something from XRE_GetBinaryPath, but it could be configured to something else if necessary. The argv0 argument in XRE_GetBinaryPath is no longer necessary with this patch but I wasn't sure if I could change that API. I'd like to cache the omnijar nsILocalFile and an omnijar nsIZipReader but I'm not sure where to stick some nsCOMPtrs to hold on to them.

(In reply to comment #48) > Does this affect l10n repacks? Didn't go through all patches, but some comments > indicate that it'd change both firefox-l10n.js and en-US.jar. > I don't know what l10n repacks involve. Omnijar is similar to flat packaging - there is a en-US/ instead of a en-US.jar, but that directory is inside the omnijar.

You should figure out what to do the the installers-% etc target in browser/locales/Makefile.in, part of that code is living in toolkit/locales/l10n.mk and the packager.mk files. That code might or might not be shared with thunderbird or seamonkey, don't recall right now.

Comment on attachment 439415 [details] [diff] [review] 3. Allow the directory service to find the omnijar, v3 >diff --git a/toolkit/xre/nsAppRunner.cpp b/toolkit/xre/nsAppRunner.cpp >@@ -1536,6 +1531,9 @@ XRE_GetBinaryPath(const char* argv0, nsI > } > > #endif >+ if (!argv0) >+ argv0 = gArgv[0]; >+ Why are you doing this? The caller is incorrect in this case, and I don't want magic to start happening here. Note that this API must work before XRE_main is called, and also in cases where XRE_main is never called. >diff --git a/xpcom/build/Omnijar.cpp b/xpcom/build/Omnijar.cpp >+nsILocalFile * >+mozilla::OmnijarPath() >+{ >+ nsILocalFile *file; >+ XRE_GetBinaryPath(nsnull, &file); >+ return file; >+ I still don't think I understand. The binary path is typically installed/bin/firefox-bin. Are you saying that firefox-bin *is* the omnijar? How does the zipreader know to skip the binary header and go to the actual ZIP data? As implemented, this should return an already_AddRefed. Although I think you should probably set this value on startup and cache it, since XRE_GetBinaryPath can be a bit expensive. >diff --git a/xpcom/build/Omnijar.h b/xpcom/build/Omnijar.h >+/** >+ * This returns the path to the omnijar. >+ * If the omnijar isn't available, this function will return null. >+ * Callers should fallback to flat packaging if null. >+ */ >+nsILocalFile *OmnijarPath(); There's no accessor for the cached nsZipArchive?

(In reply to comment #53) > (From update of attachment 439415 [details] [diff] [review]) > >diff --git a/toolkit/xre/nsAppRunner.cpp b/toolkit/xre/nsAppRunner.cpp > > >@@ -1536,6 +1531,9 @@ XRE_GetBinaryPath(const char* argv0, nsI > > } > > > > #endif > >+ if (!argv0) > >+ argv0 = gArgv[0]; > >+ > > Why are you doing this? The caller is incorrect in this case, and I don't want > magic to start happening here. Note that this API must work before XRE_main is > called, and also in cases where XRE_main is never called. > The code where XRE_GetBinaryPath (in Omnijar.cpp) is called from cannot access argv[0] easily. I guess it won't matter once the path gets cached. > >diff --git a/xpcom/build/Omnijar.cpp b/xpcom/build/Omnijar.cpp > > >+nsILocalFile * > >+mozilla::OmnijarPath() > >+{ > >+ nsILocalFile *file; > >+ XRE_GetBinaryPath(nsnull, &file); > >+ return file; > >+ > > I still don't think I understand. The binary path is typically > installed/bin/firefox-bin. Are you saying that firefox-bin *is* the omnijar? > How does the zipreader know to skip the binary header and go to the actual ZIP > data? > The "header" for zip files is at the end of the file. zipreader searches backwards for the central directory. > As implemented, this should return an already_AddRefed. Although I think you > should probably set this value on startup and cache it, since XRE_GetBinaryPath > can be a bit expensive. > Yeah, I agree. I'm just uncertain where to cache it. > >diff --git a/xpcom/build/Omnijar.h b/xpcom/build/Omnijar.h > > >+/** > >+ * This returns the path to the omnijar. > >+ * If the omnijar isn't available, this function will return null. > >+ * Callers should fallback to flat packaging if null. > >+ */ > >+nsILocalFile *OmnijarPath(); > > There's no accessor for the cached nsZipArchive? Not yet. Next patch. :)

mozilla::SetOmnijar and mozilla::OmnijarReader added. The use of nsRefPtrs to cache things is a bit weird, but it seems like using nsCOMPtr is even less pleasant. Any suggestions for a better way to cache these things is welcome.

Oops. Attached the wrong patch before.

Comment on attachment 440391 [details] [diff] [review] 3. Allow the directory service to find the omnijar, v4.1 Static refptr/comptrs are a bad idea. Please use a raw pointer and manual refcounting. I so so want mozilla::OmnijarReader to return a nsZipArchive, not a nsIZipReader. Are you sticking with nsIZipReader because nsZipArchive doesn't do everything you need? Or because you're having linkage issues and can't use nsZipArchive directly?

(In reply to comment #57) > (From update of attachment 440391 [details] [diff] [review]) > Static refptr/comptrs are a bad idea. Please use a raw pointer and manual > refcounting. > Ok. > I so so want mozilla::OmnijarReader to return a nsZipArchive, not a > nsIZipReader. Are you sticking with nsIZipReader because nsZipArchive doesn't > do everything you need? Or because you're having linkage issues and can't use > nsZipArchive directly? nsZipArchive is fairly raw. It lets us look at what's in the archive but it doesn't let us get an input stream (have to make your own nsJARInputStream for that) and it doesn't feature the zip entry filtering/enumeration. It also doesn't have the caching that nsIZipReader does, though I don't know if we currently take advantage of it.

Yes, that raw-ness is what makes it attractive in some cases: if you can get direct access to the data as a buffer, rather than having to convert it to/from an XPCOM stream, you can avoid a fair bit of copying and complexity, it seems.

(In reply to comment #58) > nsZipArchive is fairly raw. It lets us look at what's in the archive but it > doesn't let us get an input stream (have to make your own nsJARInputStream for > that) and it doesn't feature the zip entry filtering/enumeration. It also > doesn't have the caching that nsIZipReader does, though I don't know if we > currently take advantage of it. zipreader does enough enumeration for omnijar uses afaik. InputStreams support is just a feature to fit in with inputstream-expecting mozilla world, there is no reason to use that for new code that is explicitly accessing jar contents.

(In reply to comment #59) > Yes, that raw-ness is what makes it attractive in some cases: if you can get > direct access to the data as a buffer, rather than having to convert it to/from > an XPCOM stream, you can avoid a fair bit of copying and complexity, it seems. It's compressed, though. We'd have to add decompression code to nsZipArchive/nsZipItem.

(In reply to comment #61) > It's compressed, though. We'd have to add decompression code to > nsZipArchive/nsZipItem. That should be easy to factor out from jarinputstream.

This patch is a bit simpler, but it modifies the 2nd argument to NSGetModule to simplify things. Instead of a file, a URI is passed. I couldn't find any users in the tree that actually used the second argument. The LoadModuleFromURI API hasn't changed. The module loader needs a URI eventually so passing it something that can be assembled into a URI doesn't seem like it'll help much. We could possibly just pass it a nsIURI though, so the module loader doesn't need to call NS_NewURI.

Comment on attachment 443534 [details] [diff] [review] 3a. Allow users to find the omnijar, v5 >diff --git a/toolkit/xre/nsXREDirProvider.cpp b/toolkit/xre/nsXREDirProvider.cpp >- nsCOMPtr<nsIFile> manifest; >- mGREDir->Clone(getter_AddRefs(manifest)); >- manifest->AppendNative(NS_LITERAL_CSTRING("chrome")); >- manifests.AppendObject(manifest); >+#ifdef MOZ_OMNIJAR >+ nsCOMPtr<nsILocalFile> omniJar(mozilla::OmnijarPath()); >+ if (!omniJar) { Why bother with the nsCOMPtr here? OmnijarPath returns a raw pointer, and you don't actually use the variable, just null-test it. >diff --git a/xpcom/build/Omnijar.cpp b/xpcom/build/Omnijar.cpp >+void >+mozilla::SetOmnijar(nsILocalFile *aPath) >+{ >+ if (!aPath) { >+ sOmnijarReader->CloseArchive(); >+ free(sOmnijarReader); >+ sOmnijarReader = nsnull; >+ } >+ NS_IF_RELEASE(sOmnijarPath); >+ sOmnijarPath = aPath; >+ NS_IF_ADDREF(sOmnijarPath); >+} This method is called unconditionally from the XRE code, but doesn't actually check to see whether the path contains a valid omnijar. Won't this still break the "run from dist/bin" case? >+#ifndef mozilla_Omnijar_h >+#define mozilla_Omnijar_h >+ >+#include "nsILocalFile.h" >+#include "nsZipArchive.h" I don't think you really need to #include either of those here, please just forward-declare them. >+#ifdef MOZ_OMNIJAR >+namespace mozilla { >+/** >+ * This returns the path to the omnijar. >+ * If the omnijar isn't available, this function will return null. >+ * Callers should fallback to flat packaging if null. >+ */ >+nsILocalFile *OmnijarPath(); >+nsZipArchive *OmnijarReader(); >+void SetOmnijar(nsILocalFile *aPath); >+} >+#endif >+ >+#endif Style nits: * extra line of whitespace after the namespace decl and before the namespace closing * snuggle the * with the type, so nsILocalFile* OmnijarPath(); * namespace-close and the final ifdefs should have comments such as } // namespace mozilla #endif // MOZ_OMNIJAR #endif // mozilla_Omnijar_h r=me with nits picked

Comment on attachment 444474 [details] [diff] [review] 9. Load js modules and components from the omnijar, v4 >diff --git a/js/src/xpconnect/loader/mozJSComponentLoader.cpp b/js/src/xpconnect/loader/mozJSComponentLoader.cpp >+NS_IMETHODIMP >+mozJSComponentLoader::LoadModuleFromURI(const nsACString &aComponentSpec, >+ nsIModule* *aResult) Why are you using URIs here? I thought we were going to make this method take a JAR file/path, or even LoadModuleFromOmnijar(const nsACString& path). URIs are way too complicated for something this simple. >+ PRUint32 hash = nsCRT::HashCode(uriStr.get()); I don't understand what you're doing with mModules at all. It seems like you're assuming that there won't be any hash collisions. The reason we changed *from* hashing paths to using nsIHashable is because the win32 file system is not case-sensitive and short/long paths need to be equivalent, so we hash on the short pathname. You could perhaps come up with a simpler system or normalize the paths somewhere else, but what you're doing here doesn't seem like it could be correct. >+ JSObject* uri_jsobj; >+ nsCOMPtr<nsIXPConnectJSObjectHolder> uri_holder; >+ rv = xpc->WrapNative(cx, entry->global, aComponentURI, >+ NS_GET_IID(nsIURI), >+ getter_AddRefs(uri_holder)); I suggest that JS components loaded from the omnijar just don't have a __LOCATION__ at all, and the second argument to NSGetModule should be null. Let's avoid URIs and nsIURI completely in this code.

All review comments addressed.

Comment on attachment 443538 [details] [diff] [review] 3b. Add ExtractBuffer API for extracting files into a buffer working on this in bug 566686

http://hg.mozilla.org/mozilla-central/rev/cb427e0c0ed6 (first patch adding omnijar to configure)

Updated to use the latest omnijar and nszipcursor apis.

Updated to use the new omnijar api.

Use new omnijar api.

Use latest omnijar api.

Comment on attachment 446371 [details] [diff] [review] 4. Point chrome uris to the omnijar, v4 >+ nsZipCursor cursor(manifest, jarReader, outbuf, len); >+ PRUint32 readlen; >+ PRUint8* buf = cursor.Read(&readlen); >+ NS_ENSURE_TRUE(buf, NS_ERROR_FILE_CORRUPTED); This might be paranoid, but I would factor the whole-file-reading zipcursor uses for reading in whole files into a helper function. Then add a NS_ASSERTION(len == readlen, "zipreader underflow") to guard against zlib and/or zipcursor changes of behavior that cause to not fill the whole buffer.

(In reply to comment #68) > Let's avoid URIs and nsIURI completely in this code. The reason why URIs are used so much in this patch is because they're already required. ImportInto receives strings representing URIs that need to be parsed into a URI. GlobalForLocation also requires a URI to be generated to use ReadScript/WriteScript/fastload. We can try avoiding URIs, but in the end, we'll end up having to create/recreate the jar URI like we currently do with loads from files. It could be done with a bit more complexity.

Remove unnecessary header include.

Comment on attachment 446424 [details] [diff] [review] 5. Point resource uris to the omnijar, v2.1 could you please diff with more context in the future? You should add a comment to both Init functions that the two need to be kept in sync.

http://hg.mozilla.org/mozilla-central/rev/171c1b972669 (patch 3a here) http://hg.mozilla.org/mozilla-central/rev/43f6e1ce1122 (patch 5 here)

It turns out using nsZipArchive doesn't work too well since we can't get a nsZipArchive easily in the ImportInto path.

Same patch as before, but with the new nsZipItemPtr api simplifying things.

Comment on attachment 448116 [details] [diff] [review] 6. Have the pref service look inside the omnijar, v3 s/pref_InitDefaults/pref_InitAppDefaults/ to make it clear that extension defaults are still being loaded.

http://hg.mozilla.org/mozilla-central/rev/ef864316356b (patch 6 here)

Comment on attachment 446371 [details] [diff] [review] 4. Point chrome uris to the omnijar, v4 Unfortunately we can't switch this to use nsZipItemPtr yet since the chrome manifest parser modifies its buffer. Will fix in a follow up.

Comment on attachment 446376 [details] [diff] [review] 8. Find xpts inside the omnijar, v2 The early registration in NS_InitXPCOM is important. Did you remove it because of some ordering issue? If so, please make the xptinfo code get the zipreader directly using C++ instead of via XPCOM. http://mxr.mozilla.org/mozilla-central/source/xpcom/reflect/xptinfo/src/xptiInterfaceInfoManager.cpp#493

Comment on attachment 448103 [details] [diff] [review] 9. Load js modules and components from the omnijar, v5 The hashtables still look completely wrong. You can't use nsUint32HashKey to hash on values that need a complex Equals. You either need to invent a key type which holds either a file or a file/path pair and does equality-testing properly, or you need to use nsIHashable and nsHashableHashKey

(In reply to comment #87) > (From update of attachment 448103 [details] [diff] [review]) > The hashtables still look completely wrong. You can't use nsUint32HashKey to > hash on values that need a complex Equals. You either need to invent a key type > which holds either a file or a file/path pair and does equality-testing > properly, or you need to use nsIHashable and nsHashableHashKey FileHash generates a hash based off "f" + canonical file path and JarHash generates a hash based off "j" + canonical file path + path within jar. Is there something I need to do differently here?

New xpt loader patch based the world in bug 570488. Much nicer as it just needs to add the omnijar check to xptiInterfaceInfoManager::AutoRegisterInterfaces.

Hashtable handling should be fixed.

Comment on attachment 449711 [details] [diff] [review] 9. Load js modules and components from the omnijar, v6 >diff --git a/xpcom/components/nsComponentManager.cpp b/xpcom/components/nsComponentManager.cpp >+ nsCOMPtr<nsILocalFile> fakeFile; >+ rv = NS_NewNativeLocalFile(Substring(compName, startIdx), PR_FALSE, getter_AddRefs(fakeFile)); >+ if (NS_FAILED(rv)) { >+ NS_ERROR("Failed to create fake local file for JS component"); >+ continue; >+ } This bit makes me nervous. My ideal solution here is to just pass NULL, instead of a fake file, and change XPCOMUtils so that it doesn't use the file parameter. But if you need to actually do this, you'll need to construct a path which works on Windows, I think. And ugh, this is going to be a merge pain with my other stuff, but I can deal. You'll have tests for all this somehow, right?

http://hg.mozilla.org/mozilla-central/rev/eb0883ccdb63 (patch 4 here) http://hg.mozilla.org/mozilla-central/rev/91851c94fb73 (patch 8 here)

As committed. http://hg.mozilla.org/mozilla-central/rev/c666507bf280

Will need to follow up with tests.