Last Comment Bug 552121 - (omnijar) Omnijar packaging
(omnijar)
: Omnijar packaging
Status: RESOLVED FIXED
[ts]
:
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: All All
: -- normal with 5 votes (vote)
: mozilla2.0b5
Assigned To: Michael Wu [:mwu]
:
:
Mentors:
: 553428 (view as bug list)
Depends on: 553121 553428 566686 570488 572445 573612 592411
Blocks: 406538 android 556644 556648 562406 601571
  Show dependency treegraph
 
Reported: 2010-03-12 17:16 PST by Michael Wu [:mwu]
Modified: 2014-10-28 12:42 PDT (History)
32 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
1. Add omnijar as a configure option (1.84 KB, patch)
2010-03-12 17:18 PST, Michael Wu [:mwu]
ted: review+
Details | Diff | Splinter Review
2. Generate omnijar (WIP) (2.05 KB, patch)
2010-03-12 17:19 PST, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
3. Allow the directory service to find the omnijar (4.02 KB, patch)
2010-03-12 17:21 PST, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
4. Point chrome uris to the omnijar (18.21 KB, patch)
2010-03-12 17:21 PST, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
5. Point resource uris to the omnijar (2.99 KB, patch)
2010-03-12 17:22 PST, Michael Wu [:mwu]
vladimir: review+
Details | Diff | Splinter Review
6. Have the pref service look inside the omnijar (2.99 KB, patch)
2010-03-12 17:23 PST, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
7. Have the expat driver look inside the omnijar for dtds (692 bytes, patch)
2010-03-12 17:24 PST, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
8. Find xpts inside the omnijar (2.56 KB, patch)
2010-03-12 17:24 PST, Michael Wu [:mwu]
vladimir: review+
Details | Diff | Splinter Review
9. Load js modules and components from the omnijar (27.86 KB, patch)
2010-03-12 17:26 PST, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
Bonus patch: trace files opened from jars (466 bytes, patch)
2010-03-12 17:27 PST, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
A program to reorder files in a zip file based on traces (9.46 KB, text/plain)
2010-03-12 17:32 PST, Michael Wu [:mwu]
no flags Details
3. Allow the directory service to find the omnijar, v2 (5.18 KB, patch)
2010-03-15 12:10 PDT, Michael Wu [:mwu]
benjamin: review-
vladimir: review+
Details | Diff | Splinter Review
4. Point chrome uris to the omnijar, v2 (17.38 KB, patch)
2010-03-15 12:11 PDT, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
2. Generate omnijar, v2 (WIP) (2.28 KB, patch)
2010-03-15 16:00 PDT, Michael Wu [:mwu]
ted: feedback-
Details | Diff | Splinter Review
4. Point chrome uris to the omnijar, v3 (15.53 KB, patch)
2010-03-15 16:01 PDT, Michael Wu [:mwu]
benjamin: review+
vladimir: review+
Details | Diff | Splinter Review
9. Load js modules and components from the omnijar (with -U 8 -p) (36.94 KB, patch)
2010-03-15 17:29 PDT, Michael Wu [:mwu]
vladimir: review+
Details | Diff | Splinter Review
9. Load js modules and components from the omnijar, v2 (with -U 12 -p) (42.82 KB, patch)
2010-04-01 15:37 PDT, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
9. Load js modules and components from the omnijar, v3 (with -U 12 -p) (28.91 KB, patch)
2010-04-01 15:58 PDT, Michael Wu [:mwu]
sayrer: review-
Details | Diff | Splinter Review
6. Have the pref service look inside the omnijar (with -U12 -p) (5.15 KB, patch)
2010-04-01 16:34 PDT, Michael Wu [:mwu]
benjamin: review+
Details | Diff | Splinter Review
5. Point resource uris to the omnijar (with -U 12 -p) (4.84 KB, patch)
2010-04-01 18:08 PDT, Michael Wu [:mwu]
benjamin: review+
cbiesinger: review+
Details | Diff | Splinter Review
3. Allow the directory service to find the omnijar, v3 (8.29 KB, patch)
2010-04-15 17:48 PDT, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
3. Allow the directory service to find the omnijar, v4 (15.99 KB, patch)
2010-04-20 17:41 PDT, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
3. Allow the directory service to find the omnijar, v4.1 (9.48 KB, patch)
2010-04-20 17:43 PDT, Michael Wu [:mwu]
benjamin: review-
Details | Diff | Splinter Review
3a. Allow users to find the omnijar, v5 (9.74 KB, patch)
2010-05-04 18:42 PDT, Michael Wu [:mwu]
benjamin: review+
Details | Diff | Splinter Review
3b. Add ExtractBuffer API for extracting files into a buffer (2.19 KB, patch)
2010-05-04 18:51 PDT, Michael Wu [:mwu]
taras.mozilla: review-
Details | Diff | Splinter Review
9. Load js modules and components from the omnijar, v4 (35.15 KB, patch)
2010-05-10 14:00 PDT, Michael Wu [:mwu]
benjamin: review-
Details | Diff | Splinter Review
3a. Allow users to find the omnijar, v6 (9.67 KB, patch)
2010-05-17 18:27 PDT, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
4. Point chrome uris to the omnijar, v4 (15.83 KB, patch)
2010-05-19 16:10 PDT, Michael Wu [:mwu]
benjamin: review+
Details | Diff | Splinter Review
5. Point resource uris to the omnijar, v2 (4.20 KB, patch)
2010-05-19 16:12 PDT, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
6. Have the pref service look inside the omnijar, v2 (2.61 KB, patch)
2010-05-19 16:13 PDT, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
8. Find xpts inside the omnijar, v2 (2.81 KB, patch)
2010-05-19 16:19 PDT, Michael Wu [:mwu]
benjamin: review-
Details | Diff | Splinter Review
5. Point resource uris to the omnijar, v2.1 (4.05 KB, patch)
2010-05-19 18:44 PDT, Michael Wu [:mwu]
cbiesinger: review+
Details | Diff | Splinter Review
9. Load js modules and components from the omnijar, v5 (30.44 KB, patch)
2010-05-28 14:43 PDT, Michael Wu [:mwu]
benjamin: review-
Details | Diff | Splinter Review
6. Have the pref service look inside the omnijar, v3 (2.32 KB, patch)
2010-05-28 15:16 PDT, Michael Wu [:mwu]
benjamin: review+
Details | Diff | Splinter Review
8. Find xpts inside the omnijar, v3 (846 bytes, patch)
2010-06-07 12:24 PDT, Michael Wu [:mwu]
benjamin: review+
Details | Diff | Splinter Review
9. Load js modules and components from the omnijar, v6 (40.06 KB, patch)
2010-06-07 13:54 PDT, Michael Wu [:mwu]
benjamin: review+
Details | Diff | Splinter Review
9. Load js modules and components from the omnijar, v7 (31.17 KB, patch)
2010-06-15 12:43 PDT, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
2. Generate omnijar, v3 (hack for testing) (2.65 KB, patch)
2010-06-24 14:16 PDT, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review

Description Michael Wu [:mwu] 2010-03-12 17:16:38 PST
Many of the files in the application/GRE directory can be placed into a single jar.
Comment 1 Michael Wu [:mwu] 2010-03-12 17:18:48 PST
Created attachment 432263 [details] [diff] [review]
1. Add omnijar as a configure option
Comment 2 Michael Wu [:mwu] 2010-03-12 17:19:42 PST
Created attachment 432264 [details] [diff] [review]
2. Generate omnijar (WIP)
Comment 3 Michael Wu [:mwu] 2010-03-12 17:21:10 PST
Created attachment 432265 [details] [diff] [review]
3. Allow the directory service to find the omnijar
Comment 4 Michael Wu [:mwu] 2010-03-12 17:21:58 PST
Created attachment 432266 [details] [diff] [review]
4. Point chrome uris to the omnijar
Comment 5 Michael Wu [:mwu] 2010-03-12 17:22:33 PST
Created attachment 432267 [details] [diff] [review]
5. Point resource uris to the omnijar
Comment 6 Michael Wu [:mwu] 2010-03-12 17:23:31 PST
Created attachment 432269 [details] [diff] [review]
6. Have the pref service look inside the omnijar
Comment 7 Michael Wu [:mwu] 2010-03-12 17:24:16 PST
Created attachment 432270 [details] [diff] [review]
7. Have the expat driver look inside the omnijar for dtds
Comment 8 Michael Wu [:mwu] 2010-03-12 17:24:58 PST
Created attachment 432272 [details] [diff] [review]
8. Find xpts inside the omnijar
Comment 9 Michael Wu [:mwu] 2010-03-12 17:26:00 PST
Created attachment 432273 [details] [diff] [review]
9. Load js modules and components from the omnijar
Comment 10 Michael Wu [:mwu] 2010-03-12 17:27:53 PST
Created attachment 432274 [details] [diff] [review]
Bonus patch: trace files opened from jars
Comment 11 Michael Wu [:mwu] 2010-03-12 17:32:35 PST
Created attachment 432276 [details]
A program to reorder files in a zip file based on traces
Comment 12 Michael Wu [:mwu] 2010-03-12 17:43:44 PST
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.
Comment 13 (dormant account) 2010-03-13 14:04:34 PST
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.
Comment 14 Michael Wu [:mwu] 2010-03-13 23:52:28 PST
(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..
Comment 15 Michael Wu [:mwu] 2010-03-15 12:10:02 PDT
Created attachment 432609 [details] [diff] [review]
3. Allow the directory service to find the omnijar, v2

Some bits in patch #4 should be in this patch.
Comment 16 Michael Wu [:mwu] 2010-03-15 12:11:07 PDT
Created attachment 432610 [details] [diff] [review]
4. Point chrome uris to the omnijar, v2
Comment 17 Michael Wu [:mwu] 2010-03-15 12:20:32 PDT
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 18 Michael Wu [:mwu] 2010-03-15 12:31:09 PDT
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.
Comment 19 Michael Wu [:mwu] 2010-03-15 16:00:46 PDT
Created attachment 432687 [details] [diff] [review]
2. Generate omnijar, v2 (WIP)

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 20 Michael Wu [:mwu] 2010-03-15 16:01:53 PDT
Created attachment 432688 [details] [diff] [review]
4. Point chrome uris to the omnijar, v3
Comment 21 Michael Wu [:mwu] 2010-03-15 17:29:08 PDT
Created attachment 432703 [details] [diff] [review]
9. Load js modules and components from the omnijar (with -U 8 -p)
Comment 22 Michael Wu [:mwu] 2010-03-16 18:29:30 PDT
Comment on attachment 432270 [details] [diff] [review]
7. Have the expat driver look inside the omnijar for dtds

Canceling review after discussion with taras
Comment 23 (dormant account) 2010-03-16 18:30:55 PDT
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 24 Michael Wu [:mwu] 2010-03-17 17:46:28 PDT
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 .
Comment 25 (dormant account) 2010-03-17 19:03:01 PDT
(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 26 Vladimir Vukicevic [:vlad] [:vladv] 2010-03-26 12:25:44 PDT
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?
Comment 27 Michael Wu [:mwu] 2010-03-26 12:55:27 PDT
(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 28 Ted Mielczarek [:ted.mielczarek] 2010-03-29 07:59:32 PDT
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 29 Ted Mielczarek [:ted.mielczarek] 2010-03-29 08:14:21 PDT
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?
Comment 30 Ted Mielczarek [:ted.mielczarek] 2010-03-29 08:16:03 PDT
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.
Comment 31 Vladimir Vukicevic [:vlad] [:vladv] 2010-04-01 13:50:39 PDT
(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 32 Vladimir Vukicevic [:vlad] [:vladv] 2010-04-01 13:58:29 PDT
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 33 Vladimir Vukicevic [:vlad] [:vladv] 2010-04-01 14:28:44 PDT
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 34 Vladimir Vukicevic [:vlad] [:vladv] 2010-04-01 14:29:41 PDT
Comment on attachment 432267 [details] [diff] [review]
5. Point resource uris to the omnijar

Straightforward, looks fine to me
Comment 35 Michael Wu [:mwu] 2010-04-01 14:50:12 PDT
(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 */
Comment 36 Michael Wu [:mwu] 2010-04-01 15:30:30 PDT
(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]
************************************************************
Comment 37 Michael Wu [:mwu] 2010-04-01 15:37:08 PDT
Created attachment 436577 [details] [diff] [review]
9. Load js modules and components from the omnijar, v2 (with -U 12 -p)

Vlad's review comments addressed
Comment 38 Michael Wu [:mwu] 2010-04-01 15:58:42 PDT
Created attachment 436580 [details] [diff] [review]
9. Load js modules and components from the omnijar, v3 (with -U 12 -p)

AutoRegisterJar probably shouldn't be defined if there is no implementation.
Comment 39 Michael Wu [:mwu] 2010-04-01 16:34:37 PDT
Created attachment 436594 [details] [diff] [review]
6. Have the pref service look inside the omnijar (with -U12 -p)

Same patch as before, with more context. The new approach attempted in bug 553428 doesn't work.
Comment 40 Michael Wu [:mwu] 2010-04-01 18:08:51 PDT
Created attachment 436621 [details] [diff] [review]
5. Point resource uris to the omnijar (with -U 12 -p)
Comment 41 Benjamin Smedberg [:bsmedberg] 2010-04-02 08:13:24 PDT
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 42 Benjamin Smedberg [:bsmedberg] 2010-04-02 10:22:36 PDT
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 ;-)
Comment 43 (dormant account) 2010-04-02 10:33:17 PDT
(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 44 Benjamin Smedberg [:bsmedberg] 2010-04-02 12:06:04 PDT
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 45 Robert Sayre 2010-04-02 12:10:27 PDT
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.
Comment 46 Michael Wu [:mwu] 2010-04-02 13:37:22 PDT
(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 47 Christian :Biesinger (don't email me, ping me on IRC) 2010-04-03 09:34:01 PDT
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.
Comment 48 Axel Hecht [:Pike] 2010-04-03 10:12:58 PDT
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 49 Benjamin Smedberg [:bsmedberg] 2010-04-05 13:08:01 PDT
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.
>
Comment 50 Michael Wu [:mwu] 2010-04-15 17:48:15 PDT
Created attachment 439415 [details] [diff] [review]
3. Allow the directory service to find the omnijar, v3

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.
Comment 51 Michael Wu [:mwu] 2010-04-15 17:57:25 PDT
(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.
Comment 52 Axel Hecht [:Pike] 2010-04-16 01:29:17 PDT
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 53 Benjamin Smedberg [:bsmedberg] 2010-04-19 08:07:51 PDT
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?
Comment 54 Michael Wu [:mwu] 2010-04-19 10:56:27 PDT
(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. :)
Comment 55 Michael Wu [:mwu] 2010-04-20 17:41:54 PDT
Created attachment 440389 [details] [diff] [review]
3. Allow the directory service to find the omnijar, v4

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.
Comment 56 Michael Wu [:mwu] 2010-04-20 17:43:24 PDT
Created attachment 440391 [details] [diff] [review]
3. Allow the directory service to find the omnijar, v4.1

Oops. Attached the wrong patch before.
Comment 57 Benjamin Smedberg [:bsmedberg] 2010-04-28 08:51:51 PDT
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?
Comment 58 Michael Wu [:mwu] 2010-04-28 10:48:34 PDT
(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.
Comment 59 Benjamin Smedberg [:bsmedberg] 2010-04-28 10:54:42 PDT
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.
Comment 60 (dormant account) 2010-04-28 11:01:12 PDT
(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.
Comment 61 Michael Wu [:mwu] 2010-04-28 11:05:14 PDT
(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.
Comment 62 (dormant account) 2010-04-28 11:08:18 PDT
(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.
Comment 63 Michael Wu [:mwu] 2010-05-04 18:42:05 PDT
Created attachment 443534 [details] [diff] [review]
3a. Allow users to find the omnijar, v5
Comment 64 Michael Wu [:mwu] 2010-05-04 18:51:55 PDT
Created attachment 443538 [details] [diff] [review]
3b. Add ExtractBuffer API for extracting files into a buffer
Comment 65 Michael Wu [:mwu] 2010-05-05 11:29:57 PDT
*** Bug 553428 has been marked as a duplicate of this bug. ***
Comment 66 Michael Wu [:mwu] 2010-05-10 14:00:19 PDT
Created attachment 444474 [details] [diff] [review]
9. Load js modules and components from the omnijar, v4

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 67 Benjamin Smedberg [:bsmedberg] 2010-05-14 08:49:30 PDT
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 68 Benjamin Smedberg [:bsmedberg] 2010-05-14 09:01:43 PDT
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.
Comment 69 Michael Wu [:mwu] 2010-05-17 18:27:59 PDT
Created attachment 445888 [details] [diff] [review]
3a. Allow users to find the omnijar, v6

All review comments addressed.
Comment 70 (dormant account) 2010-05-18 15:07:21 PDT
Comment on attachment 443538 [details] [diff] [review]
3b. Add ExtractBuffer API for extracting files into a buffer

working on this in bug 566686
Comment 71 Michael Wu [:mwu] 2010-05-19 13:24:32 PDT
http://hg.mozilla.org/mozilla-central/rev/cb427e0c0ed6 (first patch adding omnijar to configure)
Comment 72 Michael Wu [:mwu] 2010-05-19 16:10:58 PDT
Created attachment 446371 [details] [diff] [review]
4. Point chrome uris to the omnijar, v4

Updated to use the latest omnijar and nszipcursor apis.
Comment 73 Michael Wu [:mwu] 2010-05-19 16:12:05 PDT
Created attachment 446372 [details] [diff] [review]
5. Point resource uris to the omnijar, v2

Updated to use the new omnijar api.
Comment 74 Michael Wu [:mwu] 2010-05-19 16:13:41 PDT
Created attachment 446373 [details] [diff] [review]
6. Have the pref service look inside the omnijar, v2

Use new omnijar api.
Comment 75 Michael Wu [:mwu] 2010-05-19 16:19:36 PDT
Created attachment 446376 [details] [diff] [review]
8. Find xpts inside the omnijar, v2

Use latest omnijar api.
Comment 76 (dormant account) 2010-05-19 16:28:38 PDT
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.
Comment 77 Michael Wu [:mwu] 2010-05-19 18:04:32 PDT
(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.
Comment 78 Michael Wu [:mwu] 2010-05-19 18:44:07 PDT
Created attachment 446424 [details] [diff] [review]
5. Point resource uris to the omnijar, v2.1

Remove unnecessary header include.
Comment 79 Christian :Biesinger (don't email me, ping me on IRC) 2010-05-21 06:43:00 PDT
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.
Comment 81 Michael Wu [:mwu] 2010-05-28 14:43:47 PDT
Created attachment 448103 [details] [diff] [review]
9. Load js modules and components from the omnijar, v5

It turns out using nsZipArchive doesn't work too well since we can't get a nsZipArchive easily in the ImportInto path.
Comment 82 Michael Wu [:mwu] 2010-05-28 15:16:57 PDT
Created attachment 448116 [details] [diff] [review]
6. Have the pref service look inside the omnijar, v3

Same patch as before, but with the new nsZipItemPtr api simplifying things.
Comment 83 Benjamin Smedberg [:bsmedberg] 2010-06-01 11:39:36 PDT
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.
Comment 84 Michael Wu [:mwu] 2010-06-02 15:12:10 PDT
http://hg.mozilla.org/mozilla-central/rev/ef864316356b (patch 6 here)
Comment 85 Michael Wu [:mwu] 2010-06-03 18:29:26 PDT
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 86 Benjamin Smedberg [:bsmedberg] 2010-06-04 07:58:41 PDT
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 87 Benjamin Smedberg [:bsmedberg] 2010-06-04 08:10:56 PDT
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
Comment 88 Michael Wu [:mwu] 2010-06-04 17:21:45 PDT
(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?
Comment 89 Michael Wu [:mwu] 2010-06-07 12:24:00 PDT
Created attachment 449680 [details] [diff] [review]
8. Find xpts inside the omnijar, v3

New xpt loader patch based the world in bug 570488. Much nicer as it just needs to add the omnijar check to xptiInterfaceInfoManager::AutoRegisterInterfaces.
Comment 90 Michael Wu [:mwu] 2010-06-07 13:54:25 PDT
Created attachment 449711 [details] [diff] [review]
9. Load js modules and components from the omnijar, v6

Hashtable handling should be fixed.
Comment 91 Benjamin Smedberg [:bsmedberg] 2010-06-11 10:14:59 PDT
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?
Comment 93 Michael Wu [:mwu] 2010-06-15 12:43:07 PDT
Created attachment 451348 [details] [diff] [review]
9. Load js modules and components from the omnijar, v7

As committed.

http://hg.mozilla.org/mozilla-central/rev/c666507bf280
Comment 94 Michael Wu [:mwu] 2010-06-15 12:46:58 PDT
Will need to follow up with tests.
Comment 95 Michael Wu [:mwu] 2010-06-24 14:16:18 PDT
Created attachment 453849 [details] [diff] [review]
2. Generate omnijar, v3 (hack for testing)

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