Closed
Bug 620931
Opened 14 years ago
Closed 13 years ago
Omni.jar support for xulrunner
Categories
(Toolkit Graveyard :: XULRunner, defect)
Toolkit Graveyard
XULRunner
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla6
People
(Reporter: glandium, Assigned: glandium)
References
Details
Attachments
(10 files, 20 obsolete files)
(deleted),
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
mwu
:
review+
|
Details | Diff | Splinter Review |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
robert.strong.bugs
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
Currently, xulrunner has an half broken support for omni.jar. It's not that much of a problem for the moment as xulrunner itself still defaults to using jar chrome format, but it is a problem for ff-on-xr, because firefox still defaults to omnijar when building --with-libxul-sdk.
The idea here is to allow both xulrunner and the xul application to independently use omni.jar, supporting all setups:
- GRE omni.jar, APP jar or flat
- GRE jar or flat, APP omni.jar
- GRE jar or flat, APP jar or flat
- GRE omni.jar, APP omni.jar
We, obviously, also need to keep the GRE == APP case working, which is what you get with Firefox and most applications distributed by Mozilla.
Assignee | ||
Comment 1•14 years ago
|
||
The current way resource://gre-resources/ is registered is not very helpful for omni.jar rework. On top of that, it currently depends on the chrome file format used at build time, such that switching from, e.g. flat to jar chrome breaks resource://gre-resources/ addresses.
OTOH, the resource keyword in chrome manifests allow to register the substitution. Someone switching from jar to flat chrome (and that happened to me quite often) would have to edit the manifest anyways.
Assignee: nobody → mh+mozilla
Attachment #499276 -
Flags: review?(benjamin)
Assignee | ||
Comment 2•14 years ago
|
||
When building ff-on-xr with omni.jar, channel-prefs.js ends up in the omni.jar, because xul apps use defaults/preferences instead of defaults/pref. firefox-l10n.js is also not created at the proper location.
Attachment #499278 -
Flags: review?(benjamin)
Assignee | ||
Comment 3•14 years ago
|
||
With this patch, omni.jar support is always on, but the chrome may or may not be packed in an omni.jar. Support for separated GRE and XUL application directories is also in there, each of which can use omni.jar or not. Tested in all possible scenarios.
Attachment #499280 -
Flags: review?(benjamin)
Assignee | ||
Updated•14 years ago
|
Attachment #499280 -
Flags: review?(mwu)
Assignee | ||
Comment 4•14 years ago
|
||
This is not directly related to this bug, but the breakage being related to omni.jar, and the fix being made easier by part 3...
Attachment #499281 -
Flags: review?(benjamin)
Assignee | ||
Comment 5•14 years ago
|
||
Now that xulrunner properly supports omni.jar with the previous patches, the chrome format can be switched to omni by default.
Attachment #499282 -
Flags: review?(benjamin)
Assignee | ||
Comment 6•14 years ago
|
||
(In reply to comment #3)
> Created attachment 499280 [details] [diff] [review]
> part 3 - Allow GRE and XUL application to use omni.jar independently
>
> With this patch, omni.jar support is always on, but the chrome may or may not
> be packed in an omni.jar. Support for separated GRE and XUL application
> directories is also in there, each of which can use omni.jar or not. Tested in
> all possible scenarios.
Note that maybe a diff -w would help review, here. Please tell me if you want one.
Updated•14 years ago
|
Attachment #499276 -
Flags: review?(benjamin) → review+
Updated•14 years ago
|
Attachment #499278 -
Flags: review?(benjamin) → review+
Comment 7•14 years ago
|
||
Comment on attachment 499280 [details] [diff] [review]
part 3 - Allow GRE and XUL application to use omni.jar independently
Could you please remake this patch with a much more detailed (multi-paragaph) commit message explaning what is changing? I see Omnijar.h now holds two omnijars, but I'm a bit confused about why, in the unified case, we have the same path/nsZipArchive in both slots. Wouldn't it be better to leave the APP slot empty in that case? Otherwise don't we run the risk of opening the manifests/prefs/etc from the same omnijar twice?
Attachment #499280 -
Flags: review?(benjamin) → review-
Updated•14 years ago
|
Attachment #499281 -
Flags: review?(benjamin) → review+
Updated•14 years ago
|
Attachment #499282 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 8•14 years ago
|
||
(In reply to comment #7)
> Comment on attachment 499280 [details] [diff] [review]
> part 3 - Allow GRE and XUL application to use omni.jar independently
>
> Could you please remake this patch with a much more detailed (multi-paragaph)
> commit message explaning what is changing? I see Omnijar.h now holds two
> omnijars, but I'm a bit confused about why, in the unified case, we have the
> same path/nsZipArchive in both slots. Wouldn't it be better to leave the APP
> slot empty in that case?
Now that you mention it, there are not much reasons for that, except maybe that I went through many iterations. Leaving the APP slot empty would actually make it more comfortable for callers, though it would make GetBase inconsistent with GetURI, API wise. However, this shouldn't be a problem.
> Otherwise don't we run the risk of opening the
> manifests/prefs/etc from the same omnijar twice?
In the current implementation not really, because the callers where it matters are checking that GRE and APP point to different things (and the omnijar API ensures the same nsIFile pointer is returned for both in the unified case).
Assignee | ||
Comment 9•14 years ago
|
||
Rewrote the patch considering your comments, which helped making some things simpler. Also added a longer commit message as suggested, though I'm not sure it's clear enough.
Attachment #501665 -
Flags: review?(benjamin)
Assignee | ||
Updated•14 years ago
|
Attachment #499280 -
Attachment is obsolete: true
Attachment #499280 -
Flags: review?(mwu)
Assignee | ||
Updated•14 years ago
|
Attachment #501665 -
Flags: review?(mwu)
Comment 10•14 years ago
|
||
Comment on attachment 501665 [details] [diff] [review]
part 3 - Allow GRE and XUL application to use omni.jar independently
Omnijar::GetURI returns a nsCOMPtr, which is unusual and will confuse people. Please return an already_AddRefed. Other than that, this look ok. But boy, this seems like a big change to be taking now :-(
Attachment #501665 -
Flags: review?(benjamin) → review-
Assignee | ||
Comment 11•14 years ago
|
||
(In reply to comment #10)
> But boy, this seems like a big change to be taking now :-(
Even worse, it needs some modifications, now that bug 609785 landed :(
Assignee | ||
Comment 12•14 years ago
|
||
(In reply to comment #10)
> Omnijar::GetURI returns a nsCOMPtr, which is unusual and will confuse people.
> Please return an already_AddRefed.
(Note that Omnijar::GetBase and Omnijar::GetPath do, too)
Comment 13•14 years ago
|
||
Comment on attachment 501665 [details] [diff] [review]
part 3 - Allow GRE and XUL application to use omni.jar independently
Looks fine overall, as long as we eliminate returning nsCOMPtrs as bsmedberg mentioned. I also tend towards using bool instead of PRBool whenever we aren't passing bool to something that wants PRBool, but that's a minor thing people disagree on.
Is this compatible with non-libxul? I dunno if disabling libxul still works, but it would be good to know if this breaks it if it still does.
Attachment #501665 -
Flags: review?(mwu) → review+
Assignee | ||
Comment 14•14 years ago
|
||
(In reply to comment #13)
> Is this compatible with non-libxul? I dunno if disabling libxul still works,
> but it would be good to know if this breaks it if it still does.
It should be, but I can certainly actually check.
Assignee | ||
Comment 15•14 years ago
|
||
What changed since previous iteration:
- Changed nsCOMPtr return values to already_AddRefed as suggested in review comments and modified some other parts accordingly.
- Added some necessary bits to avoid breakage of --disable-libxul builds
- Adapted startupcache/StartupCache.cpp to the new API
- Changed how URIs are canonicalized in js/src/xpconnect/loader/mozJSComponentLoader.cpp (which is where I'd like Taras' feedback)
Attachment #501665 -
Attachment is obsolete: true
Attachment #504863 -
Flags: review?(benjamin)
Attachment #504863 -
Flags: feedback?(tglek)
Assignee | ||
Comment 16•14 years ago
|
||
This is the same patch, but with context changes to due part 3 having changed. Carrying over r+.
Attachment #499281 -
Attachment is obsolete: true
Attachment #504865 -
Flags: review+
Assignee | ||
Comment 17•14 years ago
|
||
(In reply to comment #15)
> Created attachment 504863 [details] [diff] [review]
> part 3 - Allow GRE and XUL application to use omni.jar independently
>
> What changed since previous iteration:
> - Changed nsCOMPtr return values to already_AddRefed as suggested in review
> comments and modified some other parts accordingly.
> - Added some necessary bits to avoid breakage of --disable-libxul builds
> - Adapted startupcache/StartupCache.cpp to the new API
> - Changed how URIs are canonicalized in
> js/src/xpconnect/loader/mozJSComponentLoader.cpp (which is where I'd like
> Taras' feedback)
FWIW, I tested the patch in all omni/jar combinations for ff-on-xr, as well as omni and jar for ff and jar for ff --disable-libxul (all on linux x64). I'm also pushing to try right now.
Comment 18•14 years ago
|
||
Comment on attachment 504863 [details] [diff] [review]
part 3 - Allow GRE and XUL application to use omni.jar independently
This code is a bit more complex than what it replaces, but I don't see a simpler way of accomplishing this. My only [minor] concern is that canonicalizeBase will be called twice for APP uris.
Attachment #504863 -
Flags: feedback?(tglek) → feedback+
Assignee | ||
Comment 19•14 years ago
|
||
Comment on attachment 504863 [details] [diff] [review]
part 3 - Allow GRE and XUL application to use omni.jar independently
I'll address some comments Taras had over irc.
Attachment #504863 -
Flags: review?(benjamin)
Comment 20•14 years ago
|
||
Comment on attachment 504863 [details] [diff] [review]
part 3 - Allow GRE and XUL application to use omni.jar independently
feedback+ was for the startupcache bits.
feedback- is for Omnijar::GetURI. We agreed on irc to get rid of nsIURI creation(because those are slow) in Omnijar::GetURI(and the buried stat() call in it)
Attachment #504863 -
Flags: feedback+ → feedback-
Assignee | ||
Comment 21•14 years ago
|
||
Changes since last iteration:
- Replaced Omnijar::GetURI with Omnijar::GetURIString, that returns a string instead of nsIURI, and removed the nsIFile::IsFile use. Also made it only return a value for Omnijar::APP in the non-unified case, like other functions in the API.
- Updated callers accordingly.
Attachment #504863 -
Attachment is obsolete: true
Attachment #505014 -
Flags: review?(benjamin)
Assignee | ||
Comment 22•14 years ago
|
||
Oops, the previous one was the wrong revision
Attachment #505014 -
Attachment is obsolete: true
Attachment #505016 -
Flags: review?(benjamin)
Attachment #505014 -
Flags: review?(benjamin)
Assignee | ||
Comment 23•14 years ago
|
||
Again, refresh with context changes due to part 3 changes. Carrying over r+
Attachment #504865 -
Attachment is obsolete: true
Attachment #505017 -
Flags: review+
Assignee | ||
Comment 24•14 years ago
|
||
Note that the prefs change allows administrators to put custom prefs in defaults/prefs, possibility that omni.jar removed since the only file read there is, without part 3, only channel-prefs.js.
Updated•14 years ago
|
Attachment #505016 -
Flags: review?(benjamin) → review+
Comment 25•14 years ago
|
||
approval2.0 or post2.0?
Assignee | ||
Comment 26•14 years ago
|
||
Updated now that bug 595522 landed. The nsPrefService.cpp code resulting from the application of this version of the patch is the same as the nsPrefService.cpp code resulting from the application of the previous version against the tree before bug 595522 landed. It so happens that the patch from bug 595522 actually does a part of what the patch here was doing, and thus the new patch version here ends up more readable.
Carrying over r+
Attachment #505016 -
Attachment is obsolete: true
Attachment #510720 -
Flags: review+
Assignee | ||
Comment 27•14 years ago
|
||
Unbitrot part 3 (after bug 633666). Carrying over r+, the resulting code doesn't change.
Attachment #510720 -
Attachment is obsolete: true
Attachment #515039 -
Flags: review+
Assignee | ||
Comment 28•14 years ago
|
||
Unbitrot part 2, because of a small context change in toolkit/mozapps/installer/packager.mk from bug 562406. Carrying over r+.
Attachment #499278 -
Attachment is obsolete: true
Attachment #515040 -
Flags: review+
Comment 29•14 years ago
|
||
any hope that this will be fixed in 4.0.1?
Assignee | ||
Comment 30•14 years ago
|
||
My take is that this will not land on 4.0.x, but can be applied by linux distros who see fit.
Assignee | ||
Comment 31•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/f406ffe65c08
http://hg.mozilla.org/mozilla-central/rev/5c3ed4fde1e4
http://hg.mozilla.org/mozilla-central/rev/b9e6454362ef
http://hg.mozilla.org/mozilla-central/rev/9df6e8117fe0
http://hg.mozilla.org/mozilla-central/rev/b97a060746f9
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 32•14 years ago
|
||
Backed out as the likely cause of bug 644790 (along with the two other bugs it landed which, which are much less likely to be the cause):
http://hg.mozilla.org/mozilla-central/rev/1492b6e75639
http://hg.mozilla.org/mozilla-central/rev/941d126f6dd2
http://hg.mozilla.org/mozilla-central/rev/08d1aeeea824
http://hg.mozilla.org/mozilla-central/rev/16e48d6b3b9c
http://hg.mozilla.org/mozilla-central/rev/5deb267b1d33
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 33•14 years ago
|
||
This applies on top of part 3 to fix Android, and also gets rid of the MOZ_ENABLE_LIBXUL ifdefs that are now useless.
try build here (successfully tested on nexus s):
http://stage.mozilla.org/pub/mozilla.org/mobile/tryserver-builds/mh@glandium.org-34062755c477/try-mb-br-andrd-r7-bld/fennec-4.1a1pre.en-US.eabi-arm.apk
Attachment #521834 -
Flags: review?(mwu)
Comment 35•14 years ago
|
||
Ok, I think I've reviewed this for real this time, and there's two issues I'd like to see addressed:
1. The omnijar code is basically storing the omnijar and/or the gre/app dir. I would prefer it to only store the omnijar when there is a valid omnijar. That means the base uri code should query the directory enumeration service when there is no valid omnijar. I'm trying to avoid having information about the gre/app directory being stored twice in two different places.
2. For a given file that's potentially an omnijar, the code should only attempt to create a zip archive from it once. If it fails, we should drop the file and use the app/gre dir as necessary.
Assignee | ||
Comment 36•14 years ago
|
||
(In reply to comment #35)
> Ok, I think I've reviewed this for real this time, and there's two issues I'd
> like to see addressed:
>
> 1. The omnijar code is basically storing the omnijar and/or the gre/app dir. I
> would prefer it to only store the omnijar when there is a valid omnijar. That
> means the base uri code should query the directory enumeration service when
> there is no valid omnijar. I'm trying to avoid having information about the
> gre/app directory being stored twice in two different places.
Well then, why not make it store nothing at all for the common case where the file is named omni.jar and is in the corresponding base directory?
> 2. For a given file that's potentially an omnijar, the code should only attempt
> to create a zip archive from it once. If it fails, we should drop the file and
> use the app/gre dir as necessary.
It does. Except if the file is opened from another place than GetReader.
Comment 37•14 years ago
|
||
(In reply to comment #36)
> (In reply to comment #35)
> > Ok, I think I've reviewed this for real this time, and there's two issues I'd
> > like to see addressed:
> >
> > 1. The omnijar code is basically storing the omnijar and/or the gre/app dir. I
> > would prefer it to only store the omnijar when there is a valid omnijar. That
> > means the base uri code should query the directory enumeration service when
> > there is no valid omnijar. I'm trying to avoid having information about the
> > gre/app directory being stored twice in two different places.
>
> Well then, why not make it store nothing at all for the common case where the
> file is named omni.jar and is in the corresponding base directory?
>
We need an easy way to access the file since the zipreader doesn't give us that. We need to access the file in order to pass the path to child processes. Whether we need to pass the path to the child process in the common case.. is debatable. But, wouldn't keeping the file around whenever there's a valid omnijar keep things simpler?
> > 2. For a given file that's potentially an omnijar, the code should only attempt
> > to create a zip archive from it once. If it fails, we should drop the file and
> > use the app/gre dir as necessary.
>
> It does. Except if the file is opened from another place than GetReader.
Yup. The idea is to never return a file unless it's ziparchive can open it, which is how the code was before. It ensured the ziparchive is successfully created before returning the path, which is why we had a separate function for setting up the zip archive.
Updated•14 years ago
|
Whiteboard: [not-ready-for-cedar]
Assignee | ||
Comment 38•13 years ago
|
||
Refreshed context. Carrying r+ over.
Attachment #515040 -
Attachment is obsolete: true
Attachment #530570 -
Flags: review+
Assignee | ||
Comment 39•13 years ago
|
||
This changes the Omnijar API according to the last comments, and (obviously) adapts the callers accordingly. It also changes the command line arguments names and handles Android.
Attachment #515039 -
Attachment is obsolete: true
Attachment #521834 -
Attachment is obsolete: true
Attachment #530571 -
Flags: review?(mwu)
Attachment #521834 -
Flags: review?(mwu)
Assignee | ||
Comment 40•13 years ago
|
||
Refreshed context.
Attachment #499276 -
Attachment is obsolete: true
Attachment #530572 -
Flags: review+
Assignee | ||
Comment 41•13 years ago
|
||
Cleaned up patch description
Attachment #530570 -
Attachment is obsolete: true
Attachment #530573 -
Flags: review+
Assignee | ||
Comment 42•13 years ago
|
||
This add an option to allow to use an application directory different from
the directory containing xpcshell.
While I do need this particular change for ff-on-xr setups, the particular
issue that makes it necessary for this bug (the GENERATE_CACHE command in
browser/installer/Makefile.in, related patch coming soon) could be fixed by
making the already existing -g option set the application directory as well
as the GRE directory.
Attachment #530575 -
Flags: review?(benjamin)
Assignee | ||
Comment 43•13 years ago
|
||
This uses the change from part 6 to avoid xpcshell considering
resource://app is not at the same place as resource://gre, leading
to wrongly filled startup cache.
It also simplifies the startup cache precompile script taking,
taking advantage of the new path canonicalization in the startup
cache code from part 3.
Attachment #530580 -
Flags: review?(mwu)
Assignee | ||
Comment 44•13 years ago
|
||
This should help review of part 3. Most changes outside Omnijar.{cpp,h} are straightforward. On the other hand Omnijar.{cpp,h} are more easily read in part 3 directly.
Assignee | ||
Comment 45•13 years ago
|
||
The previous interdiff was weird.
Attachment #530584 -
Attachment is obsolete: true
Updated•13 years ago
|
Attachment #530575 -
Flags: review?(benjamin) → review+
Comment 46•13 years ago
|
||
Comment on attachment 530571 [details] [diff] [review]
part 3 - Allow GRE and XUL application to use omni.jar independently
Review of attachment 530571 [details] [diff] [review]:
-----------------------------------------------------------------
This looks pretty good overall. My review is mostly nits. The PathifyURI stuff is a lot better and I'd love to get that in soon. I noticed you annotated a bunch of functions with inline - generally I avoid adding inline and with PGO, hopefully gcc won't need it. But it doesn't matter much either way.
::: toolkit/xre/nsAppRunner.cpp
@@ +3837,5 @@
> return rv;
>
> + nsCOMPtr<nsILocalFile> greOmni;
> + rv = XRE_GetFileFromPath(path, getter_AddRefs(greOmni));
> + if (NS_FAILED(rv))
Might be nice to notify the user that they entered an invalid path.
::: xpcom/build/Omnijar.cpp
@@ +51,5 @@
> +nsZipArchive *Omnijar::sReader[2] = { nsnull, nsnull };
> +PRPackedBool Omnijar::sInitialized = PR_FALSE;
> +static PRPackedBool sIsUnified = PR_FALSE;
> +
> +static const char sProp[2][PR_MAX(sizeof(NS_GRE_DIR),sizeof(NS_XPCOM_CURRENT_PROCESS_DIR)) + 1] =
Can we do something like static const char *sProp[] instead? I suppose doing it like this lets us avoid a memory access, though gcc compiles it all away if you use constant indexes. Wonder if it's smart enough to do the same if you do !! to variable before using it as an index.
@@ +63,5 @@
> + delete sReader[aType];
> + }
> + sReader[aType] = nsnull;
> + NS_IF_RELEASE(sPath[aType]);
> + sPath[aType] = nsnull;
NS_IF_RELEASE should clear the pointer for you.
@@ +74,5 @@
> + if (aPath) {
> + file = aPath;
> + } else {
> + nsCOMPtr<nsIFile> dir;
> + nsDirectoryService::gService->Get(sProp[aType], NS_GET_IID(nsIFile), getter_AddRefs(dir));
There is a NS_GetSpecialDirectory though I guess this is a lot more efficient to do. Wonder if NS_GetSpecialDirectory could be optimized to one line like this.
@@ +95,4 @@
> return;
> }
>
> + if ((aType == APP) && (sPath[GRE])) {
Could unify this all into one "if" broken into multiple lines.
@@ +157,5 @@
> + return nsnull;
> +}
> +
> +nsresult
> +Omnijar::GetURIString(Type aType, nsCString &result)
Hm, I think we're suppose to use abstract strings for APIs?
@@ +161,5 @@
> +Omnijar::GetURIString(Type aType, nsCString &result)
> +{
> + NS_ABORT_IF_FALSE(IsInitialized(), "Omnijar not initialized");
> +
> + result = "";
result.Truncate();
::: xpcom/build/Omnijar.h
@@ +40,5 @@
> #ifndef mozilla_Omnijar_h
> #define mozilla_Omnijar_h
>
> +#include "nscore.h"
> +#include "nsTArray.h"
What's nsTArray for?
Attachment #530571 -
Flags: review?(mwu) → review+
Comment 47•13 years ago
|
||
Comment on attachment 530580 [details] [diff] [review]
part 7 - Make startup cache generation work better with new omni.jar handling
Nice!
Attachment #530580 -
Flags: review?(mwu) → review+
Assignee | ||
Comment 48•13 years ago
|
||
> ::: xpcom/build/Omnijar.cpp
> @@ +51,5 @@
> > +nsZipArchive *Omnijar::sReader[2] = { nsnull, nsnull };
> > +PRPackedBool Omnijar::sInitialized = PR_FALSE;
> > +static PRPackedBool sIsUnified = PR_FALSE;
> > +
> > +static const char sProp[2][PR_MAX(sizeof(NS_GRE_DIR),sizeof(NS_XPCOM_CURRENT_PROCESS_DIR)) + 1] =
>
> Can we do something like static const char *sProp[] instead? I suppose doing
> it like this lets us avoid a memory access, though gcc compiles it all away
> if you use constant indexes. Wonder if it's smart enough to do the same if
> you do !! to variable before using it as an index.
It's apparently not. aType == GRE ? sProp[GRE] : sProp[APP] works, though.
> @@ +63,5 @@
> > + delete sReader[aType];
> > + }
> > + sReader[aType] = nsnull;
> > + NS_IF_RELEASE(sPath[aType]);
> > + sPath[aType] = nsnull;
>
> NS_IF_RELEASE should clear the pointer for you.
>
> @@ +74,5 @@
> > + if (aPath) {
> > + file = aPath;
> > + } else {
> > + nsCOMPtr<nsIFile> dir;
> > + nsDirectoryService::gService->Get(sProp[aType], NS_GET_IID(nsIFile), getter_AddRefs(dir));
>
> There is a NS_GetSpecialDirectory though I guess this is a lot more
> efficient to do. Wonder if NS_GetSpecialDirectory could be optimized to one
> line like this.
Mmmmm not sure if it could, it could be used in xpcom components, and that would likely fail.
> Hm, I think we're suppose to use abstract strings for APIs?
Bsmedberg didn't mention it when he review an earlier version, but that'd make sense.
> ::: xpcom/build/Omnijar.h
> @@ +40,5 @@
> > #ifndef mozilla_Omnijar_h
> > #define mozilla_Omnijar_h
> >
> > +#include "nscore.h"
> > +#include "nsTArray.h"
>
> What's nsTArray for?
Looks like a leftover of an early iteration.
Thanks for the review. I'll address these shortly.
Assignee | ||
Comment 49•13 years ago
|
||
with review comments addressed
Attachment #530894 -
Flags: review?(mwu)
Assignee | ||
Updated•13 years ago
|
Attachment #530571 -
Attachment is obsolete: true
Assignee | ||
Comment 50•13 years ago
|
||
Comment on attachment 530894 [details] [diff] [review]
part 3 - Allow GRE and XUL application to use omni.jar independently. now store two independent locations for an omni.jar, allowing GRE/XRE and
This actually leads to this M-oth error:
2244 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/src/threads/test/test_chromeWorkerJSM.xul | Worker had an error: Script file not found: WorkerTest_worker.js
Attachment #530894 -
Flags: review?(mwu)
Assignee | ||
Comment 51•13 years ago
|
||
The WorkerTest_worker.js problem was coming from the startupcache storing the
full path of the file during packaging, making the script filename wrong at
run-time, thus breaking relative loading (cf. nsDOMWorker.cpp:1664)
Note I used resource:/// instead of resource://gre/ because resource:/// is
also going to work in the ff-on-xr case. We'd need a similar script for
xulrunner, though, but that should be a separate bug.
There could be bad surprises with other things using JS_GetScriptFilename in
the future, so maybe the component loader should fixup the js script object
to match the filename under which the component was loaded at runtime instead
of that that was used when creating the cache. I'll file a separate bug.
Attachment #531004 -
Flags: review?(mwu)
Assignee | ||
Updated•13 years ago
|
Attachment #530894 -
Flags: review?(mwu)
Assignee | ||
Updated•13 years ago
|
Attachment #530580 -
Attachment is obsolete: true
Assignee | ||
Comment 52•13 years ago
|
||
Updated•13 years ago
|
Attachment #531004 -
Flags: review?(mwu) → review+
Updated•13 years ago
|
Attachment #530894 -
Flags: review?(mwu) → review+
Assignee | ||
Comment 53•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/ddc20b6faef0
http://hg.mozilla.org/mozilla-central/rev/aad5d018120e
http://hg.mozilla.org/mozilla-central/rev/4820d39b86d7
http://hg.mozilla.org/mozilla-central/rev/56cad749a41c
http://hg.mozilla.org/mozilla-central/rev/581ddeda2222
http://hg.mozilla.org/mozilla-central/rev/2a3dd268ceb3
http://hg.mozilla.org/mozilla-central/rev/83ca7e971857
Status: REOPENED → RESOLVED
Closed: 14 years ago → 13 years ago
Resolution: --- → FIXED
Whiteboard: [not-ready-for-cedar]
Target Milestone: --- → mozilla6
Comment 54•13 years ago
|
||
Regression on this bug fix. Causing sites to hang.
Examples:
http://wikidrivers.com/wiki/Main_Page
http://www.weather.com/
Good: mozilla-central: changeset 69244:8d378453a8ac
Bad:mozilla-central: changeset 69251:83ca7e971857
Assignee | ||
Comment 55•13 years ago
|
||
(In reply to comment #54)
> Regression on this bug fix. Causing sites to hang.
>
> Examples:
>
> http://wikidrivers.com/wiki/Main_Page
> http://www.weather.com/
>
> Good: mozilla-central: changeset 69244:8d378453a8ac
> Bad:mozilla-central: changeset 69251:83ca7e971857
That doesn't make much sense. What platform?
Comment 56•13 years ago
|
||
(In reply to comment #55)
> (In reply to comment #54)
> > Regression on this bug fix. Causing sites to hang.
> >
> > Examples:
> >
> > http://wikidrivers.com/wiki/Main_Page
> > http://www.weather.com/
> >
> > Good: mozilla-central: changeset 69244:8d378453a8ac
> > Bad:mozilla-central: changeset 69251:83ca7e971857
>
> That doesn't make much sense. What platform?
Windows 7
Comment 57•13 years ago
|
||
Both sites in comment 54 WFM with Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110510 Firefox/6.0a1; D2D.
Comment 58•13 years ago
|
||
I created a new profile and the two sites I gave as an example still hang. I get a not 'responding message' in the caption bar. I'm currently on the 'bad' build
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110510 Firefox/6.0a1 - Build ID: 20110510065441
Comment 59•13 years ago
|
||
(In reply to comment #54)
> Regression on this bug fix. Causing sites to hang.
>
> Examples:
>
> http://wikidrivers.com/wiki/Main_Page
> http://www.weather.com/
>
> Good: mozilla-central: changeset 69244:8d378453a8ac
> Bad:mozilla-central: changeset 69251:83ca7e971857
I can see huge performance regression.
It takes more than 30-60sec to loading site after cset 83ca7e971857.
Comment 60•13 years ago
|
||
(In reply to comment #59)
> (In reply to comment #54)
> > Regression on this bug fix. Causing sites to hang.
> >
> > Examples:
> >
> > http://wikidrivers.com/wiki/Main_Page
> > http://www.weather.com/
> >
> > Good: mozilla-central: changeset 69244:8d378453a8ac
> > Bad:mozilla-central: changeset 69251:83ca7e971857
>
> I can see huge performance regression.
> It takes more than 30-60sec to loading site after cset 83ca7e971857.
I'm seeing slow performance even before this fix, but no hangs.
Comment 61•13 years ago
|
||
(In reply to comment #59)
> (In reply to comment #54)
> > Regression on this bug fix. Causing sites to hang.
> >
> > Examples:
> >
> > http://wikidrivers.com/wiki/Main_Page
> > http://www.weather.com/
> >
> > Good: mozilla-central: changeset 69244:8d378453a8ac
> > Bad:mozilla-central: changeset 69251:83ca7e971857
>
> I can see huge performance regression.
> It takes more than 30-60sec to loading site after cset 83ca7e971857.
If I click menubar/scroll with mouse wheel just after loading the page,
then Browser UI hangs for a while.
Comment 62•13 years ago
|
||
In addition to Comment #61.
If disabled Flash Plugins , the performance regression does not happen.
Shockwave Flash
File: NPSWF32.dll
Version: 10.2.159.1
Shockwave Flash 10.2 r159
http://hg.mozilla.org/mozilla-central/rev/83ca7e971857
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110510 Firefox/6.0a1 ID:20110510065441
Comment 63•13 years ago
|
||
My WFM was using Flash 10.3.180.42, for reference.
Comment 64•13 years ago
|
||
(In reply to comment #62)
> In addition to Comment #61.
> If disabled Flash Plugins , the performance regression does not happen.
>
> Shockwave Flash
> File: NPSWF32.dll
> Version: 10.2.159.1
> Shockwave Flash 10.2 r159
> http://hg.mozilla.org/mozilla-central/rev/83ca7e971857
> Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110510 Firefox/6.0a1
> ID:20110510065441
Alice, does it behave normally if you disable OOPP ?
Comment 65•13 years ago
|
||
(In reply to comment #64)
> (In reply to comment #62)
> > In addition to Comment #61.
> > If disabled Flash Plugins , the performance regression does not happen.
> >
> > Shockwave Flash
> > File: NPSWF32.dll
> > Version: 10.2.159.1
> > Shockwave Flash 10.2 r159
> > http://hg.mozilla.org/mozilla-central/rev/83ca7e971857
> > Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110510 Firefox/6.0a1
> > ID:20110510065441
>
> Alice, does it behave normally if you disable OOPP ?
Yes,
dom.ipc.plugins.enabled;false helps.
Comment 66•13 years ago
|
||
Comment 67•13 years ago
|
||
IMO, this needs to be backed out or we are going have broken nightly's tomorrow.
Comment 68•13 years ago
|
||
No hangs when I disable dom.ipc.plugins.enabled
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110510 Firefox/6.0a1 - Build ID: 20110510124755
Comment 69•13 years ago
|
||
Filed bug [url=https://bugzilla.mozilla.org/show_bug.cgi?id=656172]Bug 656172 – Regression on Bug 620931 causing sites to hang.[/url]
Comment 70•13 years ago
|
||
Sorry - backed out parts 3-7 since we know this is broken for some users.
http://hg.mozilla.org/mozilla-central/rev/2ab5c696884e
http://hg.mozilla.org/mozilla-central/rev/f442ce8fb412
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 71•13 years ago
|
||
Attachment #531553 -
Flags: review?(mwu)
Comment 72•13 years ago
|
||
Comment on attachment 531553 [details] [diff] [review]
Possible Fixup
[01:37:10] <mwu> I'm wondering if it has something to do with the path.. are we escaping and passing paths to the child correctly?
[01:37:45] <mwu> but this is a new bug and you're passing it the same way..
[01:38:11] <glandium> yeah this is what bugs me. there shouldn't be a difference
[01:38:24] <mwu> oh wait, the old way didn't use XRE_GetFileFromPath
[01:38:36] <mwu> it just directly did a NS_NewNativeLocalFile on the path
[01:40:54] <mwu> and XRE_GetFileFromPath has a fixed utf8->utf16 conversion
[01:41:00] <glandium> oh that would make sense
[01:41:05] <mwu> hmm
[01:42:56] <mwu> ok yeah I see an issue
[01:43:29] <mwu> so the parent converts from wide char to native narrow char, and the child converts from utf8 to utf16
Attachment #531553 -
Flags: review?(mwu) → review+
Assignee | ||
Comment 73•13 years ago
|
||
Can people seeing these freezes test these builds:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mh@glandium.org-3d08dc03ef77/try-win32/
Thanks
Comment 74•13 years ago
|
||
(In reply to comment #73)
> Can people seeing these freezes test these builds:
> https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mh@glandium.org-
> 3d08dc03ef77/try-win32/
>
> Thanks
Works fine. no problem.
Assignee | ||
Comment 75•13 years ago
|
||
(In reply to comment #74)
> (In reply to comment #73)
> > Can people seeing these freezes test these builds:
> > https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mh@glandium.org-
> > 3d08dc03ef77/try-win32/
> >
> > Thanks
>
> Works fine. no problem.
Just to be extra careful, can you verify when putting the unzipped build in the same directory the failing build was under? Thanks.
Comment 76•13 years ago
|
||
(In reply to comment #75)
> Just to be extra careful, can you verify when putting the unzipped build in
> the same directory the failing build was under? Thanks.
Aha, it hangs.
Assignee | ||
Comment 77•13 years ago
|
||
(In reply to comment #76)
> (In reply to comment #75)
> > Just to be extra careful, can you verify when putting the unzipped build in
> > the same directory the failing build was under? Thanks.
> Aha, it hangs.
Can you try another build that used to work, under the same directory?
Comment 78•13 years ago
|
||
(In reply to comment #77)
> (In reply to comment #76)
> > (In reply to comment #75)
> > > Just to be extra careful, can you verify when putting the unzipped build in
> > > the same directory the failing build was under? Thanks.
> > Aha, it hangs.
>
> Can you try another build that used to work, under the same directory?
I tried with the following latest hourlybuild under the same named folder(D:\trunk\2011\05\firefox10-May-2011 1008 Bug 620931 Omni.jar support for xulrunner\).
And I confirmed it is no problem. does not hang.
http://hg.mozilla.org/mozilla-central/rev/1f5db39fbb1a
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110511 Firefox/6.0a1 ID:20110511004805
Assignee | ||
Comment 79•13 years ago
|
||
Okay, so the only explanation that makes sense is that the omnijar argument passing never worked on windows, but it doesn't matter for plugins (it however matters for content processes on fennec)
What this bug changed, is that errors are not tolerated anymore in the omnijar argument passing.
This also tells us OOPP doesn't like when the child process fails early.
Assignee | ||
Comment 80•13 years ago
|
||
And for what it's worth, I can reproduce with a space character in the directory containing the build.
Comment 81•13 years ago
|
||
(In reply to comment #73)
> Can people seeing these freezes test these builds:
> https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mh@glandium.org-
> 3d08dc03ef77/try-win32/
>
> Thanks
Still getting hangs.
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110510 Firefox/6.0a1 - Build ID: 20110510230021
Comment 82•13 years ago
|
||
Follow up after reading posts again.
Installed the tryserver build in it's own directory as opposed to installing over my normal Fx directory. There were no hangs.
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110510 Firefox/6.0a1 - Build ID: 20110510230021
Assignee | ||
Comment 83•13 years ago
|
||
So in the end, this was not the XRE_GetFileFromPath at all. In fact,
XRE_GetFileFromPath is actually good. And as the real issue actually
exists in the current code base, though it's not creating problems,
I'll attach the actual fix to bug 656174.
The patch I'm attaching here is completely unrelated to the issue,
it's just a fail-safe when -greomni is given and -appomni is not. At
the time the arguments are treated and Omnijar::Init called, the
directory service is not up yet so the fallback when appOmni is null
can't be used.
Attachment #531918 -
Flags: review?(mwu)
Assignee | ||
Updated•13 years ago
|
Attachment #531553 -
Attachment is obsolete: true
Assignee | ||
Comment 84•13 years ago
|
||
Actually, it's just better here than to reopen the regressions bugs.
So, the issue was that when the containing directory contains a space,
the omnijar initialization wouldn't work. It doesn't matter with current
code, but it does matter with the new one. So here, we just fix quoting
when starting the child process.
The command_line.cc part is actually stolen from a more recent version of
the corresponding chrome code.
Attachment #531920 -
Flags: review?(benjamin)
Comment 85•13 years ago
|
||
Comment on attachment 531920 [details] [diff] [review]
part 2.5 - Properly quote arguments on Windows when starting child processes
I think this is right, but robstrong has done this kind of code several times before and might have code we can either share or copy.
Attachment #531920 -
Flags: review?(benjamin) → review?(robert.bugzilla)
Assignee | ||
Comment 86•13 years ago
|
||
Refreshed after landing of bug 655439
Assignee | ||
Updated•13 years ago
|
Attachment #531004 -
Attachment is obsolete: true
Assignee | ||
Comment 87•13 years ago
|
||
Dexter, Alice, can you give a try to this one under a directory where the previous one failed?
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mh@glandium.org-5dba7db3dbfd/try-win32/
Comment 88•13 years ago
|
||
(In reply to comment #87)
> Dexter, Alice, can you give a try to this one under a directory where the
> previous one failed?
> https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mh@glandium.org-
> 5dba7db3dbfd/try-win32/
Works For Me, I installed trybuild under the same folder of comment #78.
Comment 89•13 years ago
|
||
(In reply to comment #83)
> Created attachment 531918 [details] [diff] [review] [review]
> Small unrelated fixup
>
> So in the end, this was not the XRE_GetFileFromPath at all. In fact,
> XRE_GetFileFromPath is actually good. And as the real issue actually
> exists in the current code base, though it's not creating problems,
> I'll attach the actual fix to bug 656174.
>
I don't want to use XRE_GetFileFromPath unless you can ensure that the incoming argument is utf8 encoded.
> The patch I'm attaching here is completely unrelated to the issue,
> it's just a fail-safe when -greomni is given and -appomni is not. At
> the time the arguments are treated and Omnijar::Init called, the
> directory service is not up yet so the fallback when appOmni is null
> can't be used.
As far as I can tell, this behavior already exists since path isn't cleared when there is no appomni arg.
Assignee | ||
Comment 90•13 years ago
|
||
(In reply to comment #89)
> I don't want to use XRE_GetFileFromPath unless you can ensure that the
> incoming argument is utf8 encoded.
All other arguments taking paths use XRE_GetFileFromPath, and all arguments are actually converted to utf8 in toolkit/xre/nsWindowsWMain.cpp.
> As far as I can tell, this behavior already exists since path isn't cleared
> when there is no appomni arg.
Without the additional patch, if there is no appomni arg, Omnijar::Init is called with (greomni, null), and which calls InitOne(greomni), and then InitOne(null), and the latter will use the directory service.
Comment 91•13 years ago
|
||
(In reply to comment #90)
> (In reply to comment #89)
> > I don't want to use XRE_GetFileFromPath unless you can ensure that the
> > incoming argument is utf8 encoded.
>
> All other arguments taking paths use XRE_GetFileFromPath, and all arguments
> are actually converted to utf8 in toolkit/xre/nsWindowsWMain.cpp.
>
Ah! Good.
> > As far as I can tell, this behavior already exists since path isn't cleared
> > when there is no appomni arg.
>
> Without the additional patch, if there is no appomni arg, Omnijar::Init is
> called with (greomni, null), and which calls InitOne(greomni), and then
> InitOne(null), and the latter will use the directory service.
If there is no appomni arg,
+ if (path) {
+ rv = XRE_GetFileFromPath(path, getter_AddRefs(appOmni));
+ if (NS_FAILED(rv)) {
+ PR_fprintf(PR_STDERR, "Error: argument -appomni requires a valid path\n");
+ return rv;
+ }
+ }
sets up appOmni anyways since path isn't cleared in CheckArg.
Assignee | ||
Comment 92•13 years ago
|
||
Comment on attachment 531918 [details] [diff] [review]
Small unrelated fixup
You're right :)
Attachment #531918 -
Attachment is obsolete: true
Attachment #531918 -
Flags: review?(mwu)
Comment 93•13 years ago
|
||
Comment on attachment 531920 [details] [diff] [review]
part 2.5 - Properly quote arguments on Windows when starting child processes
>diff --git a/ipc/chromium/src/base/command_line.cc b/ipc/chromium/src/base/command_line.cc
>--- a/ipc/chromium/src/base/command_line.cc
>+++ b/ipc/chromium/src/base/command_line.cc
>@@ -259,45 +259,75 @@ std::wstring CommandLine::PrefixedSwitch
> #if defined(OS_WIN)
> void CommandLine::AppendSwitch(const std::wstring& switch_string) {
> std::wstring prefixed_switch_string = PrefixedSwitchString(switch_string);
> command_line_string_.append(L" ");
> command_line_string_.append(prefixed_switch_string);
> switches_[WideToASCII(switch_string)] = L"";
> }
>
>+// Quote a string if necessary, such that CommandLineToArgvW() will
>+// always process it as a single argument.
>+static std::wstring WindowsStyleQuote(const std::wstring& arg) {
>+ // We follow the quoting rules of CommandLineToArgvW.
>+ // http://msdn.microsoft.com/en-us/library/17w5ykft.aspx
>+ if (arg.find_first_of(L" \\\"") == std::wstring::npos) {
tabs are also valid though I don't know if that would be an issue for this use.
There are a couple of implementations of this in the tree... one for nsIProcess and
http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsWindowsRestart.cpp#114
There are tests for the nsWindowsRestart one
http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/test/win/
I am fairly certain this is correct but I'd feel better if there were tests.
r=me with the tab added if it is possible for a tab and tests or a reason why test aren't possible.
Attachment #531920 -
Flags: review?(robert.bugzilla) → review+
Assignee | ||
Comment 94•13 years ago
|
||
(In reply to comment #93)
> I am fairly certain this is correct but I'd feel better if there were tests.
>
> r=me with the tab added if it is possible for a tab
I guess it's possible, though it wouldn't be as common as spaces.
> and tests or a reason why test aren't possible.
At this point, I'd say a lack of time. I'd like that to land early enough before the Aurora merge to be tested on nightlies. May I go ahead without a test and land a test later?
Comment 95•13 years ago
|
||
I'm ok with that though bsmedberg might feel differently as long as you followup with tests.
Assignee | ||
Comment 96•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/fdd5cd8f391f
http://hg.mozilla.org/mozilla-central/rev/88caff1a09d0
http://hg.mozilla.org/mozilla-central/rev/bab28d839f5e
http://hg.mozilla.org/mozilla-central/rev/c744de96a133
http://hg.mozilla.org/mozilla-central/rev/5a4b358de96f
http://hg.mozilla.org/mozilla-central/rev/dec16a247230
I'll take care of the test today or tomorrow.
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Updated•9 years ago
|
Product: Toolkit → Toolkit Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•