Closed Bug 620931 Opened 13 years ago Closed 13 years ago

Omni.jar support for xulrunner

Categories

(Toolkit Graveyard :: XULRunner, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla6

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(10 files, 20 obsolete files)

594 bytes, patch
benjamin
: review+
Details | Diff | Splinter Review
2.47 KB, patch
glandium
: review+
Details | Diff | Splinter Review
3.17 KB, patch
glandium
: review+
Details | Diff | Splinter Review
2.10 KB, patch
glandium
: review+
Details | Diff | Splinter Review
2.05 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
20.06 KB, patch
Details | Diff | Splinter Review
46.89 KB, patch
mwu
: review+
Details | Diff | Splinter Review
24.69 KB, text/plain
Details
5.23 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
3.93 KB, 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.
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)
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)
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)
Attachment #499280 - Flags: review?(mwu)
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)
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)
(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.
Attachment #499276 - Flags: review?(benjamin) → review+
Attachment #499278 - Flags: review?(benjamin) → review+
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-
Attachment #499281 - Flags: review?(benjamin) → review+
Attachment #499282 - Flags: review?(benjamin) → review+
(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).
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)
Attachment #499280 - Attachment is obsolete: true
Attachment #499280 - Flags: review?(mwu)
Attachment #501665 - Flags: review?(mwu)
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-
(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 :(
(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 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+
(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.
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)
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+
(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 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+
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 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-
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)
Oops, the previous one was the wrong revision
Attachment #505014 - Attachment is obsolete: true
Attachment #505016 - Flags: review?(benjamin)
Attachment #505014 - Flags: review?(benjamin)
Again, refresh with context changes due to part 3 changes. Carrying over r+
Attachment #504865 - Attachment is obsolete: true
Attachment #505017 - Flags: review+
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.
Attachment #505016 - Flags: review?(benjamin) → review+
approval2.0 or post2.0?
Depends on: 595522
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+
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+
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+
any hope that this will be fixed in 4.0.1?
My take is that this will not land on 4.0.x, but can be applied by linux distros who see fit.
Depends on: post2.0
Blocks: 644790
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)
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.
(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.
(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.
Whiteboard: [not-ready-for-cedar]
Refreshed context. Carrying r+ over.
Attachment #515040 - Attachment is obsolete: true
Attachment #530570 - Flags: review+
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)
Refreshed context.
Attachment #499276 - Attachment is obsolete: true
Attachment #530572 - Flags: review+
Cleaned up patch description
Attachment #530570 - Attachment is obsolete: true
Attachment #530573 - Flags: review+
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)
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)
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.
The previous interdiff was weird.
Attachment #530584 - Attachment is obsolete: true
Attachment #530575 - Flags: review?(benjamin) → review+
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 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+
> ::: 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.
Attachment #530571 - Attachment is obsolete: true
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)
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)
Attachment #530894 - Flags: review?(mwu)
Attachment #530580 - Attachment is obsolete: true
Blocks: 655678
Attachment #531004 - Flags: review?(mwu) → review+
Attachment #530894 - Flags: review?(mwu) → review+
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
(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?
(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
Both sites in comment 54 WFM with Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110510 Firefox/6.0a1; D2D.
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
(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.
(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.
(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.
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
My WFM was using Flash 10.3.180.42, for reference.
(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 ?
(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.
Attached file FYI windgb log
IMO, this needs to be backed out or we are going have broken nightly's tomorrow.
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
Depends on: 656172
Filed bug [url=https://bugzilla.mozilla.org/show_bug.cgi?id=656172]Bug 656172 – Regression on Bug 620931 causing sites to hang.[/url]
Depends on: 656174
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 → ---
Attached patch Possible Fixup (obsolete) — Splinter Review
Attachment #531553 - Flags: review?(mwu)
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+
(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.
(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.
(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.
(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?
(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
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.
And for what it's worth, I can reproduce with a space character in the directory containing the build.
(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
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
Attached patch Small unrelated fixup (obsolete) — Splinter Review
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)
Attachment #531553 - Attachment is obsolete: true
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 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)
Attachment #531004 - Attachment is obsolete: true
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/
(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.
(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.
(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.
(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.
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 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+
(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?
I'm ok with that though bsmedberg might feel differently as long as you followup with tests.
Blocks: 671562
Depends on: 671798
Depends on: 1087488
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.