Closed Bug 722262 Opened 12 years ago Closed 12 years ago

Port |Bug 552864 - Throw away wrapper shell script on unix and lazily load libxul| to SeaMonkey

Categories

(SeaMonkey :: Startup & Profiles, defect)

defect
Not set
normal

Tracking

(seamonkey2.7 wontfix, seamonkey2.8 wontfix, seamonkey2.9 wontfix)

RESOLVED FIXED
seamonkey2.10
Tracking Status
seamonkey2.7 --- wontfix
seamonkey2.8 --- wontfix
seamonkey2.9 --- wontfix

People

(Reporter: sgautherie, Assigned: Callek)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 7 obsolete files)

See also TB bug 668869.
Blocks: FF2SM
Whiteboard: ff2sm
Blocks: 523773
No longer blocks: 721357
Summary: Port |Bug 668869 - port ffox work to lazily load libxul to speed up start-up perf and remove wrapper startup script| to SeaMonkey → Port |Bug 552864 - Throw away wrapper shell script on unix and lazily load libxul| to SeaMonkey
I hope to do this this cycle (and have been tracking it to do so, just never got around to filing the bug, afaict)

It is too risky to take on aurora though so wontfix-2.9
Assignee: nobody → bugspam.Callek
Status: NEW → ASSIGNED
Attached patch WIP 0 (obsolete) — Splinter Review
This is just a snapshot of where I am right now, I don't think this compiles (did not test yet) and I am pretty sure I still have a lot to do.

Notes:
* Yes I am adding features here at the same time (I think I need to turn on Telemetry for this code)
* I need to add some confvars code to support the gen app-ini stuff.
* Our Makefile for suite/app has a lot of cruft in it and will be (attempted) to be cleaned up here.
* I will be trying to limit the scope of this all to the bare minimum to support what I did in suite/app/*.cpp here and cleaning up that Makefile, beyond that (moving stuff, changing how extensions are packaged to match Firefox's test-pilot, etc.) will be out of scope here.
Attached patch WIP 0.5 (obsolete) — Splinter Review
Attachment #594527 - Attachment is obsolete: true
Blocks: 704835
Attached patch WIP 1.0 (obsolete) — Splinter Review
This is it for the night, this actually _does compile_ (on win anyway) and _does startup_.

I did not test much else yet, but this looks like a good WIP first step.
Attachment #594652 - Attachment is obsolete: true
Attached patch v1.0 (obsolete) — Splinter Review
Ok, so far as I can tell, this should do it -- accurately.

I did not test on anything other than Win7, and I did add LIBXUL supporting code, even though we don't actively support it that way (yet).

I'm HOPING for a fast review, since this is relatively risky enough to want it to land early in the cycle
Attachment #594658 - Attachment is obsolete: true
Attachment #594861 - Flags: review?(kairo)
Attachment #594861 - Flags: feedback?(mnyromyr)
Comment on attachment 594861 [details] [diff] [review]
v1.0

Review of attachment 594861 [details] [diff] [review]:
-----------------------------------------------------------------

I'm marking this reviewed, though I didn't exactly look at every line of this, in summary this looks good though, and if it builds on all platforms, I think it's a good step. Thanks for this work!
Attachment #594861 - Flags: review?(kairo) → review+
Attached patch v1.5 (obsolete) — Splinter Review
r+ from KaiRo, this has a few minor changes that should make a real difference for mac though
Attachment #594861 - Attachment is obsolete: true
Attachment #594861 - Flags: feedback?(mnyromyr)
Attachment #595981 - Flags: review+
Attachment #595981 - Flags: feedback?
Attachment #595981 - Flags: feedback? → feedback?(mnyromyr)
Comment on attachment 595981 [details] [diff] [review]
v1.5

This would break Linux builds - see bug 668869 comment 14.
Attachment #595981 - Flags: review-
So following up and documenting here for sanity:

Justin@ORION /d/Objects/SeaMonkeyTrunk/mozilla/xpcom/stub
$ make echo-variable-DEPENDENT_LIBS_LIST
nspr4.dll plc4.dll plds4.dll mozalloc.dll  mozsqlite3.dll nssutil3.dll softokn3.dll nss3.dll ssl3.dl
l smime3.dll  mozjs.dll xul.dll

Justin@ORION /d/Objects/SeaMonkeyTrunk/mozilla/xpcom/stub
$ ls ../../dist/bin/*.dll
../../dist/bin/AccessibleMarshal.dll  ../../dist/bin/nsldappr32v60.dll
../../dist/bin/D3DCompiler_43.dll     ../../dist/bin/nsldif32v60.dll
../../dist/bin/IA2Marshal.dll         ../../dist/bin/nspr4.dll
../../dist/bin/MapiProxy.dll          ../../dist/bin/nss3.dll
../../dist/bin/crashinjectdll.dll     ../../dist/bin/nssckbi.dll
../../dist/bin/d3dx9_43.dll           ../../dist/bin/nssdbm3.dll
../../dist/bin/freebl3.dll            ../../dist/bin/nssutil3.dll
../../dist/bin/gkmedias.dll           ../../dist/bin/plc4.dll
../../dist/bin/libEGL.dll             ../../dist/bin/plds4.dll
../../dist/bin/libGLESv2.dll          ../../dist/bin/smime3.dll
../../dist/bin/mozMapi32.dll          ../../dist/bin/softokn3.dll
../../dist/bin/mozalloc.dll           ../../dist/bin/ssl3.dll
../../dist/bin/mozglue.dll            ../../dist/bin/vmwarerecordinghelper.dll
../../dist/bin/mozjs.dll              ../../dist/bin/xpcom.dll
../../dist/bin/mozsqlite3.dll         ../../dist/bin/xul.dll
../../dist/bin/nsldap32v60.dll

Justin@ORION /d/Objects/SeaMonkeyTrunk/mozilla/xpcom/stub
$ pushd ../../dist/bin/; rm -f `make -C ../../xpcom/stub echo-variable-DEPENDENT_LIBS_LIST`; ls *.d
ll; popd

AccessibleMarshal.dll  d3dx9_43.dll   mozMapi32.dll      nssckbi.dll
D3DCompiler_43.dll     freebl3.dll    mozglue.dll        nssdbm3.dll
IA2Marshal.dll         gkmedias.dll   nsldap32v60.dll    vmwarerecordinghelper.dll
MapiProxy.dll          libEGL.dll     nsldappr32v60.dll  xpcom.dll
crashinjectdll.dll     libGLESv2.dll  nsldif32v60.dll
[seabld@cb-seamonkey-linux-01 bin]$ make -C ../../xpcom/stub echo-variable-DEPENDENT_LIBS_LIST
make: Entering directory `/builds/slave/comm-cen-trunk-lnx/build/objdir/mozilla/xpcom/stub'
libnspr4.so libplc4.so libplds4.so libmozalloc.so  libmozsqlite3.so libnssutil3.so libsoftokn3.so libnss3.so libssl3.so libsmime3.so  libxul.so
make: Leaving directory `/builds/slave/comm-cen-trunk-lnx/build/objdir/mozilla/xpcom/stub'

(I'm trimming out OS provided libs from the following, just for the record)

[seabld@cb-seamonkey-linux-01 bin]$ readelf -d libxpcom.so | grep NEEDED
 0x00000001 (NEEDED)                     Shared library: [libxul.so]
 0x00000001 (NEEDED)                     Shared library: [libplds4.so]
 0x00000001 (NEEDED)                     Shared library: [libplc4.so]
 0x00000001 (NEEDED)                     Shared library: [libnspr4.so]
 0x00000001 (NEEDED)                     Shared library: [libmozalloc.so]
[seabld@cb-seamonkey-linux-01 bin]$ readelf -d libxul.so | grep NEEDED
 0x00000001 (NEEDED)                     Shared library: [libsmime3.so]
 0x00000001 (NEEDED)                     Shared library: [libssl3.so]
 0x00000001 (NEEDED)                     Shared library: [libnss3.so]
 0x00000001 (NEEDED)                     Shared library: [libnssutil3.so]
 0x00000001 (NEEDED)                     Shared library: [libldap60.so]  <!--X
 0x00000001 (NEEDED)                     Shared library: [libprldap60.so] <!--X
 0x00000001 (NEEDED)                     Shared library: [libldif60.so] <!--X
 0x00000001 (NEEDED)                     Shared library: [libmozsqlite3.so]
 0x00000001 (NEEDED)                     Shared library: [libplds4.so]
 0x00000001 (NEEDED)                     Shared library: [libplc4.so]
 0x00000001 (NEEDED)                     Shared library: [libnspr4.so]
 0x00000001 (NEEDED)                     Shared library: [libmozalloc.so]

(all others have merely duplicates of the above or sys libraries)
This will be used in v2 of the comm-central patch.
Attachment #596465 - Flags: review?(mh+mozilla)
Attachment #595981 - Attachment is obsolete: true
Attachment #596465 - Attachment is obsolete: true
Attachment #595981 - Flags: feedback?(mnyromyr)
Attachment #596465 - Flags: review?(mh+mozilla)
Attachment #596467 - Flags: review?(mh+mozilla)
Comment on attachment 596466 [details] [diff] [review]
[c-c] v2
[Checked in: Comment 20]

I'll accept a review from Mark or Mike here. The key review point is as it relates to dependentlibs.

The rest of this patch was reviewed by KaiRo, but happy to have comments from either of you if you catch anything wrong.

f? from Karsten for basic functionality testing on mac. and f? from ewong for extra-sanity-checking functionality testing on linux. [I won't block landing on the feedbacks]
Attachment #596466 - Flags: review?(mh+mozilla)
Attachment #596466 - Flags: review?(mbanner)
Attachment #596466 - Flags: feedback?(mnyromyr)
Attachment #596466 - Flags: feedback?(ewong)
reattached a corrected patch per glandium.
Attachment #596467 - Attachment is obsolete: true
Attachment #596467 - Flags: review?(mh+mozilla)
Attachment #596469 - Flags: review?(mh+mozilla)
Ooh, I like the solution. Want to test it out before I give r+ but the idea looks great.
Comment on attachment 596466 [details] [diff] [review]
[c-c] v2
[Checked in: Comment 20]

Mac looks okay (rebuild/clobber).
Attachment #596466 - Flags: feedback?(mnyromyr) → feedback+
Attachment #596469 - Flags: review?(mh+mozilla) → review+
Comment on attachment 596466 [details] [diff] [review]
[c-c] v2
[Checked in: Comment 20]

Review of attachment 596466 [details] [diff] [review]:
-----------------------------------------------------------------

Whitespace excluded, nsSuiteApp.cpp just becomes the same content as nsBrowserApp.cpp, so it's obviously good :) The corresponding changes to Makefile.in are ok, but I do wonder where some of the stuff that's not in browser/app/Makefile.in come from. The rest looks good.

::: suite/extradependlibs.mk
@@ +6,5 @@
> +# Presume all mozilla/autoconf.mk is included and that it does not have
> +# Direct access to c-c specific vars not present there.
> +
> +# We don't have access to MOZ_LDAP_XPCOM here, so cheat.
> +ifneq (,$(findstring ldap,$(MOZ_APP_COMPONENT_LIBS)))

$(filter mozldap,$(MOZ_APP_COMPONENT_LIBS)) would be better imho
Attachment #596466 - Flags: review?(mh+mozilla) → review+
Whiteboard: [leave open after inbound merge]
Attachment #596469 - Attachment description: [m-c] v1.1 - include stuff from app → [m-c] v1.1 - include stuff from app [Checked in: Comment 19]
This landed:
http://hg.mozilla.org/comm-central/rev/9eddb2d0b431

Hmm... Just built myself from current trunk in my Linux VM. Then:

jens@holodoc:~/usr/local/seamonkey-central > ./seamonkey
XPCOMGlueLoad error for file /home/jens/usr/local/seamonkey-central/libxpcom.so:
libxul.so: cannot open shared object file: No such file or directory
Couldn't load XPCOM.

Same with seamonkey-bin, which is binary-equal to seamonkey (unlike firefox vs firefox-bin for the latest Nightly).

Is it just me?
Attachment #596466 - Attachment description: [c-c] v2 → [c-c] v2 [Checked in: Comment 20]
Whiteboard: [leave open after inbound merge]
Target Milestone: --- → seamonkey2.10
(In reply to Jens Hatlak (:InvisibleSmiley) from comment #20)
> This landed:
> http://hg.mozilla.org/comm-central/rev/9eddb2d0b431
> 
> Hmm... Just built myself from current trunk in my Linux VM. Then:
> 
> jens@holodoc:~/usr/local/seamonkey-central > ./seamonkey
> XPCOMGlueLoad error for file
> /home/jens/usr/local/seamonkey-central/libxpcom.so:
> libxul.so: cannot open shared object file: No such file or directory
> Couldn't load XPCOM.
> 
> Same with seamonkey-bin, which is binary-equal to seamonkey (unlike firefox
> vs firefox-bin for the latest Nightly).
> 
> Is it just me?

No not just you, Seems Standard8 noticed this as well, he may have a fix, but if not I'll back this out shortly.
Unfortunately the original patch didn't realise that dependentlibs.list was ordered and therefore would still break on Linux. This patch fixes the ordering by including the extra libs for the app first. It works for at least both SM & TB, so hopefully we'll be ok.
Attachment #597130 - Flags: review?(mh+mozilla)
Thanks mark and kyle, pushed the fixed:

http://hg.mozilla.org/mozilla-central/rev/b3a7561624f9
Attachment #597130 - Attachment description: Fix ordering for extra app libs in dependentlibs.list → Fix ordering for extra app libs in dependentlibs.list [Checked in: Comment 23]
Depends on: 727258
I didn't look too precisely, but tbpl hasn't reported any test run since "Tue Feb 14 04:29:59 2012 PST", which was the checkin before this one.

I assume this is not a result of bug 722187, which was later backed out, as it is js only.
Then this bug seems a likely culprit:
some builders may just have not (re)run since then,
but (some) Windows test builders seem to have a stuck SeaMonkey, which probably need a human maintenance or something!
	
Can you check these(/all) test builders?
(In reply to Serge Gautherie (:sgautherie) from comment #24)

> I didn't look too precisely, but tbpl hasn't reported any test run since
> "Tue Feb 14 04:29:59 2012 PST", which was the checkin before this one.

Oh, that must be another regression to be fixed by bug 727258, hopefully.

> Can you check these(/all) test builders?

Actually, only 'cb-seamonkey-win32-02' seems affected, afaict from current
http://tinderbox.mozilla.org/showbuilds.cgi?tree=SeaMonkey
(In reply to Serge Gautherie (:sgautherie) from comment #25)
> Actually, only 'cb-seamonkey-win32-02' seems affected, afaict from current
> http://tinderbox.mozilla.org/showbuilds.cgi?tree=SeaMonkey

Ftr, 'cb-seamonkey-win32-01' had been affected too (earlier), but seems fixed now.
And 'cb-seamonkey-win32-02' seems to (have) been fixed too (now).
Depends on: 727675
(In reply to Serge Gautherie (:sgautherie) from bug 722262 comment #25)
> (In reply to Serge Gautherie (:sgautherie) from bug 722262 comment #24)
> 
> > I didn't look too precisely, but tbpl hasn't reported any test run since
> > "Tue Feb 14 04:29:59 2012 PST", which was the checkin before this one.
> 
> Oh, that must be another regression to be fixed by bug 727258, hopefully.

I filed bug 722675.
> I filed bug 722675.
I think you meant Bug 727675. SeaMonkey doesn't run on Android :P
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment on attachment 596466 [details] [diff] [review]
[c-c] v2
[Checked in: Comment 20]

Clearing obsolete requests.
Attachment #596466 - Flags: review?(mbanner)
Attachment #596466 - Flags: feedback?(ewong)
Blocks: 729550
Comment on attachment 596466 [details] [diff] [review]
[c-c] v2
[Checked in: Comment 20]

Review of attachment 596466 [details] [diff] [review]:
-----------------------------------------------------------------

::: suite/installer/package-manifest.in
@@ -111,1 @@
>  @BINPATH@/@MOZ_APP_NAME@

Ftr, this part was from bug 549246.
No longer blocks: FF2SM
Depends on: 549246
Flags: in-testsuite-
Blocks: 670312
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: