Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20090827 Minefield/3.7a1pre Steps to reproduce: 1. Open the testcase. Actual result: Firefox crashes. http://crash-stats.mozilla.com/report/index/bp-58c13147-b8d4-4560-8cb8-1f52a2090827

ozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20090816 Minefield/3.7a1pre BuildID=20090816045208 SourceStamp=ef2e4699b319 works fine. Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20090817 Minefield/3.7a1pre BuildID=20090817050221 SourceStamp=d9742d839d7d fails. regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ef2e4699b319&tochange=d9742d839d7d likely from bug 493280.

Hmm, data->mBestMatch is possible to refer without initializing in current code. Here is a temporal patch to fix this issue. Maybe crean up is needed. Increment the rank after passed fe->mCharacterMap.test(ch)? Or add NULL check into last "if" statement?

I confirmed that this bug is introduced by the bug 493280. In the current code, mMatchRank in FontSearch struct is initialized by 0, although previous equivalent variable (matchRank in FontSearch struct for gfxWindowPlatform) is initialized by -1. So that a result of "rank > data->mMatchRank" at the last of the gfxWindowPlatform::FindFontForCharProc can become "false" with NULL data->mBestMatch. http://hg.mozilla.org/mozilla-central/file/b270bdb573f6/gfx/thebes/src/gfxWindowsPlatform.cpp#l658

Increment rank value instead of checking mBestMatch.

I wasn't able to reproduce the crash myself (maybe it depends on the fonts actually installed?), but your analysis is correct and the code is definitely at risk of crashing here. Just to be a little tidier, would you mind updating your patch to simply initialize rank to 1 instead of 0; there's no reason for a separate increment. A comment might be helpful too, for next time this gets modified (when Windows moves to the gfxPlatformFontList model): - PRInt32 rank = 0; + // initialize rank to 1 so that any match is better than the initial + // value of data->mMatchRank (zero); therefore the first font that + // passes the mCharacterMap.test() will become the mBestMatch until + // a better entry is found + PRInt32 rank = 1; Thanks for tracking this down.

Here's the updated patch -- I'm working on landing all the font pieces on 1.9.2, so wanted to get this in; I assume that r+ from jfkthame from you on this is correct, based on the previous comment.

Pushed as http://hg.mozilla.org/mozilla-central/rev/9950874d8ee0

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20090910 Minefield/3.7a1pre BuildID=20090910153325 SourceStamp=613cf52be14d does not crash with the testcase. -> v. Thanks for fixing this!

Comment on attachment 399675 [details] [diff] [review] Patch v2 Setting r+ for the record (already landed).