404112 - Crash with zero-sized fonts in some versions of libpango [@ libpango-1.0.so.0.1001.1 - gfxPangoFont::GetMetrics]

Status: VERIFIED FIXED

(Core :: Graphics, defect, P2)

Description

[craig harmer]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2pre) Gecko/2007111602 SeaMonkey/2.0a1pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2pre) Gecko/2007111602 SeaMonkey/2.0a1pre

The last two months worth of seamonkey builds have crashed quickly when I open any of a number of web pages I commonly visit.  One good example is finance.yahoo.com

i found some of the uploaded bug reports.  the first stack frame in the traceback is consistently:

libpango-1.0.so.0.1001.1@0xaf2c

this bug doesn't occur for every web page (for example i'm able to open this bug report using the latest build of seamonkey).


Reproducible: Always

Steps to Reproduce:
1. Download the latest nightly build of seamonkey for Linux
2. Install it and start it.
3. open http://finance.yahoo.com
4. seamonkey crashes within a few seconds; offers to send a bug report Mozilla central
Actual Results:  
Seamonkey crashes within a few seconds (while downloading data from the page).  Seamonkey then offers to send a bug
report to Mozilla central.

Expected Results:  
I expect to see news of the stock market and my stock quotes.


the bug is independant of whether or not my profile has been imported from earlier versions of seamonkey (or at least i think it is).  i've tried:

charmer@bigblack[.mozilla]rm -r ~/.mozilla/seamonkey

which should remove all cached information and profiles, have refused the offer to import profiles when Seamonkey first starts, and still see the crash when i visit sundry web sites.


i found some of the crash reports that have been uploaded.  the first stack frame in the traceback is
consistently:

libpango-1.0.so.0.1001.1@0xaf2c

This problem doesn't occur for every web page.  For example, 

here are the locations of some of the uploaded crash reports:

build 20071116:

http://crash-stats.mozilla.com/report/index/0376892b-9490-11dc-97e6-001a4bd43ed6?date=2007-11-16-22
http://crash-stats.mozilla.com/report/index/0c67e707-949b-11dc-981b-001a4bd43ef6?date=2007-11-16-23

build 20071106:
http://crash-stats.mozilla.com/report/index/045fa159-8cba-11dc-bbb9-001a4bd46e84?date=2007-11-06-22
http://crash-stats.mozilla.com/report/index/566c0582-948e-11dc-9d84-001a4bd46e84?date=2007-11-16-21

build 20071031:
http://crash-stats.mozilla.com/report/index/37d5509c-8999-11dc-ad05-001a4bd46e84?date=2007-11-02-23

build 20071019:
http://crash-stats.mozilla.com/report/index/3bdde52e-7ee4-11dc-8320-001a4bd43ef6?date=2007-10-20-08

Comment 1

[Andrew Schultz]

What version of pango do you have installed?

Loading finance.yahoo.com, I don't crash, although I do hit pango warnings:

(gecko:32735): Pango-WARNING **: failed to create cairo scaled font, expect ugly output. the offending font is 'Bitstream Charter 0'
(gecko:32735): Pango-WARNING **: failed to create cairo scaled font, expect ugly output. the offending font is 'Bitstream Charter 0'
(gecko:32735): Pango-WARNING **: shaping failure, expect ugly output. shape-engine='BasicEngineFc', font='Bitstream Charter 0', text='English Hello'
(gecko:32735): Pango-WARNING **: pango_font_get_glyph_extents called with null font argument, expect ugly output

Can you make a crash for the crash (minimal html that causes a crash)?

Comment 2

[craig harmer]

wow, someone looked at this.  cool!

oh!  i assumed that pango was bundled with mozilla.  as a result of your question, i've now discovered it's an RPM that comes with Suse.  on the Suse front, i'm running:

    SUSE Linux Enterprise Desktop 10 SP1 (i586)

(from /etc/issue).  pango is:

    charmer@bigblack[plugins]rpm -q -a | grep -i pango
    pango-devel-1.10.2-23.2
    pango-doc-1.10.2-23.2
    pango-1.10.2-23.2

i have installed all available updates to SLED 10 (there actually haven't been any updates in quite awhile, which makes me wonder whether it's really the supported product it's supposed to be or not).

Comment 3

[craig harmer]

Attachment: this page causes Seamonkey to crash

i should have mentioned that another site causing Seamonkey to crash is google (www.google.com).  i saved the web page and stripped out HTML until i got to something pretty minimal (see the attachment).

the title line and head and body statements aren't actually required to get a crash; i just left them in so that the HTML would be well formed.  really only the (lengthy) style line and the div line are required.  take either one of them out and Seamonkey will no longer crash on the page.

Comment 4

[Andrew Schultz]

Attachment: simpler testcase

I suspect you'll also crash on this (I get the pango asserts with this)

pango 1.10 doesn't the requirements listed at http://wiki.mozilla.org/Linux/Runtime_Requirements
I suspect that pango along with other packages that might not meet those requirements are causing the crash.

Comment 5

[timeless]

SeaMonkey 2.0a1pre Crash Report
UUID	0376892b-9490-11dc-97e6-001a4bd43ed6
Time	2007-11-16 14:04:47-08:00
Build ID	2007111602
OS	Linux
OS Version	0.0.0 Linux 2.6.16.46-0.12-default #1 Thu May 17 14:00:09 UTC 2007 i686 GNU/Linux
CPU	x86
CPU Info	GenuineIntel family 2 model 13 stepping 6
Crash Reason	SIGSEGV
Crash Address	0xb7918f2c
Stack Traces
Stack of Crashing Thread
frame 	signature 	source
0 	libpango-1.0.so.0.1001.1@0xaf2c 	
1 	gfxPangoFont::GetMetrics() 	mozilla/gfx/thebes/src/gfxPangoFonts.cpp:428
2 	gfxFont::Measure(gfxTextRun*, unsigned int, unsigned int, int, gfxContext*, gfxFont::Spacing*) 	mozilla/gfx/thebes/src/gfxFont.cpp:381

Comment 7

[Jesus Cea]

I am seeing this issue also in a Suse 10.0.

I thought also that pengo was included inside Firefox, like Cairo and such.

If a minimum pengo version is demanded, that *SHOULD* be documented.

In any case, I would suggest Mozilla folk to include libraries FF depends on, and not widely available. Suse 10.0 is a mere two years old beast...

Comment 8

[Jesus Cea]

castor:/home/jcea # rpm -q -a | grep -i pango
pango-devel-1.10.0-3
pango-1.10.0-3
pango-doc-1.10.0-3

Comment 9

[Jesus Cea]

Upgrading Cairo and Pango, I can't reproduce this bug anymore.

I've installed the new libs aside, using LD_LIBRARY_PATH magic, so I'm able to keep testing firefox with an "unaltered" environment. Fell free to ask.

I would suggest two approaches:

1. Include the minimal supported version of each lib inside firefox.

or

2. Test for minimal supported version for each lib when firefox launchs, and print a *clear* message about upgrading, instead of crashing at random.

Comment 12

[Jesus Cea]

A bit sideline but related to this, relevant to KDE users: bug 404966

Comment 13

[craig harmer]

(In reply to comment #7)
> I am seeing this issue also in a Suse 10.0.
> 
> I thought also that pengo was included inside Firefox, like Cairo and such.
> 
> If a minimum pengo version is demanded, that *SHOULD* be documented.
> 
> In any case, I would suggest Mozilla folk to include libraries FF depends on,
> and not widely available. Suse 10.0 is a mere two years old beast...

do you really mean Firefox?  i have Firefox 2.0.0.10 installed on the same system, and i haven't see any problem.  i think the problem with Seamonkey first appeared around Firefox 2.0.0.6 (whatever was the current version 2 - 3 months ago), but even though Seamonkey has been crashing Firefox has been fine.

Comment 14

[craig harmer]

(In reply to comment #4)
> Created an attachment (id=290497) [details]
> simpler testcase
> 
> I suspect you'll also crash on this (I get the pango asserts with this)
> 
> pango 1.10 doesn't the requirements listed at
> http://wiki.mozilla.org/Linux/Runtime_Requirements
> I suspect that pango along with other packages that might not meet those
> requirements are causing the crash.

my pango 1.10.2 is well below the required version of 1.14, so it does seem likely that's why i'm seeing this crash and others are not.  and Jesus seems to have proved that upgrading to a more recent pango will fix this.

i can't disagree with the reasoning behind dropping support for older versions of libraries, though it is a pain for those of us who would like to run Seamonkey 2.0pre on supported versions of Linux (Suse is unlikely to come along with a Seamonkey 2.0pre for SLED 10.

hopefully someone will fix Seamonkey so that Pango stops popping asserts for whatever it is that's bothering it and that will fix my problem.  meanwhile, i'll probably look into upgrading libpango and report on whether that causes the problem.

if there's an bug report open for libpango assertion failures then you might attach this to it.

Comment 15

[Andrew Schultz]

> i have Firefox 2.0.0.10 installed on the same system, and i haven't see any 
> problem.  i think the problem with Seamonkey first appeared around Firefox 
> 2.0.0.6 

Firefox 2.0.0.x and SeaMonkey 1.1.x should both run fine on your system.  SeaMonkey 2.0x and Firefox 3.0x will both have problems.

> of libraries, though it is a pain for those of us who would like to run
> Seamonkey 2.0pre on supported versions of Linux (Suse is unlikely to come 
> along with a Seamonkey 2.0pre for SLED 10.

Right.  But you can run SeaMonkey 1.1.x just fine.  And I wouldn't be surprised if SLED 11 is ready by the time SeaMonkey 2.0 is actually released.  If you're in an environment where SLED makes sense (stability), running a very alpha SeaMonkey doesn't seem to make a lot of sense.

Comment 16

[Jesus Cea]

Craig, I'm talking about Firefox 3.

Comment 17

[Mats Palmgren (inactive)]

Attachment: Like so?

I can't reproduce this crash myself, I only see the Pango-WARNINGs.
This patch silences the warnings and hopefully also avoids the crash.
Can anyone test it for me please?

Comment 19

[Jesus Cea]

I upgraded my system libs, with considerable pain. Since that, my problems with pango/cairo vanished. I can't test your patch, then.

I could test the warning side if I had a compiled versión. I'm no crazy enough to try to compile firefox myself :-).

In any case, since upgrading the libs, I see no warnings. Or, at least, far fewer that previously. In fact, checking my terminal for a 16 hours intensive navigation session, the only warnings that I'm having are flash related. None about pango&friends.

Comment 21

[Mats Palmgren (inactive)]

Since this occurs on top100 sites I think we should try to work around it.

There are a try-server build with that attached patch here:
https://build.mozilla.org/tryserver-builds/2007-12-04_08:[e-mail]-1196786133/
If anyone that can reproduce the crash could try that build and see
if it fixes it that would be appreciated.

Comment 22

[Mike Schroepfer]

Is the workaround safe?  What's the min supported version of Pango we require?

Comment 23

[Stuart Parmenter]

our minimum version is 1.14.

Comment 24

[Karl Tomlinson (:karlt)]

The workaround looks safe to me and it would silence assertions that occur even with the latest Pango.

An optimization that could be added to the patch would be to skip the call to GetPangoFont() in gfxPangoFont::GetMetrics() when size is zero.

Comment 26

[Mats Palmgren (inactive)]

Bug 406266 crashed with:
pango - 1.16.4-1.fc7.i386
libpango-1.0.so.0.1600.4
libpangocairo-1.0.so.0.1600.4

He also confirmed that my test build with the workaround fixed it for him.

dbaron says it crashed for him (bug 406635) with:
Fedora release 7 (Moonshine)
pango-1.16.4-2.fc7

Comment 27

[Karl Tomlinson (:karlt)]

Comment on attachment 291365 [details] [diff] [review]
Like so?

+    // some versions of libpango crash (bug 404112).

libpango _to_ crash

Please do avoid the call to GetPangoFont() and GetPangoLanguage() when size is
zero.  We should try to avoid creating the PangoFont unless needed.

r+=me with those changes.

underlineOffset could be -1 even for size==0, but i guess it doesn't matter if
the underline thickness is 0.  I'll let you decide on that.

mSpaceGlyph won't be set quite right but I can't see that that's a problem.

Comment 28

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

I vaguely recall old code that treated all 0-size fonts as having all metrics zero, or something like that...

Comment 29

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

(In reply to comment #27)
> libpango _to_ crash

fixed

> Please do avoid the call to GetPangoFont() and GetPangoLanguage() ...

It was intentional actually, because RealizeFont() resets 'mHasMetrics'
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/gfx/thebes/src/gfxPangoFonts.cpp&rev=1.117&root=/cvsroot&mark=327-328,417-418#292
I was afraid that it would lead to "flapping" between RealizeFont/GetMetrics.
I think it's ok though, since we short-circuit those text runs:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/gfx/thebes/src/gfxTextRunWordCache.cpp&rev=1.12&root=/cvsroot&mark=386-391#381
but as you noted in 406635 comment 8 there seems to be at least one
short-circuit missing.  With this patch I now see:
###!!! ASSERTION: throwing away our good metrics
on https://www.bankofamerica.com/

I think you're right though, we should skip GetPangoFont() and address
these separately as they turn up.  If it turns out to be hard to fix
we can always add it back.  I'll file a new bug on it (unless you
already did?).

> underlineOffset could be -1 even for size==0, 

fixed

> mSpaceGlyph won't be set quite right but I can't see that that's a problem.

Agreed.
I guess we could assert it's not used in gfxPangoFont::GetSpaceGlyph() ?

Comment 30

[Mats Palmgren (inactive)]

> I guess we could assert it's not used in gfxPangoFont::GetSpaceGlyph()

I tried that, and it was noisy due to MakeSpaceTextRun(),
after fixing MakeSpaceTextRun() it seems to be silent though -
I filed bug 407352 on it.

Comment 31

[Karl Tomlinson (:karlt)]

(In reply to comment #29)

Looks good, Mats.

> > Please do avoid the call to GetPangoFont() and GetPangoLanguage() ...
> 
> It was intentional actually, because RealizeFont() resets 'mHasMetrics'
> http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/gfx/thebes/src/gfxPangoFonts.cpp&rev=1.117&root=/cvsroot&mark=327-328,417-418#292

I can't see a reason why that needs to reset mHasMetrics.  The only place that calls RealizeFont(force = PR_TRUE) should be gfxPangoFont::RealizeFont.
I wonder if that's why Stuart or Behdad added the assert.
At least it told us where we realized the font unnecessarily.

> With this patch I now see:
> ###!!! ASSERTION: throwing away our good metrics
> on https://www.bankofamerica.com/
> 
> I think you're right though, we should skip GetPangoFont() and address
> these separately as they turn up.  If it turns out to be hard to fix
> we can always add it back.  I'll file a new bug on it (unless you
> already did?).

Thanks for sorting that out.

BTW, normally we use nsnull, but I see a few NULLs have already slipped into gfxPangoFonts.cpp.

Comment 32

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

I just hit this crash on http://answers.yahoo.com/question/index?qid=20070825021253AAvW3la

I confirmed that the crash on that page was fixed by attachment 292062 [details] [diff] [review].

Given that a bunch of people are hitting it, should we get this in for beta 2?

Comment 33

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

(System details as per last paragraph of comment 26.)

Comment 34

[Mike Schroepfer]

(In reply to comment #32)
> I just hit this crash on
> http://answers.yahoo.com/question/index?qid=20070825021253AAvW3la
> 
> I confirmed that the crash on that page was fixed by attachment 292062 [details] [diff] [review].
> 
> Given that a bunch of people are hitting it, should we get this in for beta 2?
> 

We have nearly zero margin for error (drop-dead to kick off builds is Monday am) - so if this is a *super-safe* patch I'm find with it.  I'll let Dbaron/pav make that call...

Comment 35

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

I'm in favor, but I'd also want Mats to be comfortable with landing it.

Comment 36

[Mats Palmgren (inactive)]

(In reply to comment #31)
> BTW, normally we use nsnull,

Yes, normally, but I've been told in the past to use NULL for platform
native types so there seems to be inconsistent conventions about that
(eg. bug 355273 comment 20). (I'll change it to 'nsnull' before landing)

Comment 37

[Mats Palmgren (inactive)]

Given a "nearly zero margin for error" I can't say I'm comfortable with
taking it for beta 2.  With this change we will feed all-zeroed metrics
into layout and unless gfxWindowsFont or gfxAtsuiFont is already doing
that then there is some risk with a novelty like that.

It will at least be the first time for Linux.  The values I currently
get from gfxPangoFont::GetMetrics() for a zero sized font are:
maxAscent=14.000000
maxDescent=0.000000
aveCharWidth=10.000000
underlineOffset=-1.000000
underlineSize=1.000000
strikeoutOffset=7.000000
strikeoutSize=1.000000
maxAdvance=10.000000
superscriptOffset=12.000000
subscriptOffset=12.000000

I looked around a bit on how these values are used and within 10 minutes
I found this:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/gfx/src/thebes/nsThebesFontMetrics.cpp&rev=1.43&root=/cvsroot&mark=267#263
GetMaxStringLength() will return a very large negative value when
'maxAdvance' is zero, which will circumvent the 8000 limit here:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/gfx/src/shared/nsRenderingContextImpl.cpp&rev=3.67&root=/cvsroot&mark=82-83,128,155,158#79
but fortunately we still limit it to the string length, but mostly
by luck I would say.

Taking bug 407352 at the same time would eliminate much of the risk,
but I still wouldn't call it "super-safe".

If all-zeroed metrics already occurs on Windows/Mac then I think
it's worth the risk, otherwise I think we should wait.

... later ...

I've just tested Mac/Windows and they also have non-zero fallback
values so I think this should wait until after beta 2.
We can relnote this I guess?

Comment 38

[Mats Palmgren (inactive)]

(In reply to comment #37)
> GetMaxStringLength() will return a very large negative value

I misread that, I was logging 'len' which was negative but
PR_MAX(1,len) will fix the return value of course.

Anyway, I still think there is some risk we will do something
unexpected in layout with this change.

Comment 39

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

We really should nuke GetMaxStringLength and FindSafeLength. It's no longer necessary because we don't call platform text APIs directly anymore, everything goes through the new gfx APIs which are safe with any string length.

Comment 40

[Karl Tomlinson (:karlt)]

(In reply to comment #36)
> (In reply to comment #31)
> > BTW, normally we use nsnull,
> 
> Yes, normally, but I've been told in the past to use NULL for platform
> native types so there seems to be inconsistent conventions about that
> (eg. bug 355273 comment 20). (I'll change it to 'nsnull' before landing)

There is some sense in what Josh says and the way you have it.
There doesn't seem to be a consistent behavior in this file even, so no need to change unless you really want to.

Comment 42

[craig harmer]

(In reply to comment #21)
 
> There are a try-server build with that attached patch here:
> https://build.mozilla.org/tryserver-builds/2007-12-04_08:[e-mail]-1196786133/
> If anyone that can reproduce the crash could try that build and see
> if it fixes it that would be appreciated.

This fixes the problem for me on SLED 10 using pango-1.10.2-23.2.

However, i see an assertion (no stack traceback):
(Gecko:13036): Pango-CRITICAL **: pango_context_load_font: assertion `pango_font_description_get_size (desc) != 0' failed

this occurs when i visit www.google.com (one of the original sites i had problems with) and the reproduction case in Comment #4.  however, i don't see the assertion *consistently* on www.gogle.com, only most of the time.

i'm sorry to have taken so long to test this.  i kept getting interrupted when i went to try.

in my opinion, you probably should not include this patch in your beta as is.  if you changed it to set non-zero metrics, such as those shown in Comment 37 it would be safer (though not really as correct).

thanks very much for looking into this.

Comment 43

[Mats Palmgren (inactive)]

mozilla/gfx/thebes/src/gfxPangoFonts.cpp 	1.119 

Also checked in the first two attachments as crash tests:
mozilla/gfx/thebes/crashtests/404112-1.html 	1.1
mozilla/gfx/thebes/crashtests/404112-2.html 	1.1
mozilla/gfx/thebes/crashtests/crashtests.list 	1.9 

Filed bug 408642 to follow up on comment 39.
Filed bug 407765 to make metrics on Windows/Mac also be all-zero for zero
size fonts, for consistency between platforms.

-> FIXED

Comment 44

[Jens-Uwe]

hmmm, since the patch was just included, I guess it is ok that the bug is still in beta2 and I should not panic?

Comment 46

[Carsten Book [:Tomcat]]

verified fixed using Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9b3) Gecko/2008020513 Firefox/3.0b3 - no crash on testcase

-> Verified fixed