88486 - Some pages cause crash in font code [@nsFontGTKNormal::GetWidth]

Status: VERIFIED FIXED

(Core :: Internationalization, defect)

Description

[Matthew Cline]

Build 20010629, Linux 2.4.5-ac17 i686, RedHat 6.1, XFree86 4.1.0

This bug seems to have popped up between the build from the 27th and the
build from the 29th. Certain pages cause a crash at nsFontMetricsGTK.cpp:2005.
The cause of the crash is a SIGSEGV. The offending line is:

ENCODER_BUFFER_ALLOC_IF_NEEDED(p, mCharSetInfo->mConverter,
aString, aLength, buf, sizeof(buf), bufLen);

The object instance causing the crash looks like:


(gdb) p *this
$9 = {<nsFontGTK> = {mMap = 0x89c5358, mCharSetInfo = 0x416291c0,
mName = 0x42092fc0
"-adobe-helvetica-medium-r-normal--12-120-75-75-p-67-iso10646-1",
mUserDefinedFont = 0x0, mSize = 12, mBaselineAdjust = 0,
mFont = 0x898b498, mAlreadyCalledLoadFont = 1,
_vptr. = 0x4162a660}, <No data fields>}


All the adresses seem readable, except that mCharSetInfo->mConverter is
NULL. mCharSetInfo looks like:

(gdb) p *mCharSetInfo
$11 = {mCharSet = 0x0,
Convert = 0x4160abc8 <ISO10646Convert(nsFontCharSetInfo *, XFontStruct *,
unsigned short const *, int, char *, int)>, mSpecialUnderline = 1 '\001',
mMap = 0x0, mConverter = 0x0, mLangGroup = 0x0, mInitedSizeInfo = 1,
mOutlineScaleMin = 6, mBitmapScaleMin = 12, mBitmapOversize = 1.2,
mBitmapUndersize = 0.80000000000000004}

The fonts that I'm using are:

urw-fonts-2.0-3mdk
chkfontpath-1.4.1-1
ghostscript-fonts-6.0-2
XFree86-75dpi-fonts-4.1.0-1mdk
mozilla-fonts-20000310-8mdk
XFree86-100dpi-fonts-4.1.0-4mdk
freefont-0.10

This looks very simmillar to bug 86436, but I think it is different,
because I was unable to duplicate that bug with either a build from the 27th
or from the 29th.  Also, that bug was filed 11 days ago, and this bug showed
up in the past 2 days.

This bug might be related to bug 88444, as there were a bunch of these
assertions before the crash (and that bug showed up in around the same
time frame as this bug):

###!!! ASSERTION: unexpected number of nodes: '(nodes.Count() == 1)', file
nsFontMetricsGTK.cpp, line 3421
###!!! Break: at file nsFontMetricsGTK.cpp, line 3421

Comment 1

[Matthew Cline]

Attachment: Stack trace for the bug

Comment 2

[Mike Pinkerton (not reading bugmail)]

looks like layout to me.

Comment 3

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

->Internationalization (font code)

Comment 4

[nhottanscp]

Reassign to bstell, dup of bug 88752?

Comment 5

[Matthew Cline]

I've done some more digging, and the problem with the page that I've
referred to seems to be caused by this tag:

   <font face="arial,helvetica" size="-1">

More specifically, the very last <font> tag in the page; the rest
don't seem to cause any problem.  It might have the same root problem
as for the bug with the utf-8 encoded page, since the crash happens in
a macro from intl/uconv/public/nsIUnicodeEncoder.h, but the ZDNet page
is not UTF-8, nor does it even have anything to change its charset
from the default one.

As far as I can tell, the crash happens because the macro
ENCODER_BUFFER_ALLOC_IF_NEEDED assumes that the encoder argument (the
second argument) will be non-NULL.  However, the code that sets up the
nsFontCharSetInfo struct assumes that the mConverter member of the
struct (which is of type nsIUnicodeEncoder*) might be NULL:

	static void
	SetUpFontCharSetInfo(nsFontCharSetInfo* aSelf)
	{
	
	.
	.
	.
	
	    nsIUnicodeEncoder* converter = nsnull;
	    res = gCharSetManager->GetUnicodeEncoder(charset, &converter);
	    if (NS_SUCCEEDED(res)) {
	      aSelf->mConverter = converter;

Also, some of the static functions in nsFontMetricsGTK.cpp assume that
mConverter might be NULL, like DoubleByteConvert() and
SingleByteConvert().  I stuck a different version of
ENCODER_BUFFER_ALLOC_IF_NEEDED into the file nsFontMetricsGTK.cpp,
that checks to see if the encoder argument is NULL, and the example
ZDNet then renders fine without any crashes:

#undef ENCODER_BUFFER_ALLOC_IF_NEEDED
#define ENCODER_BUFFER_ALLOC_IF_NEEDED(p,e,s,l,sb,sbl,al)   \
  PR_BEGIN_MACRO                                            \
    if (e                                                   \
        && NS_SUCCEEDED((e)->GetMaxLength((s), (l), &(al))) \
        && ((al) > (PRInt32)(sbl))                          \
        && (nsnull!=((p)=(char*)nsMemory::Alloc((al)+1)))   \
        ) {                                                 \
    }                                                       \
    else {                                                  \
      (p) = (char*)(sb);                                    \
      (al) = (sbl);                                         \
    }                                                       \
  PR_END_MACRO 

Comment 6

[kill this account]

*** Bug 88752 has been marked as a duplicate of this bug. ***

Comment 7

[kill this account]

Attachment: patch; test for non-null pointer

Comment 8

[Stuart Parmenter]

r=pavlov

Comment 9

[kill this account]

*** Bug 88927 has been marked as a duplicate of this bug. ***

Comment 10

[pocemit]

Yay, this might fix xlib bug 88695...

Comment 11

[Christopher Blizzard (:blizzard)]

Macro hell!

sr=blizzard

Comment 12

[Roland Mainz]

CC:'ing [e-mail] for checkin to get this in _quick_ ... :-)

Comment 13

[Mike Kaply [:mkaply]]

Fix checked in

Comment 14

[kill this account]

thanks to everyone for getting this in!

It was so weird / cool: I got up the morning and I knew I had to get this one 
in and I could not find it in my list of open bugs!

Comment 15

[Frank Tang]

reopen so we won't forget to land it into m92 branch.

Comment 16

[Frank Tang]

reassign to ftang for m92 branch landing

Comment 17

[Boris Zbarsky [:bzbarsky]]

*** Bug 88473 has been marked as a duplicate of this bug. ***

Comment 18

[timeless]

*** Bug 88546 has been marked as a duplicate of this bug. ***

Comment 19

[timeless]

*** Bug 88548 has been marked as a duplicate of this bug. ***

Comment 20

[timeless]

*** Bug 88823 has been marked as a duplicate of this bug. ***

Comment 21

[Frank Tang]

the change add additional null checking, which should be safe. add vtrunk+ to
the status whiteboard

Comment 22

[Boris Zbarsky [:bzbarsky]]

*** Bug 88750 has been marked as a duplicate of this bug. ***

Comment 23

[Frank Tang]

*** Bug 89201 has been marked as a duplicate of this bug. ***

Comment 24

[Frank Tang]

pdt+ per pdt meting. Land it today

Comment 25

[Frank Tang]

Sorry, this does not make sense to nsbranch at all. The crashing code is only in 
the trunk but not in the branch. remove nsBranch, vtrunk, nsbrach+ and pdt+ and 
mark it fixed. 

Comment 26

[timeless]

are you sure? what about bug 89358

Comment 27

[kill this account]

*** Bug 89358 has been marked as a duplicate of this bug. ***

Comment 28

[kill this account]

this code is not on the branch

Comment 29

[Jacek Piskozub]

Adding mostfreq for completness/correctness (whatever) at 10 dups.

Comment 30

[Yuying Long]

Verified it doesn't crash on 08-22 trunk build.

However, if click by URL of bug 89358 still crash, I'll re-open that one.