Crashes in thebes.dll when not able to match the font

RESOLVED FIXED

Status

()

Core
Graphics
--
major
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Peter Weilbacher, Assigned: Peter Weilbacher)

Tracking

({crash})

Trunk
All
OS/2
crash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

9 years ago
I see quite a few crashes in SeaMonkey (comm-central 20081118122900), it was reproducible for me with a default profile on <http://de.wikipedia.org/w/index.php?title=Diskussion:Kasserolle&action=edit>. I finally traced it down to a NULL pattern that is returned when a font pattern cannot be matched in gfxOS2Font::CairoFontFace.

I now made FontConfig in mzfntcfgft more crash safe, so this shouldn't occur any longer if linked against the newest code from <http://hg.mozilla.org/users/mozilla_weilbacher.org/mzfntcfgft/>. But we still should fix the gfxOS2Font class, too.

Could be that this is related to bug 449727 even though the debugging output shows something different.
(Assignee)

Comment 1

9 years ago
Hmm, I see that the font not found in my case is "Deja Vu Serif". This apparently comes from all.js where we list the Unicode fonts to use by default. But on my system the Deja fonts are all called "DejaVu" and not "Deja Vu". Were they always called like that, without the space or has that changed recently?
(Assignee)

Comment 2

9 years ago
Created attachment 348945 [details] [diff] [review]
fix crash and rename fonts

Well, here's the patch that I think we should apply and eventually also get into CVS (on the 1.9.0 "branch").
Attachment #348945 - Flags: review?(wuno)

Comment 3

9 years ago
I don't know if they ever had a space in the name, but they are definitely without it here (using version 2.27).
(Assignee)

Comment 4

9 years ago
And I checked v2.23, v2.20, v2.0, and v1.0 and they are all without space. Don't know how I ever got the idea that I should specify them with space in all.js, so we should definitely correct that.

Comment 5

9 years ago
Comment on attachment 348945 [details] [diff] [review]
fix crash and rename fonts

> if (GetName() == NS_LITERAL_STRING("Workplace Sans") && fcW >= FC_WEIGHT_DEMIBOLD) {

wouldn't it be cheaper to do the math comparison before the function call?
(Assignee)

Comment 6

9 years ago
Created attachment 349005 [details] [diff] [review]
fix crash and rename fonts, v2

Thanks timeless, yes, I think that's true.
Attachment #348945 - Attachment is obsolete: true
Attachment #349005 - Flags: review?(wuno)
Attachment #348945 - Flags: review?(wuno)

Comment 7

9 years ago
(In reply to comment #6)
> Created an attachment (id=349005) [details]
> fix crash and rename fonts, v2

Hm, I still crash in thebes with this patch applied and (three times) rebuilt mzfntcfgft. First thought was about gcc346 making the trouble, but the same behavior when built with gcc335. Your nightlies from yesterday do run however (except the crashes in mozjs).
(Assignee)

Comment 8

9 years ago
Can you add printfs in the code to check if this is the location where it crashes? Or can you run under some debugger to get the crash location? It might be that it still crashes in some other part of the code...

Updated

9 years ago
Attachment #349005 - Flags: review?(wuno) → review+

Comment 9

9 years ago
Comment on attachment 349005 [details] [diff] [review]
fix crash and rename fonts, v2

will investigate my crash later, its probably caused by sth else
(Assignee)

Updated

9 years ago
Attachment #349005 - Flags: approval1.9.1b2?
(Assignee)

Comment 10

9 years ago
Comment on attachment 349005 [details] [diff] [review]
fix crash and rename fonts, v2

OS/2 only patch, touching cross-platform file all.js.
Attachment #349005 - Flags: approval1.9.1b2? → approval1.9.1b2+
Comment on attachment 349005 [details] [diff] [review]
fix crash and rename fonts, v2

a191b2=beltzner
(Assignee)

Comment 12

9 years ago
Pushed as http://hg.mozilla.org/mozilla-central/rev/951178f392ef

I also pushed a tiny debug-only fix as http://hg.mozilla.org/mozilla-central/rev/b902fb44692d to fix compilation with DEBUG_thebes_2.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Comment 13

9 years ago
Hi, trying Seamonkey just after this patch was checked in Seamonkey immediately crashes with
Killed by SIGSEGV
pid=0x2cb2 ppid=0x001c tid=0x0001 slot=0x00f9 pri=0x0200 mc=0x0001
F:\MOZILLA\SEAMONKEY\SEAMONKEY.EXE
THEBES 0:00023961
cs:eip=005b:1d173961      ss:esp=0053:0011dd54      ebp=0011debc
 ds=0053      es=0053      fs=150b      gs=0000     efl=00012212
eax=00000000 ebx=0011dee4 ecx=1bff0002 edx=00000000 edi=20697160 esi=2066bb78
Process dumping was disabled, use DUMPPROC / PROCDUMP to enable it.

Trying a debug build I get this from stdout,
Could not match font for:
  family=!/!ERROR_UNDEFINED_PREF!/!, weight=400, slant=0, size=0

and from stderr

###!!! ASSERTION: Failed to make font face: 'mFontFace', file I:/comm-central/mozilla/gfx/thebes/src/gfxOS2Fonts.cpp, line 366

After which Seamonkey hangs with 100% CPU usage (debug build). Tested with a new profile and old profile.
This is with GCC 3.4.6
(Assignee)

Comment 14

9 years ago
Dave, that shows that the patch actually did what it should. Although the "!/!ERROR_UNDEFINED_PREF!/!" is curious.

Please update mzfntcfgft once more to fix the crash.

Comment 15

9 years ago
Ok, updating mzfntcfgft and rebuilding thebes.dll allows Seamonkey to make it to the Profile Manager. After exiting it once again crashes. Same error on stderr but on stdout now this,

Could not match font for:
  family=Tms Rmn, weight=400, slant=0, size=0

Pressing enter after the assertion still hangs Seamonkey with 100% CPU usage.
You need to log in before you can comment on or make changes to this bug.