User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.4 (KHTML, like Gecko) Chrome/22.0.1229.94 Safari/537.4 Steps to reproduce: Various mozilla products, such as the pre-built binary versions of Firefox 15,16 , Thunderbird 15,16 , Seamonkey 2.13.2 , etc. will crash (segfault) at some point (often on exit) after they load plugins or extensions that link to spidermonkey-1.8.5. The root cause of this issue seems to be that either firefox uses symbols from the external libmozjs185.so or the plugin uses symbols from libxul.so, either one of which will cause a crash as the JS components within libxul are not ABI compatible with libmozjs185. The solution seems to be that unique symbol versioning must be applied to both spidermonkey and the js symbols in libxul. With the help of 'glandium' in #developers on irc.mozilla.org I was able to develop these two patches, which when tested on my system seem to prevent the crash of FF16.0.2 caused by libproxy-0.4.10 (linked to spidemronkey-1.8.5) Actual results: I don't know how to attach two files to the same bug at once, so here is the patch against spidermonkey-1.8.5.: diff -Naur js/src/configure.in js.new/src/configure.in --- a/js/src/configure.in 2012-11-07 09:36:16.000000000 -0500 +++ b/js/src/configure.in 2012-11-07 09:37:05.000000000 -0500 @@ -6018,6 +6018,11 @@ rm conftest.sh echo $MAKEFILES > unallmakefiles +cat <<SYMVEREOF >symverscript +MOZJS185 { + global: *; +}; +SYMVEREOF mv -f config/autoconf.mk config/autoconf.mk.orig 2> /dev/null diff -Naur js/src/Makefile.in js.new/src/Makefile.in --- a/js/src/Makefile.in 2012-11-07 09:36:16.000000000 -0500 +++ b/js/src/Makefile.in 2012-11-07 09:37:09.000000000 -0500 @@ -871,7 +871,7 @@ SHLIB_ANY_VER := $(DESTDIR)$(libdir)/$(SHARED_LIBRARY) SHLIB_ABI_VER := $(DESTDIR)$(libdir)/$(SHARED_LIBRARY).$(SRCREL_ABI_VERSION) SHLIB_EXACT_VER := $(DESTDIR)$(libdir)/$(SHARED_LIBRARY).$(SRCREL_VERSION) -$(SHARED_LIBRARY): EXTRA_DSO_LDOPTS += -Wl,-soname,$(notdir $(SHLIB_ABI_VER)) +$(SHARED_LIBRARY): EXTRA_DSO_LDOPTS += -Wl,-soname,$(notdir $(SHLIB_ABI_VER)) -Wl,-version-script,symverscript endif endif

second patch, attached as a file.

Comment on attachment 679143 [details] [diff] [review] spidermonkey-1.8.5-symver.patch Review of attachment 679143 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/Makefile.in @@ +871,4 @@ > SHLIB_ANY_VER := $(DESTDIR)$(libdir)/$(SHARED_LIBRARY) > SHLIB_ABI_VER := $(DESTDIR)$(libdir)/$(SHARED_LIBRARY).$(SRCREL_ABI_VERSION) > SHLIB_EXACT_VER := $(DESTDIR)$(libdir)/$(SHARED_LIBRARY).$(SRCREL_VERSION) > +$(SHARED_LIBRARY): EXTRA_DSO_LDOPTS += -Wl,-soname,$(notdir $(SHLIB_ABI_VER)) -Wl,-version-script,symverscript soname should already be set by the build system, so you only really need -version-script. Please don't generate the symbol version script in configure.in, but instead, create it in Makefile.in with Preprocessor.py (check the various places in the tree using that script), and add it as a dependency of the shared library target.

Comment on attachment 679142 [details] [diff] [review] firefox-js-symver.patch Review of attachment 679142 [details] [diff] [review]: ----------------------------------------------------------------- ::: mozilla-release/toolkit/library/Makefile.in @@ +617,4 @@ > endif > endif > > +EXTRA_DSO_LDOPTS += -Wl,-version-script,../../symverscript Same comments as the other patch ; this also needs to be conditional to building for linux.

Note you can merge both patches.

(In reply to Mike Hommey [:glandium] from comment #2) > > +$(SHARED_LIBRARY): EXTRA_DSO_LDOPTS += -Wl,-soname,$(notdir $(SHLIB_ABI_VER)) -Wl,-version-script,symverscript > > soname should already be set by the build system, so you only really need > -version-script. > That's all i did -- the '-Wl,-soname,$(notdir $(SHLIB_ABI_VER))' part already existed within Makefile.in, i just appended to it. > Please don't generate the symbol version script in configure.in, but > instead, create it in Makefile.in with Preprocessor.py (check the various > places in the tree using that script), and add it as a dependency of the > shared library target. I did it within configure.in for Firefox as the symbol-version is created dynamically from ${MOZ_APP_NAME} and ${MOZ_APP_VERSION} (which i thought would theoretically allow the exact same code to be used across all mozilla products). (In reply to Mike Hommey [:glandium] from comment #4) > Note you can merge both patches. I was thinking that would be the case, but I haven't tested it. I'm working from official source tarball distfiles rather than sources checked out from VCS. Can the improvements necessary for integration be left to the mozilla dev's (who know what they're doing) or would it be quicker if I attempted to improve the patches and resubmit?

This patch generates the symverscript and applies it appropriately, in better accordance with the mozilla build system

This patch is of a similar format to the updated FF patch, but is meant to be applied against a spidermonkey release. Mozilla-Central does not seem to hold 'js' sources appropriate for an actual spidermonkey release, and so this patch will not apply cleanly there. The patch as-is applies to js185 , and will with minor modification apply to js187 as proposed in https://bugzilla.gnome.org/show_bug.cgi?id=678410 .. there seems to be no way to nicely substitute the library soname via Preprocessor.py into symverscript (since there is no defined source of the JS_VERSION in configure, etc), so the symverscript.in file will need to be adjusted by hand.

oops, I meant the js187 as proposed in bug 735599

I added a forward-port of the spidermonkey patch (as best i could do) to make it work with the mozilla-central codebase -- symbol version is set to MOZJS_EXT instead of something that would be version-specific to a particular spidermonkey release; this will suffice to keep from conflicting against libxul.

Comment on attachment 679705 [details] [diff] [review] Patch to merge to mozilla-central >+#filter substitution >+@MOZ_APP_BASENAME@_@MOZ_APP_VERSION@ { >+ global: JS_*; js_*; _Z[NTVK0-9]*JS*; _Z[NTVK0-9]*js*; Please put each on a separate line. >--- a/mozilla/toolkit/library/Makefile.in 2012-11-07 11:08:34.000000000 -0500 >+++ b/mozilla/toolkit/library/Makefile.in 2012-11-07 11:08:15.000000000 -0500 >@@ -120,6 +120,13 @@ > ifeq (Linux,$(OS_ARCH)) > ifneq (Android,$(OS_TARGET)) > OS_LIBS += -lrt >+EXTRA_DSO_LDOPTS += -Wl,-version-script,symverscript >+ >+symverscript: symverscript.in >+ $(PYTHON) $(topsrcdir)/config/Preprocessor.py -DMOZ_APP_BASENAME="$(MOZ_APP_BASENAME)" -DMOZ_APP_VERSION="$(MOZ_APP_VERSION)" $< > $@ >+ >+EXTRA_DEPS += symverscript >+ > endif > endif > >--- a/mozilla/js/src/symverscript.in 2012-11-07 09:36:16.000000000 -0500 >+++ b/mozilla/js/src/symverscript.in 2012-11-07 09:37:05.000000000 -0500 >@@ -0,0 +1,5 @@ >+#filter substitution >+MOZJS_EXT { You're not making any substitution here, which effectively will literally use "MOZJS_EXT" as the soversion. Also, MOZJS_EXT is not defined in js/src without the standalone tarball patches (i guess that's where it comes from), which would break building as standalone without them (and we do that on our buildbots). >+# ensure symbol versions of shared library on linux do not conflict with those in libxul >+ifeq (Linux,$(OS_ARCH)) >+ifneq (Android,$(OS_TARGET)) ifeq (Linux,$(OS_TARGET)) is enough. >+OS_LIBS += -lrt Why add this? This seems unrelated.

(In reply to Mike Hommey [:glandium] from comment #10) > Comment on attachment 679705 [details] [diff] [review] > Patch to merge to mozilla-central > > >+#filter substitution > >+@MOZ_APP_BASENAME@_@MOZ_APP_VERSION@ { > >+ global: JS_*; js_*; _Z[NTVK0-9]*JS*; _Z[NTVK0-9]*js*; > > Please put each on a separate line. *nod* > >--- a/mozilla/js/src/symverscript.in 2012-11-07 09:36:16.000000000 -0500 > >+++ b/mozilla/js/src/symverscript.in 2012-11-07 09:37:05.000000000 -0500 > >@@ -0,0 +1,5 @@ > >+#filter substitution > >+MOZJS_EXT { > > You're not making any substitution here, which effectively will literally > use "MOZJS_EXT" as the soversion. > Also, MOZJS_EXT is not defined in js/src without the standalone tarball > patches (i guess that's where it comes from), which would break building as > standalone without them (and we do that on our buildbots). > MOZJS_EXT is entirely arbitrary, i chose it because it seemed an appropriate string for the standalone library. As long as it is different from "@MOZ_APP_BASENAME@_@MOZ_APP_VERSION@" for every mozilla product this is all that matters. It has no relation to the sources. As mentioned in the comments when i posted this patch, there is currently no appropriate spidermonkey version identifiers within configure.in in js/src , so there is nothing to use for an appropriate substutition. I added the #filter substitution directive just to supress the WARNING from Preprocessor.py > >+# ensure symbol versions of shared library on linux do not conflict with those in libxul > >+ifeq (Linux,$(OS_ARCH)) > >+ifneq (Android,$(OS_TARGET)) > > ifeq (Linux,$(OS_TARGET)) is enough. > Sure. > >+OS_LIBS += -lrt > > Why add this? This seems unrelated. ..this is probably cruft I accidentally copied from the patch against the JS185 distfile. I'll remove it.

(In reply to Ian Stakenvicius from comment #11) > MOZJS_EXT is entirely arbitrary, i chose it because it seemed an appropriate > string for the standalone library. As long as it is different from > "@MOZ_APP_BASENAME@_@MOZ_APP_VERSION@" for every mozilla product this is all > that matters. It has no relation to the sources. As mentioned in the > comments when i posted this patch, there is currently no appropriate > spidermonkey version identifiers within configure.in in js/src , so there is > nothing to use for an appropriate substutition. But then, if two system libraries pull a different version of libmozjs, they will both have the same soversion, and we're back to square one (of if firefox is built with libmozjs as a shared library, which is what i do in debian, for instance) > I added the #filter substitution directive just to supress the WARNING from Preprocessor.py Note that if you don't need substitution, then you can just use the file without preprocessing it. But you need substitution anyways, to use some kind of version for the soversion.

(In reply to Mike Hommey [:glandium] from comment #12) > (In reply to Ian Stakenvicius from comment #11) > > MOZJS_EXT is entirely arbitrary, i chose it because it seemed an appropriate > > string for the standalone library. As long as it is different from > > "@MOZ_APP_BASENAME@_@MOZ_APP_VERSION@" for every mozilla product this is all > > that matters. It has no relation to the sources. As mentioned in the > > comments when i posted this patch, there is currently no appropriate > > spidermonkey version identifiers within configure.in in js/src , so there is > > nothing to use for an appropriate substutition. > > But then, if two system libraries pull a different version of libmozjs, they > will both have the same soversion, and we're back to square one (of if > firefox is built with libmozjs as a shared library, which is what i do in > debian, for instance) > Yep, that's a problem. And I don't think that this case (two copies of libmozjs*.so) is supportable at all -- you are bound to only having one copy of the libmozjs###.so shared lib and all must link to that. This bug and patch only provides the solution for collision between libmozjs###.so and libxul.so (mainly in terms of how it's built and distributed in binary form direct from mozilla). To fix this, there needs to be some versioning defined and implemented/added to configure.in so that it can be substituted appropriately in the first place. Proper SONAME based versioning wouldn't hurt, either. That's beyond the scope of this bug/patch though as its something the dev team in charge of js/src need to sort out.

(In reply to Ian Stakenvicius from comment #13) > Yep, that's a problem. And I don't think that this case (two copies of > libmozjs*.so) is supportable at all -- you are bound to only having one copy > of the libmozjs###.so shared lib and all must link to that. It's bound to happen when you transition from libmozjs version x to libmozjs version x+1. You won't necessarily have updated all packages that depend on version x.

(In reply to Mike Hommey [:glandium] from comment #14) > (In reply to Ian Stakenvicius from comment #13) > > Yep, that's a problem. And I don't think that this case (two copies of > > libmozjs*.so) is supportable at all -- you are bound to only having one copy > > of the libmozjs###.so shared lib and all must link to that. > > It's bound to happen when you transition from libmozjs version x to libmozjs > version x+1. You won't necessarily have updated all packages that depend on > version x. I'm not saying this wouldn't be nice and good; I'm saying that, based on how things are established within the codebase in mozilla-central, that this isn't a supported option. As there hasn't been any releases of spidermonkey since 1.8.5 (libmozjs185), there isn't any established history to fall back on (previous versions don't count as they had proper soname and were also at least API-compatible with one-another). I can say that Gentoo doesn't support having multiple spidermonkeys installed concurrently, and new versions of spidermonkey are masked (excluded from the package manager) until everything that depends on it can build against the new version; too much breakage occurs otherwise. I would expect with a binary distro the issue would be the same -- that is, everything for a current "version" would need to be built homogeneously against the same libmozjs and mixing between "versions" of the distro would not be supported. I very much agree it SHOULD be supported. But as I said, supporting this requires the dev's in charge to make a design decision on how they're going to implement versioning in js/src, that imo is beyond the scope of this bug. Along similar lines, since there doesn't seem to be a release of spidermonkey on the horizon (bug 735599 has had no movement in some time), getting the patch into place as it stands and as soon as possible (ie for FF17) would still be a significant improvement over the status-quo. If you prefer, though, I can make something up and propose it in the next patch??

Update, incorporating requested changes. I changed js/src/symverscript.in so that it substitutes LIBRARY_NAME -- after substitution the result will be identical to the previous patch, but when diff'ing Makefile.in between mozilla-central and the js185 distfile, LIBRARY_VERSION does differ (it is "mozjs185" in the distfile); so theoretically this variable is what will be updated to differentiate the library version for every release.

fixed the syntax of the symbol version script for libxul (other one worked but was not the same form that i since found in other packages)

Comment on attachment 681995 [details] [diff] [review] Patch to merge to mozilla-central Review of attachment 681995 [details] [diff] [review]: ----------------------------------------------------------------- I'd like someone from the js team to look at the list of symbols from mozjs we'd export with versions, whether that's enough or if there are other patterns to add. ::: mozilla/toolkit/library/symverscript.in @@ +1,2 @@ > +#filter substitution > +@MOZ_APP_BASENAME@_@MOZ_APP_VERSION@ { It would probably be better to make that XUL_@MOZ_APP_VERSION@ or GECKO_@MOZ_APP_VERSION@. @@ +1,3 @@ > +#filter substitution > +@MOZ_APP_BASENAME@_@MOZ_APP_VERSION@ { > +global: JS_*; Can you put "JS_*" on a separate line, and indent the remaining items? @@ +2,5 @@ > +@MOZ_APP_BASENAME@_@MOZ_APP_VERSION@ { > +global: JS_*; > +js_*; > +_Z[NTVK0-9]*JS*; > +_Z[NTVK0-9]*js*; While looking in ld documentation what characters are legal for versions, I found out that we could use something like: extern "C++" { js::*; JS::*; }; which would make things much clearer. Can you test this works?

(In reply to Mike Hommey [:glandium] from comment #19) > > I'd like someone from the js team to look at the list of symbols from mozjs > we'd export with versions, whether that's enough or if there are other > patterns to add. > [ Snip! ] > @@ +2,5 @@ > > +@MOZ_APP_BASENAME@_@MOZ_APP_VERSION@ { > > +global: JS_*; > > +js_*; > > +_Z[NTVK0-9]*JS*; > > +_Z[NTVK0-9]*js*; > > While looking in ld documentation what characters are legal for versions, I > found out that we could use something like: > extern "C++" { > js::*; > JS::*; > }; > > which would make things much clearer. Can you test this works? In theory it does work, sort of. A symverscript in the spidermonkey-1.8.5 codebase will get all but four symbols via ' global: js_*; JS_*; extern "C++" { JS*; js*; }; ' , plus those symbols it doesn't get also does not exist in libxul from FF16.0.2 codebase. However, using that match for js/src in the FF16.0.2 codebase leaves 93 symbols that aren't caught. The original match isn't perfect either though as it leaves 24 symbols unmatching. Also something to consider is that ' extern "C++" { JS*; js*; }; ' for libxul.so seems to match most (maybe all) of the symbols from js/jsd , which are unnecessary. In short, I can't find a nice simple way to match these symbols. I'm now looking into finding a way to generate symverscript from code so that it contains the full list of symbols from js/src; barring that I recommend versioning all symbols in both libxul and libmozjs with a 'global: *' , despite the additional file size it will add to libxul (which seems to be close to 2MB). > ::: mozilla/toolkit/library/symverscript.in > @@ +1,2 @@ > > +#filter substitution > > +@MOZ_APP_BASENAME@_@MOZ_APP_VERSION@ { > > It would probably be better to make that XUL_@MOZ_APP_VERSION@ or > GECKO_@MOZ_APP_VERSION@. > Probably substituting @LIBRARY_NAME@ for both js/src/symverscript.in and toolkit/library/symverscript.in would be a good fit.

Given the issues with trying to match all mozjs symbols; and Given that whether there is symbol versioning on one symbol or all of them, the stripped libxul.so is identical in size (ELF,linux,gcc-4.5 toolchain); Request that toolkit/library/symverscript.in include "global: *;" and be done with it. Friendly?

per above comments: - substitute LIBRARY_NAME instead of MOZ_APP_NAME - version all symbols because matching just the JS ones are proving to be impossible in the generic sense and there seems to be no drawback to versioning all of them versus just a few. Did not add dmandelin to review because all symbols are versioned so not much need to evaluate if it's a complete list or not

Comment on attachment 679299 [details] [diff] [review] Updated patch to FF (etc) acceptable for submission main patch obsoletes this one

minor update to symbol version definitions to fix a potential syntax error (it didn't play nice with the proposed patch for bug 812265, now it'll apply clean and permanent)

Comment on attachment 687267 [details] [diff] [review] add symbol-versions to libxul and libmozjs for linux (m-c and tagged releases) Review of attachment 687267 [details] [diff] [review]: ----------------------------------------------------------------- ::: mozilla/toolkit/library/symverscript.in @@ +123,5 @@ > +EXTRA_DSO_LDOPTS += -Wl,-version-script,symverscript > + > +symverscript: symverscript.in > + $(PYTHON) $(topsrcdir)/config/Preprocessor.py \ > + -DLIBRARY_NAME="$(subst -,_,$(LIBRARY_NAME))" \ It would be better with something like -DVERSION="$(LIBRARY_NAME)$(MOZILLA_VERSION)" here and -DVERSION="$(subst -,_,$(LIBRARY_NAME))" in js/src. (no need for the subst in toolkit/library, LIBRARY_NAME is either xul or XUL) Other than this nit, this is good for me, after carefully testing the effects of such a version script.

same patch with the versioning adjusted to address the nit.

Comment on attachment 687787 [details] [diff] [review] add symbol-versions to libxul and libmozjs for linux (m-c and tagged releases) nit free --- a/mozilla/toolkit/library/Makefile.in 2012-11-07 11:08:34.000000000 -0500 +++ b/mozilla/toolkit/library/Makefile.in 2012-11-07 11:08:15.000000000 -0500 @@ -120,6 +120,14 @@ ifeq (Linux,$(OS_ARCH)) ifneq (Android,$(OS_TARGET)) OS_LIBS += -lrt +EXTRA_DSO_LDOPTS += -Wl,-version-script,symverscript + +symverscript: symverscript.in + $(PYTHON) $(topsrcdir)/config/Preprocessor.py \ + -DVERSION="$(LIBRARY_NAME)$(MOZILLA_VERSION)" $< > $@ + +EXTRA_DEPS += symverscript + endif endif Try builds have been run for all everything green other then reftest failure not related to patch.

I backed out https://hg.mozilla.org/integration/mozilla-inbound/rev/75e0d521bc2b due to the bug 819172 regression. Clearly none of us there had any idea what the right fix is, best to punt back to the original author. :-) FYI, the steps to reproduce the issue reported there go like this. Have a clean tree with that rev in it, then: cd js/src autoconf-2.13 mkdir objdir cd objdir ../configure --enable-debug --disable-optimize make

Yeah I don't know why this happens. 'make all' works as expected, but 'make' on it's own doesn't. Glandium mentioned that this was a bug that was already fixed, but as far as I can tell, the default target needs to be set in the makefile. Note that this works fine in 17ESR but not in HEAD..

Same patch, but doesn't override the default target for js/src's Makefile in HEAD; allows patch to be applied again without causing bug 819172 to re-occur

ACK. No, this doesn't work -- symverscript target currently fails to build when patch is used against latest m-c, so all builds will fail. Hold off pls, i'll figure it out.

In the future, you probably want to "checkin-needed" keyword.

OK, the solution seems to be to make the symverscript target always be present at the end of the file, while placing the LDOPTS addition and EXTRA_DEPS inclusion where it originally was in the first patch. It worked locally, but if glandium or another dev can confirm before checking it in again I would appreciate it. I'd rather not generate another bug if possible.

(swapped XUL_ for @LIBRARY_VERSION@ -- accidental regression)

Comment on attachment 690168 [details] [diff] [review] add symbol versions and have correct target location in Makefile.in to support default 'make' Review of attachment 690168 [details] [diff] [review]: ----------------------------------------------------------------- If you put the rule after the default rule, and before including rules.mk, you can group the rule and the flags.

I think this has broken |make -C toolkit/library| for me. With this patch, > $ rm -rf toolkit/library/libxul.so > $ make -C toolkit/library > make: Entering directory `/home/jlebar/code/moz/ff-git/debug/toolkit/library' > make: `symverscript' is up to date. > make: Leaving directory `/home/jlebar/code/moz/ff-git/debug/toolkit/library' without this patch, that works fine. We should probably back this out, unless there's a new way to build toolkit/library.

'make libs -C toolkit/library' works. (It would still be nice to fix 'make -C toolkit/library', though.)

> 'make libs -C toolkit/library' works. That's good enough for me! Thanks.

I just wasted a ton of time because my normal workflow was not remaking libxul :-(. This has also broken the new smartmake functionality of "mach build <dir>".

I mailed dev-platform so that hopefully nobody else ends up wasting time because of this.

Or we could just fix it.

Comment on attachment 745374 [details] [diff] [review] fix building in toolkit/library I'm going to pass rubber-stamping duties off to gps here.

Comment on attachment 745374 [details] [diff] [review] fix building in toolkit/library Review of attachment 745374 [details] [diff] [review]: ----------------------------------------------------------------- Gotta love ordering of rules in make files. ::: toolkit/library/Makefile.in @@ +695,5 @@ > libs:: $(FINAL_TARGET)/dependentlibs.list > > $(FINAL_TARGET)/dependentlibs.list: dependentlibs.py $(SHARED_LIBRARY) $(wildcard $(if $(wildcard $(FINAL_TARGET)/dependentlibs.list),$(addprefix $(FINAL_TARGET)/,$(shell cat $(FINAL_TARGET)/dependentlibs.list)))) > $(PYTHON) $< $(SHARED_LIBRARY) -L $(FINAL_TARGET) $(if $(TOOLCHAIN_PREFIX),$(addprefix -p ,$(TOOLCHAIN_PREFIX))) > $@ > + Nit: Whitespace

Comment on attachment 745374 [details] [diff] [review] fix building in toolkit/library Review of attachment 745374 [details] [diff] [review]: ----------------------------------------------------------------- Please don't land this. Please instead file a new bug, and figure out why .DEFAULT_GOAL is not working in toolkit/library.

I reopened bug 777379 because our .DEFAULT_GOAL usage appears to be wrong. The way I see it, this regression is high developer impact and a fix is available in this bug. I'm not sure we'll fix bug 777379 quickly. Therefore, you have my permission (as build module owner) to land this trivial fix.

(In reply to Gregory Szorc [:gps] from comment #48) > The way I see it, this regression is high developer impact and a fix is > available in this bug. I'm not sure we'll fix bug 777379 quickly. A patch is up already.