282453 - XFT crash when displaying page with bad font if character not found

Status: VERIFIED FIXED

(Core Graveyard :: GFX: Gtk, defect)

Description

[Lorenzo Colitti]

Recently I've been crashing a lot on some web pages containing glyphs that are
not found in my fonts.

I most recently did this by searching for "acpi assembly language" on google.
Talkback ID TB3721222H,
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=TB3721222H

The problem seems to be here:

http://lxr.mozilla.org/seamonkey/source/gfx/src/gtk/nsFontMetricsXft.cpp#948

948     for (PRInt32 end = mLoadedFonts.Count(); i < end; ++i) {
949         nsFontXft *font = (nsFontXft *)mLoadedFonts.ElementAt(i);
950         if (font->HasChar(aChar)) {
951             if (font->GetXftFont())
952                 return font;
953             // This is a bad font, so remove it from mLoadedFonts.  This
954             // could happen if it's in fc.cache-1 but the font doesn't exist
955             // (https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=111973)
956             // or isn't readable.
957             mLoadedFonts.RemoveElementAt(i--);
958         }
959     }

The loop keeps iterating until i == end, which is the size of the array at the
beginning of the loop. However, if RemoveElementAt() has been called, if i ==
end it means that the loop has already gone past the end of the array. Result:
an assertion in nsVoidArray, followed by a crash.

Comment 1

[Lorenzo Colitti]

Attachment: Testcase

Testcase, crashes every time for me.

Comment 2

[Lorenzo Colitti]

CCing dbaron, who is in CVS blame for those lines

Comment 3

[Lorenzo Colitti]

Attachment: proof-of-concept patch

This fixes the problem, although I'm not sure about the elegance of a for
statement with an empty first clause...

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 174469 [details] [diff] [review]
proof-of-concept patch

Nothing wrong with an empty  first clause.

r+sr=bzbarsky.

I think it's worth taking this crash fix for 1.8b1

Comment 5

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

Comment on attachment 174469 [details] [diff] [review]
proof-of-concept patch

r+sr=dbaron as well.  This should go in on the branches where bug 180309 went
in.  This is a simple crash fix for the case where all the fonts are invalid.

Comment 6

[Mike Kaply [:mkaply]]

Comment on attachment 174469 [details] [diff] [review]
proof-of-concept patch

a=mkaply for all

Comment 7

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

Fix checked in to trunk, 2005-02-16 09:07 -0800.
Fix checked in to MOZILLA_1_7_BRANCH, 2005-02-16 09:08 -0800.
Fix checked in to AVIARY_1_0_1_20050124_BRANCH, 2005-02-16 09:09 -0800.

Thanks for the patch.

Comment 8

[sairuh (rarely reading bugmail)]

using 2005022307-1.0.1 firefox bits (gtk2 installer build, on linux fc3), the
attached test case doesn't crash for me.

Lorenzo, does this work for using a recent nightly build? (trunk or branch,
firefox or mozilla)

Comment 9

[Lorenzo Colitti]

(In reply to comment #8)
> using 2005022307-1.0.1 firefox bits (gtk2 installer build, on linux fc3), the
> attached test case doesn't crash for me.

I think it depends on the fonts you have installed. The bug is only triggered if
you have a "bad font".

> Lorenzo, does this work for using a recent nightly build? (trunk or branch,
> firefox or mozilla)

Yes, since dbaron's checkin this works perfectly. Marking verified.

Comment 10

[Lorenzo Colitti]

In case you need more info, I'm using a recent FF trunk build:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0

Comment 11

[Lorenzo Colitti]

Sorry, that was FF 1.0. The one I'm using is this:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b2) Gecko/20050222 Firefox/1.0+