365923 - [GTK1 only?] Crash [@ SetFontCharsetInfo]

Status: RESOLVED FIXED

(Core Graveyard :: GFX: Gtk, defect)

Description

[Mats Palmgren (inactive)]

Spawned from bug 306902, testcases in that bug.

bug 306902 comment 21:

It still crashes. I'm pretty sure it's gtk1 or xlib widget only.
It's a SEGV crash from a read. I'm not sure how much of a priority that is.

The bug seems to have changed from the last time (attachment 197271 [details])
The crash now occurs when calling CCMAP_HAS_CHAR_EXT(m,c) when 'c' is a
surrogate character and 'm' is 'gDoubleByteSpecialCharsCCMap' which comes
from a generated file ("dbyte_special_chars.ccmap"):
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/gfx/src/gtk/nsFontMetricsGTK.cpp&rev=1.289&root=/cvsroot&mark=872-873#865
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/gfx/src/x11shared/dbyte_special_chars.ccmap&rev=1.5&root=/cvsroot

It could mean that the ccmap generator has a bug or CCMAP_HAS_CHAR_EXT
has bug, both are used on other (modern) platforms too I think.

Or maybe CCMAP_HAS_CHAR_EXT is just used inappropriately here:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/gfx/src/gtk/nsFontMetricsGTK.cpp&rev=1.289&root=/cvsroot&mark=4112,4122#4089
(We crash on line 4122, with 'c' == 120793, which was assigned on line 4112
from aString[i]=55349 and aString[i+1]=57305.)

Some more debug data, for posterity:
gDoubleByteSpecialCharsCCMap c=120793 (0x1d7d9) (c)&0xffff0000 is true
(0x10000)
extraSurrogateLength=1 , aString[i]=55349 aString[i+1]=57305
ALU_SIZE=32 CCMAP_SIZE(gDoubleByteSpecialCharsCCMap)=16361
(CCMAP_FLAG(m) & CCMAP_SURROGATE_FLAG) is true
CCMAP_PLANE(c) = 1
CCMAP_TO_PAGE((m),(c)) = 32
CCMAP_ALU_INDEX(c & 0xffff) = 6
(CCMAP_TO_ALU(m, c & 0xffff) = 0
CCMAP_BIT_INDEX(c & 0xffff) = 25
(CCMAP_TO_ALU(m, c & 0xffff) >> CCMAP_BIT_INDEX(c & 0xffff) = 0
CCMAP_FOR_PLANE_EXT(gDoubleByteSpecialCharsCCMap, CCMAP_PLANE(c)) = 0xf6e1c24
(-824388892 bytes off gDoubleByteSpecialCharsCCMap)
Program received signal SIGSEGV, Segmentation fault.

Maybe Talkback can enlighten us if this a problem on other platforms as well?
(analyzing consumers of SupportsChar() and CCMAP_HAS_CHAR_EXT)

Comment 1

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

Isn't GTK1 totally unsupported these days? I thought we were going to CVS remove it actually.

Comment 2

[Daniel Veditz [:dveditz]]

Critical security bugs must have owners. If you can't work on this bug please help us find another active owner for it.

Does SeaMonkey still use GTK1? If not let's kill it entirely.

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

I think we provide gtk1 builds in addition to gtk2 ones, but for trunk those will have to go away anyway.

I won't fix gtk1-only bugs.

Comment 4

[Mats Palmgren (inactive)]

Maybe we shouldn't write this off as "GTK1 only" just yet.
I searched for talkback reports on functions that uses CCMAP_HAS_CHAR_EXT
and found several crashes on Win32. See for example TB25693388 and TB28343522.
Both have CCMAP_HAS_CHAR_EXT on the crash line.

Comment 5

[chris hofmann]

mats/anybody, have additional iseas on a fix?

Comment 6

[Window Snyder]

Critical security bugs need to have an owner.  If you are not the correct person for this bug, please help us find someone else.  Thanks.

Comment 7

[Mats Palmgren (inactive)]

Attachment: TB28343522

Preserving TB28343522 before it's removed... 
FWIW, the top stack frame links to
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/gfx/src/windows/nsFontMetricsWin.cpp&mark=2825&rev=MOZILLA_1_8_BRANCH#2825

Comment 8

[Mats Palmgren (inactive)]

Still occurs in a gtk1 1.8 branch build of todays date.

Comment 9

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

This fixes the crashes for me (with the fix in bug 366493 also).
(The patch is against the 1.8 branch)

There are two changes:

1. Change all CCMAP_HAS_CHAR_EXT to SAFE_CCMAP_HAS_CHAR_EXT that also
   null-checks mCCmap before using it.
   This is a low-risk change I think.
2. skip looking in the ccmap of already loaded fonts for surrogate
   characters in nsFontMetricsGTK::LocateFont(), instead go directly
   to FindFont().  (this change is the second hunk).
   I have no clue if this part is a correct and/or complete change.

I only tested this on a few sites, but I did verify that
for example http://mozilla.org.il/ and http://www.news.cn/
still looks correct.

Take it or leave it ;-) this is as far as I'm willing to take this bug.

Comment 10

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

(In reply to comment #9)
> There are two changes:
> 
> 1. Change all CCMAP_HAS_CHAR_EXT to SAFE_CCMAP_HAS_CHAR_EXT that also
>    null-checks mCCmap before using it.
>    This is a low-risk change I think.

I agree this even if there are other bugs. I think that it is enough for the legacy gfx.

> 2. skip looking in the ccmap of already loaded fonts for surrogate
>    characters in nsFontMetricsGTK::LocateFont(), instead go directly
>    to FindFont().  (this change is the second hunk).
>    I have no clue if this part is a correct and/or complete change.

I cannot review this, sorry.

jshin:
Can you review this?

Comment 11

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

Comment on attachment 259513 [details] [diff] [review]
Patch rev. 1

Jshin, can you review this? I think that you are best reviewer for this patch.

Comment 12

[chris hofmann]

simon might also be able to help with the review if we can't find jshin.

Comment 13

[Mike Connor [:mconnor]]

this looks like branch-only now, and seamonkey-only now, since bug 326152 is resolved

Comment 14

[Simon Montagu :smontagu]

Comment on attachment 259513 [details] [diff] [review]
Patch rev. 1

I am a little hesitant here because I am totally unable to reproduce the crash. With the patch, how does a page containing supplementary characters appear? e.g. http://alanwood.net/unicode/aegean_numbers.html
As far as I know there is no way that the supplementary characters are ever going to appear correctly on GTK1, so the most I'm hoping for is sane fallback.

Comment 15

[Damon Sicore (:damons)]

Poke.  

What's the plan?

Comment 16

[Mats Palmgren (inactive)]

(In reply to comment #14)
> ... I am totally unable to reproduce the crash.
> With the patch, how does a page containing supplementary characters appear?
> e.g. http://alanwood.net/unicode/aegean_numbers.html

That page always crashes for me without the patch.  Unfortunately
it also crashes with the patch, but now it's much harder to reproduce.
It usually crashes a couple of times, then I need to reboot to be able
to reproduce it again.  The crash this time is through:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 16384 (LWP 2342)]
0x41d26103 in nsFontMetricsGTK::FindStyleSheetGenericFont(unsigned) (this=0x87b1ef0, aChar=65792) at nsFontMetricsGTK.cpp:6312
6312          else if (sub_font && SAFE_CCMAP_HAS_CHAR_EXT(sub_font->mCCMap, aChar)) {
(gdb) bt
#0  0x41d26103 in nsFontMetricsGTK::FindStyleSheetGenericFont(unsigned) (this=0x87b1ef0, aChar=65792) at nsFontMetricsGTK.cpp:6312
#1  0x41d26ef2 in nsFontMetricsGTK::FindFont(unsigned) (this=0x87b1ef0, aChar=65792) at nsFontMetricsGTK.cpp:6599
#2  0x41d21a6b in nsFontMetricsGTK::GetTextDimensions(unsigned short const*, unsigned, nsTextDimensions&, int*, nsRenderingContextGTK*) (this=0x87b1ef0, aString=0xbfffa828, aLength=2, aDimensions=@0xbfffa610, aFontID=0x0, aContext=0x87ee4d8)

so avoiding CCMAP_HAS_CHAR_EXT for surrogates in
nsFontMetricsGTK::LocateFont() is not enough.

I did some more testing on this and it seems it's always
gDoubleByteSpecialCharsCCMap that we crash on.  I find it a bit
odd that CCMAP_SURROGATE_FLAG is set for this map considering the
data it contains:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/gfx/src/x11shared/dbyte_special_chars.ccmap&rev=1.5&root=/cvsroot&mark=80-105#75
That seems like an error to me.

Comment 17

[Mats Palmgren (inactive)]

Attachment: Wallpaper rev. 2

This patch also fixes the URL in comment 14.
It avoids doing any lookups of surrogates in the gDoubleByteSpecialCharsCCMap
at all, which would never find a match anyway AFAICT.

To answer the question from comment 14 on what it looks like with the patch:
?    ?   AEGEAN WORD SEPARATOR LINE
?    ?   AEGEAN WORD SEPARATOR DOT
?    ?   AEGEAN CHECK MARK
[1]  [1] AEGEAN NUMBER ONE
[2]  [2] AEGEAN NUMBER TWO
... etc ...
[90000] [90000] AEGEAN NUMBER NINETY THOUSAND
then ? in both columns for the rest

Comment 18

[Simon Montagu :smontagu]

Comment on attachment 269354 [details] [diff] [review]
Wallpaper rev. 2

This seems like rather hacky wallpapering over the real issue, but I guess I'm OK with it as a branch-only patch.

The rendering described in the previous comment is what I wanted to see, with transliteration for the numbers and single question marks for the rest.

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 269354 [details] [diff] [review]
Wallpaper rev. 2

approved for 1.8.1.5 and 1.8.0.13, a=dveditz for release-drivers

Comment 20

[Mats Palmgren (inactive)]

Checked in to MOZILLA_1_8_BRANCH:
/cvsroot/mozilla/gfx/src/gtk/Attic/nsFontMetricsGTK.cpp,v: 1.280.6.3

Checked in to MOZILLA_1_8_0_BRANCH:
/cvsroot/mozilla/gfx/src/gtk/Attic/nsFontMetricsGTK.cpp,v: 1.280.6.1.4.1

(gfx/src/gtk/* is removed on trunk)

-> FIXED

Comment 21

[Carsten Book [:Tomcat]]

The Testcase from Bug 306902 (https://bugzilla.mozilla.org/attachment.cgi?id=194712) still crashs 1.8.0.13 and 1.8.1.7 build with segfault /opt/firefox/run-mozilla.sh: line 131:  3107 Segmentation fault      "$prog" ${1+"$@"} on Linux Fedora F7 (i think its related to Bug 366493), so i cannot set the verified keywords for 1.8.0.13/1.8.1.7 here.

Comment 22

[Mats Palmgren (inactive)]

The crash you're now seeing with the testcase from bug 306902
is indeed bug 366493.  I think we should use the URL instead
for verification/testcase.