127442 - Installing New Themes not possible, leads to a crash

Status: VERIFIED FIXED

(SeaMonkey :: Themes, defect, P2)

Description

[Carsten Menke]

Go to View->Apply Themes->Get New Themes, now take any theme you want from
xulplanet, as soon as you hit the Install Now link, Mozilla Crashes.

This happened with the Build from Feb 2nd until today's build Feb 23th 2002.
Build System:
Mandrake 8.0 gcc-2.95.3 glib-2.2.2, gtk-1.2.10 Kernel: 2.4.8-ac7

Build Options:
ac_add_options --prefix=/usr/lib/mozilla-097
ac_add_options --libdir=/usr/lib/mozilla-097
ac_add_options --includedir=/usr/include/mozilla-097
ac_add_options --enable-crypto
ac_add_options --disable-accessibility
ac_add_options --enable-mathml
ac_add_options --enable-svg
ac_add_options --disable-tests
ac_add_options --disable-debug
ac_add_options --enable-optimize="-O6 -march=i686 -mcpu=i686"
ac_add_options --disable-dtd-debug
ac_add_options --enable-strip
ac_add_options --with-default-mozilla-five-home=/usr/lib/mozilla-097
ac_add_options --with-user-appdir=".mozilla-098"
ac_add_options --enable-static
ac_add_options --disable-shared

I bet that this is directly related to bug 119450, and fixing that bug would
also fix this bug. I marked this blocker, due to that this both things are not
working I could not imagine that this should go into 0.99

GDB Output Attached

Comment 1

[Carsten Menke]

Attachment: GDB Output

Comment 2

[Brant Gurganus]

WFM 2002022203/WinXP

Comment 3

[Matthias Versen [:Matti]]

wfm with win2k 20020223..

Comment 4

[Carsten Menke]

Hmmh, intersting because it works on Win32 it is not a blocker ?

Comment 5

[Andrew Schultz]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:0.9.8+) Gecko/20020222
WFM.  This is not a blocker because it does not block Mozilla development.

Do you crash on all themes?  Have you tried installing other xpi packages
(stacktrace shows you are in xpinstall)?

Comment 6

[Carsten Menke]

Yes all themes I tried lead to a crash, also using:
Edit->Preferences->Appearance->Content/Language Packs->Download More

to install a new Language Pack leads to a crash. Give me some links, I should
try and I will attach a gdb for each.

Comment 7

[hacker formerly known as [e-mail]]

Confirmed the crash using the build options posted.  Also confirmed it without
the optimize flags & without the strip option.  The trace looks similar to what
Carsten attached.  

Comment 8

[Colin Blake]

I get the same crash when trying to install any theme from the "View->Apply
Themes->Get New Themes" page. OpenVMS (static build based on Linux), 1.0-rc1.

Comment 9

[Colin Blake]

This appears to be a static build problem. Shouldn't Seawood be in on this? 
Taking the liberty of cc'ing him.

This might have something to do with the fact that libxpinstall.so has its own 
copy of the javascript code (libmozjs). Looking at the call frames you can see 
javascript routines from both the main mozilla image and libxpinstall.so being 
invoked. Something's then getting very confused.

There's a comment in libxpinstall's Makefile saying that it has to be built 
shared, even for static builds, because its used by the installer. So does 
libmozjs also need to be built shared?

Comment 10

[Colin Blake]

Attachment: Build mozjs shared, even on a static build

JavaScript must be built shared, even for static builds, as it is used by other
modules which are always built shared. Failure to do so results in the js code
getting copied into xpinstall and jsd as well as mozilla-bin, and then the
static data cells used for locking no longer work.

Attaching patch. Can we get this into RC2?

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 82625 [details] [diff] [review]
Build mozjs shared, even on a static build

r=[e-mail]

This is definitely safe to do, and appears to be the issue here (while there
might be better ways to solve shared/static dependencies like this in the
future, they're much more complex).  We still want r/sr=seawood before approval
can be given.

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

*** Bug 119450 has been marked as a duplicate of this bug. ***

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

Reassigning to patch owner (I volunteer to check it in if needed).

This is a wgate issue (we use static build - big perf win).

CC'ing some JS/xpcom people.  Looking for sr (especially from seawood) and then
approval.

Comment 14

[Colin Blake]

Attachment: Revised patch with EXPORT_LIBRARY removed

I just completed a full clean build and with the previous patch the link of
mozilla-bin was still looking for libmozjs.a. This is because EXPORT_LIBRARY
was set in mozjs's Makefile. So the second version of the patch removes
EXPORT_LIBRARY. Now mozilla-bin links.

Comment 15

[Colin Blake]

Attachment: Make embedding tests link too

The GTK embedding tests now need to link with MOZ_JS_LIBS since the js code is
no longer in a static lib. Hopefully this is the final version of the patch.

Comment 16

[Colin Blake]

Attachment: MOZ_JS_LIBS for mozilla-bin static link

mozilla-bin now needs MOZ_JS_LIBS for both static and shared libs.

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

Added to RC3 buglist as per shaver and Asa

Comment 18

[hacker formerly known as [e-mail]]

Comment on attachment 82908 [details] [diff] [review]
MOZ_JS_LIBS for mozilla-bin static link

We should be using FORCE_SHARED_LIBS (as we do for win32) rather than
redefining  BUILD_STATIC_LIBS & BUILD_SHARED_LIBS.  Also, because JS requires
NSPR in cross compiled builds, we should be linking JS before NSPR.

Comment 19

[hacker formerly known as [e-mail]]

Just a thought: are we going to run into a similar problem with our other
application level libs that are also linked into some components (gfx, jsj,
etc)?  Should we only be linking the components statically?  If so, I need to
rename the configure option has it's a huge misnomer.

Comment 20

[hacker formerly known as [e-mail]]

Attachment: Use FORCE_SHARED_LIB, reorder link decl & update static packaging

Comment 21

[Colin Blake]

> Are we going to run into a similar problem with our other application
> level libs

Anything which has static data and is linked into more than one shared library 
(or linked into one shared library and mozilla-bin) is busted, I think.

Comment 22

[Cathleen]

do we need to address this in windows static build too?

Comment 23

[hacker formerly known as [e-mail]]

Cathleen: This particular problem? No. Win32 was already building js dynamically
in static builds. 

Comment 24

[chris hofmann]

lets get reivew going.

Comment 25

[Brian Ryner (not reading)]

Comment on attachment 83305 [details] [diff] [review]
Use FORCE_SHARED_LIB, reorder link decl & update static packaging

r=bryner

Comment 26

[hacker formerly known as [e-mail]]

Patch has been checked into the trunk.

Comment 27

[Dawn Endico]

we need an sr on this. drivers wants to take this for rc3. It will need to be
checked in to the branch by thursday pm.

Comment 28

[Colin Blake]

Reassigning to (correct) patch owner. Thanks for the right fix, Chris!

Comment 29

[chris hofmann]

Comment on attachment 83305 [details] [diff] [review]
Use FORCE_SHARED_LIB, reorder link decl & update static packaging

a=chofmann for the 1.0 branch

Comment 30

[Dawn Endico]

seawood, can you check this in asap to get in for 1.0?

Comment 31

[hacker formerly known as [e-mail]]

The patch has been checked into the moz1.0.0 branch.

Comment 32

[hacker formerly known as [e-mail]]

fixed.

Comment 33

[Patty Mac]

Verified on linux redhat 7.3 (netscape trunk build: 2002-11-18-08-TRUNK), and
netscape branch build (2002-11-14-07-1.0)