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

RESOLVED FIXED in seamonkey2.10

Status

SeaMonkey
Startup & Profiles
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: sgautherie, Assigned: Callek)

Tracking

(Blocks: 1 bug)

Trunk
seamonkey2.10
Dependency tree / graph
Bug Flags:
in-testsuite -

SeaMonkey Tracking Flags

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

Details

Attachments

(3 attachments, 7 obsolete attachments)

(Reporter)

Description

6 years ago
See also TB bug 668869.
(Reporter)

Updated

6 years ago
Blocks: 467530
Whiteboard: ff2sm
(Reporter)

Updated

6 years ago
Blocks: 523773
No longer blocks: 721357
(Reporter)

Updated

6 years ago
status-seamonkey2.7: --- → wontfix
status-seamonkey2.8: --- → wontfix
status-seamonkey2.9: --- → affected
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
(Assignee)

Comment 1

6 years ago
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-seamonkey2.9: affected → wontfix
(Reporter)

Updated

6 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 2

6 years ago
Created attachment 594527 [details] [diff] [review]
WIP 0

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.
(Assignee)

Comment 3

6 years ago
Created attachment 594652 [details] [diff] [review]
WIP 0.5
Attachment #594527 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Blocks: 704835
(Assignee)

Comment 4

6 years ago
Created attachment 594658 [details] [diff] [review]
WIP 1.0

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
(Assignee)

Comment 5

6 years ago
Created attachment 594861 [details] [diff] [review]
v1.0

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 6

6 years ago
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+
(Assignee)

Comment 7

5 years ago
Created attachment 595981 [details] [diff] [review]
v1.5

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?
(Assignee)

Updated

5 years ago
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-
(Assignee)

Comment 9

5 years ago
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
(Assignee)

Comment 10

5 years ago
[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)
(Assignee)

Comment 11

5 years ago
Created attachment 596465 [details] [diff] [review]
[m-c] v1 - include stuff from app

This will be used in v2 of the comm-central patch.
Attachment #596465 - Flags: review?(mh+mozilla)
(Assignee)

Comment 12

5 years ago
Created attachment 596466 [details] [diff] [review]
[c-c] v2
[Checked in: Comment 20]
Attachment #595981 - Attachment is obsolete: true
Attachment #596465 - Attachment is obsolete: true
Attachment #595981 - Flags: feedback?(mnyromyr)
Attachment #596465 - Flags: review?(mh+mozilla)
(Assignee)

Comment 13

5 years ago
Created attachment 596467 [details] [diff] [review]
[m-c] v1 - include stuff from app
Attachment #596467 - Flags: review?(mh+mozilla)
(Assignee)

Comment 14

5 years ago
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)
(Assignee)

Comment 15

5 years ago
Created attachment 596469 [details] [diff] [review]
[m-c] v1.1 - include stuff from app
[Checked in: Comment 19]

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+
(Assignee)

Updated

5 years ago
Whiteboard: [leave open after inbound merge]
https://hg.mozilla.org/mozilla-central/rev/0d95f0ca65a0
(Reporter)

Updated

5 years ago
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?
(Reporter)

Updated

5 years ago
Attachment #596466 - Attachment description: [c-c] v2 → [c-c] v2 [Checked in: Comment 20]
(Reporter)

Updated

5 years ago
Whiteboard: [leave open after inbound merge]
Target Milestone: --- → seamonkey2.10
(Assignee)

Comment 21

5 years ago
(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.
Created attachment 597130 [details] [diff] [review]
Fix ordering for extra app libs in dependentlibs.list
[Checked in: Comment 23]

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)
Attachment #597130 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 23

5 years ago
Thanks mark and kyle, pushed the fixed:

http://hg.mozilla.org/mozilla-central/rev/b3a7561624f9
(Reporter)

Updated

5 years ago
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]
(Assignee)

Updated

5 years ago
Depends on: 727258
(Reporter)

Comment 24

5 years ago
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?
(Reporter)

Comment 25

5 years ago
(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
(Reporter)

Comment 26

5 years ago
(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).
(Reporter)

Updated

5 years ago
Depends on: 727675
(Reporter)

Comment 27

5 years ago
(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.

Comment 28

5 years ago
> I filed bug 722675.
I think you meant Bug 727675. SeaMonkey doesn't run on Android :P
(Assignee)

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 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)
(Reporter)

Updated

5 years ago
Blocks: 729550
(Reporter)

Comment 30

5 years ago
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.
(Reporter)

Updated

5 years ago
No longer blocks: 467530
Depends on: 549246
(Reporter)

Updated

5 years ago
Flags: in-testsuite-

Updated

5 years ago
Blocks: 670312
You need to log in before you can comment on or make changes to this bug.