513192 - Crash [@nsAString_internal::BeginReading(nsReadingIterator<unsigned short>&) ]

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Atsushi Sakai]

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

Comment 1

[Atsushi Sakai]

Attachment: testcase

Comment 2

[Atsushi Sakai]

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.

Comment 3

[Takuro Ashie [:ashie]]

Attachment: Crash fix.

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?

Comment 4

[Takuro Ashie [:ashie]]

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

Comment 5

[Takuro Ashie [:ashie]]

Attachment: Patch v2

Increment rank value instead of checking mBestMatch.

Comment 6

[Jonathan Kew [:jfkthame]]

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.

Comment 7

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

Attachment: updated with review comments

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.

Comment 8

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

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

Comment 9

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

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/4a75e600f301

Comment 10

[Atsushi Sakai]

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 11

[Jonathan Kew [:jfkthame]]

Comment on attachment 399675 [details] [diff] [review]
Patch v2

Setting r+ for the record (already landed).