Adam Guthrie Reporter
	Description • 18 years ago

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20061203 Minefield/3.0a1

Steps to reproduce:
1. Load the testcase.

Actual results:
Nothing is displayed.

Expected results:
A dash is displayed.

Bugzilla uses this character in its flag names; e.g. "in-testsuite".

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 2 • 18 years ago

I have two questions:

1. Do you have 'Arial' font in your system? (Fedora Core doesn't have it.)
2. Your Arial font has the glyph for U+2011? On WinXP, Arial font doesn't have it.

	Adam Guthrie Reporter
	Comment 3 • 18 years ago

I installed the MS True Type Core Fonts [0], so yes, I do have Arial installed. This could occur with other fonts, too; Arial is just the one that I encountered it with.

I'm under the assumption that Arial supports that glyph because before the patch for bug 352174 landed my testcase passes.

[0] http://zeuscat.com/andrew/software/corefonts/

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 4 • 18 years ago

Can you attach a screenshot?

	Adam Guthrie Reporter
	Comment 5 • 18 years ago

I'm using Ubuntu Edgy Eft, for what it's worth.

	Adam Guthrie Reporter
	Comment 6 • 18 years ago

Masayuki, do you have any idea what's going on here? Maybe you could install these fonts yourself and test? This issue isn't just isolated to the non-breaking hyphen character, it happens for characters, too. 

Other applications like gedit and OpenOffice display this character just fine, so I have a feeling it's a bug in our code. 

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 7 • 18 years ago

The screenshot wasn't clear -- that's a screenshot of it working before the patch went in.  I do not have Arial installed and I see the hyphen in the testcase under ubuntu feisty -- I have not tried with Arial.  ?'ing again to bring this up when we do triage.

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 9 • 17 years ago

Ah, font fallback was broken when the linux code was redone.  Stuart's working on this; not sure what the bug # is, but this'll be fixed by his work.

	Stuart Parmenter Assignee
	Comment 10 • 17 years ago

this should fix this and most other linux text bugs. haven't done any perf tests and we probably need to append cjk pref fonts when the script is cjk and there are a few XXX comments to address.  Would be good to get some testing with this patch though.

	Michael Wu [:mwu]
	Comment 11 • 17 years ago

Tested this patch. It makes the test case work again, but.. it doesn't seem to work so well on Chinese now.

	Stuart Parmenter Assignee
	Comment 13 • 17 years ago

this is a mix of my patch and behdad's patch and his cairo patch that should basically result in us doing the right thing.

this patch makes an assumptiont that we don't use our own font prefs and that we use fontconfig instead which i think we're going to go with.

	Behdad Esfahbod
	Comment 14 • 17 years ago

Ok, uploaded an updated patch here:

  http://www.gnome.org/~behdad/ff3/

It's the same as yours, just some clarifications added to XXX entries.  No code changes.

	Damon Sicore (:damons)
	Comment 15 • 17 years ago

Targeting this to Beta 1 (M9) per discussion with Stuart.

	Behdad Esfahbod
	Comment 17 • 17 years ago

This version doesn't try to use pango_cairo_font_get_scaled_font() when it's available.  That may be desirable for upstream, but certainly not for Linux distributions.  Just something to know.

Alternative would be to do those nasty hacks to detect whether it's available at run time.

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 18 • 17 years ago

Comment on attachment 281546 [details] [diff] [review]
ok, lets go with this

r=me with misc changes we discussed; can you file a bug on the out-of-order usage of pref fallback fonts?

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 20 • 17 years ago

Comment on attachment 281551 [details] [diff] [review]
argh, wrong patch.

Update looks fine, but is missing the configure changes from the previous patch

	Stuart Parmenter Assignee
	Comment 21 • 17 years ago

ok, since i can't seem to post all the right patches, we want the last patch i posted with the configure stuff from the previous one.  behdad: you're right -- i couldn't get the ifs there to actually compile.  We should follow up with that with a seperate patch.

	Stuart Parmenter Assignee
	Comment 22 • 17 years ago

linux tinderboxen haven't been updated to the ref platform.

	Karl Tomlinson (:karlt)
	Comment 23 • 17 years ago

> gfxPangoFontGroup::CanTakeFastPath(PRUint32 aFlags)
> {
>    if (!PANGO_IS_FC_FONT (GetFontAt(0)->GetPangoFont ()))

Can we do this after checking the flags so we don't realize the font until necessary?

When is it not FC_FONT?

	Behdad Esfahbod
	Comment 24 • 17 years ago

(In reply to comment #23)
> > gfxPangoFontGroup::CanTakeFastPath(PRUint32 aFlags)
> > {
> >    if (!PANGO_IS_FC_FONT (GetFontAt(0)->GetPangoFont ()))
> 
> Can we do this after checking the flags so we don't realize the font until
> necessary?
> 
> When is it not FC_FONT?

When run against Pango's win32 or atsui backends.  You probably don't care about it, but the code is correct this way.

	Boris Zbarsky [:bzbarsky]
	Comment 25 • 17 years ago

If this changes are minimum runtime or compile-time pango versions, please post to .announce or some such with a note about it so people don't get surprised, ok?

	Stuart Parmenter Assignee
	Comment 26 • 17 years ago

the ref platform is pango 1.14 -- this only bumps us to 1.10.  I will post there as well though.

	Boris Zbarsky [:bzbarsky]
	Comment 27 • 17 years ago

Note also that tree rules are to not break Seamonkey or Thunderbird on Tier 1 platforms (including Linux), and some those tinderboxen are not on the ref platform yet (e.g. cb-sea-linux-tbox is on CentOS 4.4, as is tb-linux-tbox).  You probably want to get those updated.

	Stuart Parmenter Assignee
	Comment 28 • 17 years ago

they are in the process of being updated and should be done in the next couple days.  see the bugs this depends on.

	David Baron :dbaron:
	Comment 29 • 17 years ago

So Stuart back this out due to leak regression and a startup time regression -- although it's worth noting that the maximum heap usage improved.  (Though the total number of allocations went *way* up.)

I'd also note that when I tried the patch, underlines appeared too close to text.

	David Baron :dbaron:
	Comment 30 • 17 years ago

This is an allocation stack tree by bytes leaked, trimmed to show only branches of at least 10000 bytes.

It's the memory that was not freed when starting up and shutting down the browser (and displaying the Firefox+Google start page).

	Walter Meinl
	Comment 31 • 17 years ago

>-GTK2_VERSION=1.3.7
>+PANGO_VERSION=1.10.0
>+GTK2_VERSION=1.8.0
Looking at this change in version number of gtk+ I'm asking if this shouldn't be
>-GTK2_VERSION=1.3.7
>+PANGO_VERSION=1.10.0
>+GTK2_VERSION=2.8.0

	Karl Tomlinson (:karlt)
	Comment 32 • 17 years ago

(In reply to comment #31)
> >+GTK2_VERSION=2.8.0

Looks more reasonable but I can't see anything that should require 2.8.0.

	Behdad Esfahbod
	Comment 33 • 17 years ago

(In reply to comment #32)
> (In reply to comment #31)
> > >+GTK2_VERSION=2.8.0
> 
> Looks more reasonable but I can't see anything that should require 2.8.0.

I think there may be some assumption that the fontmap used by Gtk+ is a pangocairo one.

	Karl Tomlinson (:karlt)
	Comment 34 • 17 years ago

Including a call to cairo_debug_reset_static_data() after gdk_display_close() reduces the shutdown leak byte count on resource:///res/bloatcycle.html from 7151329 to 2424366, which is pretty close to the 2349362 bytes without this patch (with pangoxft).

	Jim Jeffery not reading bug-mail 1/2/11
	Comment 35 • 17 years ago

Following the checkin the Lk numbers on the Linux box went up.
Linux fxdbug-linux-tbox Dep
Before: Lk:1.41MB
After:  Lk:3.45MB


  

	Hussam Al-Tayeb
	Comment 36 • 17 years ago

This is causing a failure in running my builds.

./firefox
GetResolvedFonts
GetResolvedFonts
GetResolvedFonts
GetResolvedFonts
GetResolvedFonts
GetResolvedFonts
GetResolvedFonts
GetResolvedFonts
./run-mozilla.sh: line 131:  6128 Segmentation fault      "$prog" ${1+"$@"}

	Hussam Al-Tayeb
	Comment 37 • 17 years ago

I forgot to mention that I'm using pango 1.18.2

	Heinrich Witt
	Comment 38 • 17 years ago

Same here  with pango 1.18.2 :

./firefox
GetResolvedFonts
GetResolvedFonts
GetResolvedFonts
GetResolvedFonts
./run-mozilla.sh: line 131: 17380 Speicherzugriffsfehler  "$prog" ${1+"$@"}

	Stuart Parmenter Assignee
	Comment 39 • 17 years ago

(In reply to comment #35)
> Following the checkin the Lk numbers on the Linux box went up.
> Linux fxdbug-linux-tbox Dep
> Before: Lk:1.41MB
> After:  Lk:3.45MB
> 

These leaks are in Pango and/or Freetype.  Behdad and I dug in to them some yesterday and we're both going to continue digging but they aren't in our code and there isn't much we're going to be able to do about them short of saying upgrade your system libraries.

	Stuart Parmenter Assignee
	Comment 40 • 17 years ago

(In reply to comment #36)
> This is causing a failure in running my builds.
> 
> ./firefox
> GetResolvedFonts
> GetResolvedFonts
> GetResolvedFonts
> GetResolvedFonts
> GetResolvedFonts
> GetResolvedFonts
> GetResolvedFonts
> GetResolvedFonts
> ./run-mozilla.sh: line 131:  6128 Segmentation fault      "$prog" ${1+"$@"}
> 

Do any of you have a stack trace?  I'm pretty sure that behdad was testing with newer versions.  I only have 1.14 and 1.16 on my box.. I can build 1.18 soon...

	Hussam Al-Tayeb
	Comment 41 • 17 years ago

I reverted to an old firefox3 build so I can add this comment. I'll try to make a debug build against pango 1.18.2

	Heinrich Witt
	Comment 42 • 17 years ago

With a new build (2007-10-06 21:00 PDT) i got only: 

./run-mozilla.sh: line 131: 17380 Segmentation fault  "$prog" ${1+"$@"}

	Frederic Bezies
	Comment 43 • 17 years ago

Same here. Debug build gives no more info.

Could it be possible to back out this patch ?

It makes impossible to run a freshly made firefox on very recent linux distros.

I am using Ubuntu Gutsy Gibbon - I know, still beta - but I think OpenSuSE 10.3, Mandriva 2008.0 will be affected to, because of Pango 1.18.2 :(

	Robert Kaiser
	Comment 44 • 17 years ago

The crash at startup may very well be bug 398512, which is a different problem (note that despite the bug being about shutdown, the restart sequence we do at startup does triggers this in most cases).

The patch here probably only causes the leaks mentioned here, which are known, but not the startup crash.

	Frederic Bezies
	Comment 45 • 17 years ago

I don't think so. I could get working build until 5 of october, and bug mentionned was opened on 3 of october. And it is a shutdown crash, not a startup one.

Anyway the problem stay the same : no working starting build with pango 1.18.2 :(

	Boris Zbarsky [:bzbarsky]
	Comment 46 • 17 years ago

If you have a debug build, please run:

  firefox -g

then type "run" at the gdb prompt.  After the crash, type "bt" and put the resulting stacktrace in the bug?

	Walter Meinl
	Comment 47 • 17 years ago

(In reply to comment #46)
> If you have a debug build, please run:
> 
>   firefox -g
> 
> then type "run" at the gdb prompt.  After the crash, type "bt" and put the
> resulting stacktrace in the bug?
> 

stacktraces are attached to bug39885
I saw the same problem with pango-1.18.2 crash, compiled pango-1.16.5, then I saw with the same firefox build
symbol lookup error: /usr/lib64/mozilla-minefield/libxul.so: undefined symbol: pango_cairo_font_get_scaled_font
recompiled firefox against pango-1.16.5 and now it works

	Walter Meinl
	Comment 48 • 17 years ago

sorry, my bad missed an "8" stacktraces are attached to bug398885

	Fabien Tassin
	Comment 49 • 17 years ago

Here is a stack trace of a crash in xulrunner (libxul) when using webrunner.
It seems it started between 2007-10-05 10:06 and 2007-10-06 10:14 leading me to this bug.

	Frederic Bezies
	Comment 50 • 17 years ago

Here is another trace. I build firefox with this .mozconfig :

#
# See http://www.mozilla.org/build/ for build instructions.
#

export CC=gcc-4.2
export CXX=g++-4.2

. $topsrcdir/browser/config/mozconfig

# Options for 'configure' (same as command-line options).
ac_add_options --enable-default-toolkit=cairo-gtk2

	Caio Tiago Oliveira (asrail)
	Comment 51 • 17 years ago

(In reply to comment #44)
> The crash at startup may very well be bug 398512, which is a different problem
> (note that despite the bug being about shutdown, the restart sequence we do at
> startup does triggers this in most cases).
> 
> The patch here probably only causes the leaks mentioned here, which are known,
> but not the startup crash.
> 

No, Kairo.
I've reverted the commits related to this bug and I can confirm they cause this crash.

Bug 398885 is about this.

(In reply to comment #47)
> (In reply to comment #46)
> > If you have a debug build, please run:
> > 
> >   firefox -g
> > 
> > then type "run" at the gdb prompt.  After the crash, type "bt" and put the
> > resulting stacktrace in the bug?
> > 
> 
> stacktraces are attached to bug39885
> I saw the same problem with pango-1.18.2 crash, compiled pango-1.16.5, then I
> saw with the same firefox build
> symbol lookup error: /usr/lib64/mozilla-minefield/libxul.so: undefined symbol:
> pango_cairo_font_get_scaled_font
> recompiled firefox against pango-1.16.5 and now it works
> 

This undefined symbol is another issue... there is a check of the pango's version here:
http://mxr.mozilla.org/seamonkey/source/gfx/thebes/src/gfxPangoFonts.cpp#644

	Boris Zbarsky [:bzbarsky]
	Comment 52 • 17 years ago

I have no idea how this compiled for anyone (including tinderbox).  When I compile it here (against pango 1.12), I get:

../../../../mozilla/gfx/thebes/src/gfxPlatformGtk.cpp:278: error: ‘pango_cairo_context_get_resolution’ was not declared in this scope

which is not surprising.  This patch fixes that problem.

	David Baron :dbaron:
	Comment 53 • 17 years ago

The reason it compiled here is the following chain of #includes:

gfxPlatformGtk.cpp
gtk/gtk.h
gdk/gdk.h
gdk/gdkcairo.h
pango/pangocairo.h

	Boris Zbarsky [:bzbarsky]
	Comment 54 • 17 years ago

Ah, right.  I'm not compiling against a new enough GTK that it would use cairo, as far as I can tell.

	Walter Meinl
	Comment 55 • 17 years ago

(In reply to comment #51)
> (In reply to comment #47)
> > (In reply to comment #46)
> > saw with the same firefox build
> > symbol lookup error: /usr/lib64/mozilla-minefield/libxul.so: undefined symbol:
> > pango_cairo_font_get_scaled_font
> > recompiled firefox against pango-1.16.5 and now it works
> > 
> 
> This undefined symbol is another issue... there is a check of the pango's
> version here:
> http://mxr.mozilla.org/seamonkey/source/gfx/thebes/src/gfxPangoFonts.cpp#644
> 
firefox built against pango-1.16.5 won't crash even if you afterwards bump pango to 1.18.2 again. So pango_cairo_font_get_scaled_font that is only used when you compile against pango >=1.17.5 seems to be a candidate for the crasher

	Boris Zbarsky [:bzbarsky]
	Comment 56 • 17 years ago

Comment on attachment 283926 [details] [diff] [review]
Fix build bustage

Checked this in.

	Hideo Oshima
	Comment 57 • 17 years ago

(In reply to comment #56)
> (From update of attachment 283926 [details] [diff] [review])
> Checked this in.
 
I can start Firefox built with pango 1.18.2 now.
Thanks!

	Boris Zbarsky [:bzbarsky]
	Comment 58 • 17 years ago

That patch had no effect on that.  You're seeing the fix from bug 398885.

	Behdad Esfahbod
	Comment 59 • 17 years ago

The crash was caused by old/broken internal cairo in mozilla.  Works fine with system cairo.

	Fabien26
	Comment 60 • 17 years ago

Thank you guys, you have made a very good work, it work for me with (Ubuntu Gutsy gibbon) alpha 8 build of granparadiso !!

	Fabien Tassin
	Comment 61 • 17 years ago

(In reply to comment #59)
> The crash was caused by old/broken internal cairo in mozilla.  Works fine with
> system cairo.

not sure what you mean by old.

my system cairo is 1.4.10 (the latest released afaik) while from what I see, mozilla is now using 1.5.1 straight from cairo's git. I've disabled system cairo at/since a6 because of a crash on shutdown, mozilla was far behind then, now it's reversed.

	Marcia Knous [:marcia]
	Comment 62 • 17 years ago

verified fixed using  Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a9pre) Gecko/2007100804 Minefield/3.0a9pre. I used the testcase in the bug to verify.

	Walter Meinl
	Comment 64 • 17 years ago

I still don't see the reason for the gtk+-1.8.0 check in configure. According to ftp.gnome.org there is the 1.3.x series leading directly to 2.0.0, nothing in between so gtk+-1.8.0 never existed.

	Behdad Esfahbod
	Comment 65 • 17 years ago

Yes, that's supposed to be gtk+-2.8.0

	Boris Zbarsky [:bzbarsky]
	Comment 66 • 17 years ago

I'm still not sure what the point of checking that at compile time would be.  We don't depend on anything there at compile time, right?  If there is a dependency per comment 33, it's a runtime dependency, and then we should enforce it at runtime.

That said, I can run builds with this patch just fine against a gtk that's older than 2.8 (as well as compile against such a gtk), and I haven't seen obvious correctness problems.

	Behdad Esfahbod
	Comment 67 • 17 years ago

You are most probably right.  The main dependency is pangocairo.

	Boris Zbarsky [:bzbarsky]
	Comment 68 • 17 years ago

And that's enforced at build time via includes and at runtime because we link to it, yeah.

I have to admit to having a bias for not unconditionally bumping the gtk version dependency.  Updating pango is easy (I just installed 1.12 on a FC3 system).  Updating gtk is basically impossible, since so many GNOME packages hardcode a particular gtk version they depend on...

	Behdad Esfahbod
	Comment 69 • 17 years ago

(In reply to comment #68)
> Updating gtk is basically impossible, since so many GNOME packages
> hardcode a particular gtk version they depend on...

That doesn't sound right.  gtk doesn't change soname, so upgrading should be as simple as it is for pango.

	Boris Zbarsky [:bzbarsky]
	Comment 70 • 17 years ago

Hmm.  I just tried it again, and it seems that you might be right.  Once I commented out a bunch of BuildRequires in the spec file having to do with someone deciding to restructure the packages all the X stuff came from and do it incompatibly, I can at least compile it.  And it does look like other things only have it listed as a >= dep....

I think the last time I tried this I tried the binary package (which failed because of explicit deps on libc versions) and then tried the source package which failed because of the stuff above.

Good, good.  So I can keep compiling Mozilla even if we up the version dep.  ;)

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 71 • 17 years ago

This removed the setup of override codes in AppendDirectionalIndicatorUTF8. I put those in because I was told that otherwise characters in the textrun could override the Pango base direction. For example, we might have Hebrew text that has been forced to LTR by CSS or other means; Pango would then ignore the base direction and make it RTL, making our code very confused.

Simon, do we have automated tests for this situation? Is it still working since this patch landed? If so, how?

	Simon Montagu :smontagu
	Comment 72 • 17 years ago

I've just been preparing a bunch of reftests to catch any regressions that might be caused by bug 399850, and directional overrides certainly display OK on Pango, but they do seem to trigger the assertion "RTL assumption mismatch" at http://mxr.mozilla.org/seamonkey/source/gfx/thebes/src/gfxPangoFonts.cpp#1118.

An exception is Arabic shaping as per bug 402427 -- I wonder if putting back the override codes will fix that. 

Lots of the tests already checked in in layout/reftests/bidi work by comparing logical text without directional overrides to the expected reordering expressed with directional overrides, so there is very unlikely to be an unnoticed regression.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 73 • 17 years ago

> An exception is Arabic shaping as per bug 402427 -- I wonder if putting back
> the override codes will fix that.

Would you mind trying that?

I'm not sure how things are working without the override codes. That makes me uncomfortable.