Bugzilla

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

Reporter

Comment 1

•

16 years ago

According to crash-stats, this first occurred in 2008050206 (though it also appeared in the 4.0a1pre build 2008050103).

Shorter loading query: http://crash-stats.mozilla.com/report/list?range_unit=months&query_search=signature&query_type=exact&product=Firefox&platform=windows&version=Firefox%3A3.0b5%2CFirefox%3A3.0b5pre%2CFirefox%3A3.0pre&branch=1.9&signature=gfxWindowsFont%3A%3AGetOrMakeFont%28FontEntry%2A%2C+gfxFontStyle+const%2A%29&query=gfxWindowsFont%3A%3AGetOrMakeFont%28FontEntry%2A%2C+gfxFontStyle+const%2A%29&range_value=3

Longer loading, more thorough query: http://crash-stats.mozilla.com/report/list?range_unit=months&query_search=signature&query_type=startswith&signature=gfxWindowsFont%3A%3AGetOrMakeFont%28FontEntry%2A%2C+gfxFontStyle+const%2A%29&query=gfxWindowsFont%3A%3AGetOrMakeFont&range_value=3

Regression range: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2008-05-01+00&maxdate=2008-05-02+06&cvsroot=%2Fcvsroot

The only thebes checkin was bug 418479, which seems to be Mac-only. Given the stack, this could be a layout bug. Bug 430813 changed layout/generic/nsBlockFrame.cpp which appears in this stack. CCing dbaron.

Keywords: regression

Comment 2

•

16 years ago

Pretty unlikely to be the nsBlockFrame changes.

Mats Palmgren (inactive)

Comment 3

•

16 years ago

Bug 410544 and bug 434046 looks related.

Damon Sicore (:damons)

Comment 4

•

16 years ago

1.9-, 1.9.0.1+.   Still need to keep a close eye on this one as a crash on startup may result in lower top crash numbers.

Flags: wanted1.9.0.x?

Flags: blocking1.9?

Flags: blocking1.9.0.1+

Flags: blocking1.9-

Kai Liu

Comment 6

•

16 years ago

Anyone considered bug 427411 as a possible culprit?  That landed on April 29, which is pretty close to the regression range and looks much more likely than the other two candidates suggested so far.

Also, the cause of bug 410544, bug 434046, and this bug are all similar in that they are caused by a null font entry in the font entries list; all three could be band-aided by performing a sanity check before adding each entry.

Martijn Wargers (dead)

Comment 7

•

16 years ago

CC-ing roc then, since you mentioned bug 427411.

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

Comment 8

•

16 years ago

427411 would have caused us to crash at this point if we have a null aFontEntry, but the null aFontEntry is probably another bug which would quite likely have caused us to crash somewhere else.

Kai Liu

Comment 9

•

16 years ago

After the vector font bug, I was thinking something along the lines of 427411 simply unearthing another bug that would have otherwise remained obscured.

Tim Riley [:timr]

Comment 10

•

16 years ago

Adding [RC2?].  We are really concerned that the crash stats are under-counting this one since people aren't likely to come back a 2nd or 3rd time when there is a start-up crash.  But we also understand there is no patch yet and it is hard to repro.

Whiteboard: [RC2?]

Assignee

Comment 11

•

16 years ago

(In reply to comment #8)
> 427411 would have caused us to crash at this point if we have a null
> aFontEntry, but the null aFontEntry is probably another bug which would quite
> likely have caused us to crash somewhere else.

Probably bug 418381.

Depends on: 418381

Lance McCord

Comment 12

•

16 years ago

I can reproduce on Vista SP1 with RC1 by uninstalling the following three fonts:

* micross_0.ttf ("Microsoft Sans Serif");
* tahomabd.ttf ("Tahoma Bold"); and
* tahoma_0.ttf ("Tahoma").

Crash Report: http://crash-stats.mozilla.com/report/index/05ad0063-294f-11dd-afdb-001cc4e2bf68?p=1

Assignee

Comment 13

•

16 years ago

(In reply to comment #12)
> I can reproduce on Vista SP1 with RC1 by uninstalling the following three
> fonts:
> 
> * micross_0.ttf ("Microsoft Sans Serif");
> * tahomabd.ttf ("Tahoma Bold"); and
> * tahoma_0.ttf ("Tahoma").

If I try to remove any of those fonts through (Control Panel -> Fonts) on XP SP2, they magically reinstall, a couple of seconds later.

Kai Liu

Comment 14

•

16 years ago

(In reply to comment #13)
> If I try to remove any of those fonts through (Control Panel -> Fonts) on XP
> SP2, they magically reinstall, a couple of seconds later.
> 

Windows keeps a backup of system files in dllcache and restores them if they get changed or deleted.  You can run "sfc /purgecache" to clear the cache, but it won't let you delete them if those fonts are in use.  I've only tried doing this on XP; dunno if it's somehow different on Vista.

Assignee

Comment 15

•

16 years ago

My best guess here is to use gfxWindowsFontGroup::FamilyListToArrayList instead of gfxWindowsPlatform::FindFontEntry with logFont.lfFaceName from DEFAULT_GUI_FONT to address the first part of bug 418381 comment 15.

Comment 16

•

16 years ago

Stuart: can you please look at this and tell us if you have an idea of what might be causing it or what the solution would entail? I'm pretty sure we want to relnote and punt on this, though.

Flags: blocking1.9- → blocking1.9?

Comment 17

•

16 years ago

we should give what karlt suggests a try if we do an rc2

Comment 18

•

16 years ago

Karl: can you put together a patch?

Assignee

Comment 19

•

16 years ago

Attached patch resolve DEFAULT_GUI_FONT name before looking for an entry (obsolete) — Details — Splinter Review

Guess 1:

I'm not confident this would resolve the situation with STR in comment 12, but then I doubt that's that situation that most crash reports are coming from.

Assignee: nobody → mozbugz

Status: NEW → ASSIGNED

Attachment #322615 - Flags: review?

Assignee

Comment 20

•

16 years ago

Attached patch try SPI_GETNONCLIENTMETRICS (obsolete) — Details — Splinter Review

Guess 2:

The message box font sounded a reasonable option.

http://msdn.microsoft.com/en-us/library/ms724506(VS.85).aspx

This patch includes the changes from guess 1 but tries another way to get a default font.  I'm assuming we don't want to pull in nsSystemFontsWin here.

I haven't tested that either of these patches compile, sorry.

If we can get try server builds (and smoke test them) then we can ask the reporters of bug 410544 and bug 434046 to test.  I've submitted both to the try server but haven't seen any evidence of response.

Attachment #322616 - Flags: review?

Assignee

Updated

•

16 years ago

Attachment #322615 - Flags: review? → review?(pavlov)

Assignee

Updated

•

16 years ago

Attachment #322616 - Flags: review? → review?(pavlov)

Assignee

Comment 21

•

16 years ago

Attached patch resolve DEFAULT_GUI_FONT name before looking for an entry v1.1 (obsolete) — Details — Splinter Review

Forgot an &.

Attachment #322615 - Attachment is obsolete: true

Attachment #322627 - Flags: review?(pavlov)

Attachment #322615 - Flags: review?(pavlov)

Assignee

Comment 22

•

16 years ago

Attached patch try SPI_GETNONCLIENTMETRICS v1.1 (obsolete) — Details — Splinter Review

More &s.

Attachment #322616 - Attachment is obsolete: true

Attachment #322628 - Flags: review?(pavlov)

Attachment #322616 - Flags: review?(pavlov)

Comment 23

•

16 years ago

I think we probably want to do those in the opposite order, but i'm not sure...

Tony Chung [:tchung]

Comment 24

•

16 years ago

From sam's additional comments offline,
"may be happening in thebes or layout. dbaron believes thebes. Needs a developer, comments in stack aren't helpful. This is a crash on startup. We want to nominate for RC2."

The crash on startup part would be a concern for a RC2 nom if we can get a fix.  Sam, please comment further if you have more information.

Reporter

Comment 25

•

16 years ago

As I said in the meeting today, this doesn't need to block the release but if we get a fix that clearly fixes the problem in a safe way, we should definitely take it. If not, this should be a 1.9.0.1 blocker.

Comment 26

•

16 years ago

Stuart, if we get a safe patch that you think we should take as a ridealong, please find one of the drivers to get approval. Otherwise we'll wait until 1.9.0.1

Flags: blocking1.9? → blocking1.9-

Assignee

Comment 27

•

16 years ago

This crash looks like it is happening with system fonts.
Otherwise a NULL font entry would crash here:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/gfx/thebes/src/gfxWindowsFonts.cpp&rev=1.206&mark=855,857#843

(which is bug 434046).

This makes me wonder why GroupFamilyListToArrayList would not find the system font.

Are we confident that MultiByteToWideChar(CP_ACP, 0, LOGFONT.lfFacename) is the same as LOGFONTW.lfFacename?

Assignee

Comment 28

•

16 years ago

MultiByteToWideChar is

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/gfx/src/thebes/nsSystemFontsWin.cpp&rev=1.8&mark=56-57#46

Updated

•

16 years ago

Whiteboard: [RC2?] → [RC2-]

Assignee

Updated

•

16 years ago

Blocks: 434046

Assignee

Updated

•

16 years ago

Blocks: 410544

Assignee

Comment 29

•

16 years ago

Comment on attachment 322627 [details] [diff] [review]
resolve DEFAULT_GUI_FONT name before looking for an entry v1.1

This doesn't resolve the crashes in bug 410544 and bug 434046.  (Attachment 322628 [details] [diff] does.)

Attachment #322627 - Attachment is obsolete: true

Attachment #322627 - Flags: review?(pavlov)

Assignee

Comment 30

•

16 years ago

(In reply to comment #23)
> I think we probably want to do those in the opposite order, but i'm not
> sure...

I choose the order based on this information:

http://blogs.msdn.com/michkap/archive/2006/04/28/585735.aspx
http://blogs.msdn.com/oldnewthing/archive/2005/07/07/436435.aspx

Stuart:  Do you think having mFonts and mFontEntries of zero length poses any higher risk than having mFontEntries with 1 NULL entry?  Or is one as bad as the other.  If you think there's a difference I can put together a patch the goes back to FindFontEntry but still uses SPI_GETNONCLIENTMETRICS.

Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)

Assignee

Comment 31

•

16 years ago

I'm setting 1.9? to bring attention to the fact we can probably fix this.

The patch here resolves bug 410544 and bug 434046 and so would probably resolve this too.

I know we are past the code freeze for rc2, but I don't know how firm a deadline that is, so I'm asking whether we can still get something in.

Flags: blocking1.9- → blocking1.9?

Comment 32

•

16 years ago

(In reply to comment #31)
> I'm setting 1.9? to bring attention to the fact we can probably fix this.
> 
> The patch here resolves bug 410544 and bug 434046 and so would probably resolve
> this too.
> 
> I know we are past the code freeze for rc2, but I don't know how firm a
> deadline that is, so I'm asking whether we can still get something in.

It's firm -- like, there's someone waiting to press a button.  This patch doesn't have tests and this code has been fragile in the past; has anyone been able to test with the proposed patch at least to make sure that it fixes the problem described here?  Have we run the test suites with the patch to make sure we didn't regress anything else?  Either way, I think that this ship has sailed -- we should fix this for 3.0.1.

Assignee

Comment 33

•

16 years ago

(In reply to comment #32)
> has anyone been
> able to test with the proposed patch at least to make sure that it fixes the
> problem described here?

Yes.  See bug 410544 and bug 434046.  (We don't really have any STR for the crash reports for this bug.)

> Have we run the test suites with the patch to make
> sure we didn't regress anything else?

No.

> Either way, I think that this ship has
> sailed -- we should fix this for 3.0.1.

There's enough uncertainty about what the real cause of the problem is, that waiting for 3.0.1 seems a reasonable option.

Comment 34

•

16 years ago

Attached patch updated patch — Details — Splinter Review

I think it is pretty important we fix this.  It is sadly pretty easy to hit this case (which won't normally fail except on Windows where your default GUI and system fonts are localized.)  I've modified karl's patch a little bit to just do one resolution call and append both.  This also appends the DEFAULT_GUI_FONT stuff first so that the behavior we had before will continue to be the same.

Attachment #322628 - Attachment is obsolete: true

Attachment #322863 - Flags: review?(mozbugz)

Attachment #322628 - Flags: review?(pavlov)

Assignee

Comment 35

•

16 years ago

Comment on attachment 322863 [details] [diff] [review]
updated patch

(In reply to comment #30)
> Stuart:  Do you think having mFonts and mFontEntries of zero length poses any
> higher risk than having mFontEntries with 1 NULL entry?  Or is one as bad as
> the other?

We would crash either way so there should be no change here.

This patch would fix two known crashes, so is worth considering.

Attachment #322863 - Flags: review?(mozbugz) → review+

Comment 36

•

16 years ago

There's a lot of back and forth in this bug. What I'm reading is:

 - the set of users affected is unknown, but the effect is pretty severe
 - the code involved is fragile and doesn't have good test coverage
 - it's unclear how invasive this patch is, or how well tested it is

Can someone break it down for me like I'm a UI guy pretending I know how to make these sorts of decisions?

Reporter

Comment 37

•

16 years ago

(In reply to comment #36)
>  - the set of users affected is unknown, but the effect is pretty severe

Well, we know it's a start-up crash and is the #10 topcrasher overall, #6 on Windows (it's also Windows-only). It has ~2500 reports in crash-stats right now and, assuming not everyone submits (though we have not idea how many), that seems like a fairly high number.

I can't speak to the other points.

Comment 38

•

16 years ago

some set of unknown users fail for an unknown cause in a known location that we know how to work-around is what it boils down to.  in my expert opinion, it is very super duper safe to add additional fonts to the set of things to try for fallback.  worst case, we still crash for that set of people.

Comment 39

•

16 years ago

Comment on attachment 322863 [details] [diff] [review]
updated patch

a=beltzner, after a lot of discussion with Stuart and Shaver about relative safety, it is our belief that this does nothing worse and potentially fixes the issue.

Stuart will also file a follow-up bug to land afterwards that will create a fatal assertion to the crash case in debug builds to help us figure this out more thoroughly later.

Attachment #322863 - Flags: approval1.9+

Comment 40

•

16 years ago

checked in.

Status: ASSIGNED → RESOLVED

Closed: 16 years ago

Resolution: --- → FIXED

Updated

•

16 years ago

Flags: blocking1.9? → blocking1.9-

Whiteboard: [RC2-] → [RC2+]

Comment 41

•

16 years ago

Karl, can you send the people reporting crashes the latest nightlies with Stuart's patch to ensure that it fixes things for them?

Guillaume Romagny

Comment 42

•

16 years ago

Hi,

I have opened related bug 410544

I have just tested 
firefox/nightly/2008-05-29-01-mozilla1.9.0/ on my WinXP 64bits

it seems to be working fine. Thanks!

Guillaume

Lance McCord

Comment 43

•

16 years ago

I opened bug 435350

I just tested build 2008052906 on Vista SP1 x64 and am still crashing when the three fonts I identified above are absent. Crash report: http://tinyurl.com/5q6sq9

Assignee

Comment 44

•

16 years ago

(In reply to comment #43)

Thanks for testing, Lance.  I'm going to reopen bug 435350 as that has some specific steps to reproduce.

This bug was filed based on a particular crash signature but apparently there was more than one way to produce the same crash (as the bug 410544 situation has been resolved).

At this stage we do not know what proportion of the crashes are fixed and how many remain outstanding.  Also, the patch here has changed the stack signature of the crash.

Assignee

Comment 45

•

16 years ago

FWIW roughly 90% of the crashes reported on RC1 were "Windows NT 5.1.2600" i.e XP (with various service packs).

john

Comment 46

•

16 years ago

I'd submitted a number of these crash-on-start reports (with comments) in both b5 and RC1, and most recently with the current trunk build:

 http://crash-stats.mozilla.com/report/index/86e2b61e-2e99-11dd-9541-001cc45a2c28?date=2008-05-30-22

But RC2 installs, loads, and runs just fine: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008052906 Firefox/3.0.