362682 - Some Unicode characters are no longer displayed with certain fonts (e.g. Arial)

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Adam Guthrie]

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".

Comment 1

[Adam Guthrie]

Attachment: testcase

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 3

[Adam Guthrie]

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/

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Can you attach a screenshot?

Comment 5

[Adam Guthrie]

Attachment: screenshot

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

Comment 6

[Adam Guthrie]

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. 

Comment 7

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

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.

Comment 9

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

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.

Comment 10

[Stuart Parmenter]

Attachment: v0.75?

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.

Comment 11

[Michael Wu [:mwu]]

Attachment: Results of patch

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

Comment 13

[Stuart Parmenter]

Attachment: fix

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.

Comment 14

[Behdad Esfahbod]

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.

Comment 15

[Damon Sicore (:damons)]

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

Comment 16

[Stuart Parmenter]

Attachment: ok, lets go with this

Comment 17

[Behdad Esfahbod]

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.

Comment 18

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

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?

Comment 19

[Stuart Parmenter]

Attachment: argh, wrong patch.

Comment 20

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

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

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

Comment 21

[Stuart Parmenter]

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.

Comment 22

[Stuart Parmenter]

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

Comment 23

[Karl Tomlinson (:karlt)]

> 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?

Comment 24

[Behdad Esfahbod]

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

Comment 25

[Boris Zbarsky [:bzbarsky]]

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?

Comment 26

[Stuart Parmenter]

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

Comment 27

[Boris Zbarsky [:bzbarsky]]

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.

Comment 28

[Stuart Parmenter]

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

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: allocation stack tree of bytes leaked

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

Comment 31

[Walter Meinl]

>-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

Comment 32

[Karl Tomlinson (:karlt)]

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

Comment 33

[Behdad Esfahbod]

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

Comment 34

[Karl Tomlinson (:karlt)]

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

Comment 35

[Jim Jeffery not reading bug-mail 1/2/11]

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


  

Comment 36

[Hussam Al-Tayeb]

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+"$@"}

Comment 37

[Hussam Al-Tayeb]

I forgot to mention that I'm using pango 1.18.2

Comment 38

[Heinrich Witt]

Same here  with pango 1.18.2 :

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

Comment 39

[Stuart Parmenter]

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

Comment 40

[Stuart Parmenter]

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

Comment 41

[Hussam Al-Tayeb]

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

Comment 42

[Heinrich Witt]

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

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

Comment 43

[Frederic Bezies]

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 :(

Comment 44

[Robert Kaiser]

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.

Comment 45

[Frederic Bezies]

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 :(

Comment 46

[Boris Zbarsky [:bzbarsky]]

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?

Comment 47

[Walter Meinl]

(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

Comment 48

[Walter Meinl]

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

Comment 49

[Fabien Tassin]

Attachment: stack trace of xulrunner crash while using webrunner

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.

Comment 50

[Frederic Bezies]

Attachment: Backtrace wanted.

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

Comment 51

[Caio Tiago Oliveira (asrail)]

(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

Comment 52

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix build bustage

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.

Comment 53

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 54

[Boris Zbarsky [:bzbarsky]]

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

Comment 55

[Walter Meinl]

(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

Comment 56

[Boris Zbarsky [:bzbarsky]]

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

Checked this in.

Comment 57

[Hideo Oshima]

(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!

Comment 58

[Boris Zbarsky [:bzbarsky]]

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

Comment 59

[Behdad Esfahbod]

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

Comment 60

[Fabien26]

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

Comment 61

[Fabien Tassin]

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

Comment 62

[Marcia Knous [:marcia]]

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.

Comment 63

[Karl Tomlinson (:karlt)]

Attachment: patch of what was committed

Comment 64

[Walter Meinl]

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.

Comment 65

[Behdad Esfahbod]

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

Comment 66

[Boris Zbarsky [:bzbarsky]]

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.

Comment 67

[Behdad Esfahbod]

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

Comment 68

[Boris Zbarsky [:bzbarsky]]

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...

Comment 69

[Behdad Esfahbod]

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

Comment 70

[Boris Zbarsky [:bzbarsky]]

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.  ;)

Comment 71

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 72

[Simon Montagu :smontagu]

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.

Comment 73

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> 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.