440714 - Firefox 3 failed to start on Solaris with GNOME 2.23.3

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Ginn Chen]

WIth latest JDS vermillion installed, the bundled Firefox 3 could not be started.

The stack is like this,
--- called from signal handler with signal 11 (SIGSEGV) ---
 00000000 ???????? (fb38cdd8, 0, 0, fb385964, 0, ffbf7778)
 fb37c000 pixman_composite_rect_general (ffbf7778, ffbf7798, f7b79920, ffbf1700, f7b79920, 50) + ec
 fe01db7c pixman_image_composite_rect (1, 14, 0, 0, 0, 0) + c4
 fe01da7c pixman_walk_composite_region (0, 0, 0, 14, 0, 14) + cac
 fe01e618 _moz_pixman_image_composite (0, 1, 0, 14, 0, 0) + 600
 fdfcbafc _cairo_image_surface_composite (0, f78db640, 0, f8eacdf0, 0, 0) + 200
 fdfdd55c _cairo_surface_composite (f78db640, f78db640, 0, 1, 0, 0) + a0
 fdfdfa38 _composite_trap_region (0, f78db640, 14, f8eacdf0, 14, ffbfdfe8) + 1c4
 fdfdfe58 _clip_and_composite_trapezoids (f78db640, 0, f8eacdf0, ffbfe07c, 0, 1) + 328
 fdfe0044 _cairo_surface_fallback_paint (f8eacdf0, 1, f78db640, 14, 0, 14) + e4
 fdfdda34 _cairo_surface_paint (0, 0, fe7c5c28, f8eacdf0, fcdf7e47, 1) + 80

The cause is there're some global  "C" functions of pixman in libxul.so conflict with /usr/lib/libcario.so.

We have cairo-rename.h to deal with this issue.
But it doesn't define all the functions.

Also pixman-remap.h is not used by any program now. We can delete it.

Comment 1

[Ginn Chen]

Attachment: patch

Comment 2

[Ginn Chen]

Attachment: patch (update to trunk)

the removal of pixman-remap.h is done by other commits.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 331286 [details] [diff] [review]
patch (update to trunk)

Weird, those symbols shouldn't even be exported.. but this is fine for now.

Comment 4

[Ginn Chen]

Pushed
http://hg.mozilla.org/index.cgi/mozilla-central/rev/4691e4934089

BTW:
It's also reproducible with latest trunk on GNOME 2.22.x now.

For pixman-priviate.h, I didn't see anything stopping these functions being exported.

Comment 5

[Ginn Chen]

backed out, since it failed on Mac OS X.

Comment 6

[Ginn Chen]

For pixman, it is supposed to build with "-fvisibility=hidden", and
#define PIXMAN_EXPORT __attribute__ ((visibility("default")))
However, it's only for GCC 4.0 or higher, and it is turned off if MOZ_ENABLE_LIBXUL is not defined.

I don't know why pixman-mmx.c is broken by my patch yet.
I'll make a build to find out.

Comment 7

[Ginn Chen]

pixman-mmx.o is broken because it uses a special build rule in Makefile, which doesn't generate .deps file.

Comment 8

[Ginn Chen]

Attachment: patch v2

Changes:
1) rename mmx functions as well
2) Fix Makefile.in, use %.$(OBJ_SUFFIX) instead, so that
OS_COMPILE_CFLAGS $(filter-out %/.pp,-Wp,-MD,$(MDDEPDIR)/$(*F).pp) will work for these files.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

This is ok, the problem is that it's not maintainable -- the mmx functions and others should not be exported from any library.  If they are, that's a bug in the upstream.  There's no easy way to generate a rename script for the non-public methods in pixman, so there will always be a chance that some will be missed in the future.  Is there any way to fix the core problem without renaming?

Comment 10

[Ginn Chen]

I agree.

I don't think it's a bug in the upstream.
It's kind of 50% in the upstream C file, and 50% in our Makefile.in.

I need some time to address it.
What about we land this fix first, and file a followup bug to get rid of the root cause?

BTW: Do you think we need to take care of Mac OS X, Windows for this issue? There's unlikely a conflict of  cairo library on Mac OS X and Windows.

Comment 11

[Ginn Chen]

Attachment: patch v3

Use hidden attribute for pixman functions.
This patch works for me on Solaris, Linux, Mac OS X. I have only tried debug build.

I don't know why we have
ifndef MOZ_ENABLE_LIBXUL
VISIBILITY_FLAGS =
endif

pixman will link into libthebes.so.
So far, I don't have any problem with hidden attributes for pixman functions.

I don't know how VC works for exporting symbols, but this bug is unlikely reproducible on Windows anyway.

BTW: For the dep file of pixman-mmx.c issue, I'll file another bug to discuss.

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 333738 [details] [diff] [review]
patch v3

I think this is ok, but let's get benjamin to confirm -- also please send this off to the try servers to make sure that removing the visibility override in pixman doesn't break anything.

Comment 13

[Ginn Chen]

Comment on attachment 333738 [details] [diff] [review]
patch v3

I just found it's not enough.
If we set VISIBILITY_FLAGS for Sun Studio we will need more code like NP_VISIBILITY_DEFAULT to recover the visibility for Sun Studio.

It would be a little hard to find them all. :(

Comment 14

[Ginn Chen]

To summarize what's the root cause and what I'm going to do:

Cause:
1) In Solaris GNOME 2.22, 2.23, libcairo exports libpixman private functions as global.
I've fixed this bug in latest trunk.
See: http://bugs.freedesktop.org/show_bug.cgi?id=17183
2) Mozilla also exports libpixman private functions as global for Sun Studio compiler or disable-libxul.
3) When linking libxul.so, MOZ_PANGO_LIBS is ahead of MOZ_CAIRO_LIBS, therefore private functions in system cairo lib are possible to be binded, if bug 1) is present.

Fixes:
1) We always need VISIBILITY_FLAGS for libpixman. Switch the order in libxul-rules.mk to get around the Solaris system library bug.

2) Use __global and __hidden for Sun Studio compiler for all Mozilla modules. In general this is helpful to make Mozilla safer on Solaris. Mozilla private functions will not be visible outside of the module.
Some bugs will be gone, e.g.
https://bugzilla.mozilla.org/show_bug.cgi?id=437041

The patch is a little big. But I think it's safe, and I've tested on try-server.

Comment 15

[Ginn Chen]

Attachment: patch part1

Comment 16

[Ginn Chen]

Attachment: patch part2

For Sun Studio,
-xldscope=hidden equals to -fvisibility=hidden
__hidden equals to __attribute__((visibility("hidden")))
__global equals to __attribute__((visibility ("default")))

Sun Studio doesn't allow __hidden be followed by semicolon.
So I have to switch the order for some files.

Comment 17

[Benjamin Smedberg]

Comment on attachment 335331 [details] [diff] [review]
patch part2

I'm not sure whether

NS_HIDDEN void Method(); 

is allowed in GCC, but I'm sure that

nsIID NS_HIDDEN kIID;

is an incorrect declaration. The __attribute__((visibility("hidden")) would modify the *type* in that case, and not the declaration, which is not at all what we want.

Comment 18

[Benjamin Smedberg]

Comment on attachment 335330 [details] [diff] [review]
patch part1

I don't think the VISIBILITY_FLAGS logic is correct: in the non-libxul configuration, how would cairo link against pixman symbols (which are in a shared library?) without turning off the visibility flags?

In any case, this needs review by a thebes/gfx peer.

Comment 19

[Stuart Parmenter]

Comment on attachment 335330 [details] [diff] [review]
patch part1

vlad: what does this do to extensions?  anything?

Comment 20

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 335330 [details] [diff] [review]
patch part1

Shouldn't do anything -- extensions shouldn't be touching pixman or cairo directly anyway.  This part of the fix looks fine to me.

Comment 21

[Ginn Chen]

(In reply to comment #18)
> (From update of attachment 335330 [details] [diff] [review])
> I don't think the VISIBILITY_FLAGS logic is correct: in the non-libxul
> configuration, how would cairo link against pixman symbols (which are in a
> shared library?) without turning off the visibility flags?

In the non-libxul configuration, cairo and pixman are in one shared library -
libthebes.so.

Comment 22

[Ginn Chen]

(In reply to comment #17)
> (From update of attachment 335331 [details] [diff] [review])
> I'm not sure whether
> 
> NS_HIDDEN void Method(); 
> 
> is allowed in GCC

It is allowed and widely used in current Mozilla code.

>but I'm sure that 
> nsIID NS_HIDDEN kIID;
> 
> is an incorrect declaration. The __attribute__((visibility("hidden")) would
> modify the *type* in that case, and not the declaration, which is not at all
> what we want.
> 

I don't understand what you mean.

visibility is not a valid "type attribute", compiler will give a warning, if we use it for a type.

I wrote a test program on Mac OS X.
test.cpp:

typedef int T1;
T1 __attribute__((visibility("hidden"))) foo1;
T1 foo2 __attribute__((visibility("hidden")));
__attribute__((visibility("hidden"))) T1 foo3;
T1 foo4;

int main()
{ return 0; }

gcc test.cpp; nm -m a.out | grep foo

The result is,
0000203c (__DATA,__common) non-external (was a private external) _foo1
00002038 (__DATA,__common) non-external (was a private external) _foo2
00002034 (__DATA,__common) non-external (was a private external) _foo3
00002030 (__DATA,__common) external _foo4

Comment 23

[Benjamin Smedberg]

Currently, if you apply a decl attribute to a type, GCC will fix things for you by moving the attribute to the associated type. But this is incorrect usage and will probably change in future releases of GCC.

But I'm a little confused, having read up the bug some more: you're using -xldscope=hidden. Is that equivalent to GCC -fvisibility=hidden (i.e. it makes definitions hidden-visibility, but doesn't affect declarations?) Or is it more like the more intrusive #pragma visibility solution we use on more modern versions of GCC?

In either case, why do you need the extra NS_HIDDEN declarations?

Comment 24

[Ginn Chen]

I don't actually need it.
Actually GCC4 doesn't need it, either.

I put it in the patch in case some modules use global visibility by default, i.e. set VISIBILITY_FLAG to empty, e.g. cairo did it.

We can do it case by case anyway.

Comment 25

[Ginn Chen]

Attachment: patch part2 v2

No change to NS_HIDDEN

Comment 26

[Ginn Chen]

Pushed
http://hg.mozilla.org/mozilla-central/rev/8b5344112ce7