Closed Bug 593296 Opened 14 years ago Closed 14 years ago

crash [@ gfxHarfBuzzShaper::GetKerning(unsigned short, unsigned short) ]

Categories

(Core :: Graphics, defect)

x86
All
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta7+

People

(Reporter: scoobidiver, Assigned: jfkthame)

References

Details

(Keywords: crash, regression, topcrash)

Crash Data

Attachments

(3 files, 1 obsolete file)

Build : Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b6pre) Gecko/20100902 Firefox/4.0b6pre

This is a new crash signature which was introduced by this build.
It is #2 crasher for this build. For the moment, it happens only on Windows OS.

Signature	gfxHarfBuzzShaper::GetKerning(unsigned short, unsigned short)
UUID	c317e4b5-d023-410e-a152-f61c22100903
Time 	2010-09-03 00:17:42.560134
Uptime	107
Last Crash	138003 seconds (1.6 days) before submission
Install Age	20025 seconds (5.6 hours) since version was first installed.
Product	Firefox
Version	4.0b6pre
Build ID	20100902095949
Branch	2.0
OS	Windows NT
OS Version	6.1.7600
CPU	x86
CPU Info	GenuineIntel family 6 model 14 stepping 12
Crash Reason	EXCEPTION_ACCESS_VIOLATION
Crash Address	0xa5db000
App Notes 	AdapterVendorID: 8086, AdapterDeviceID: 27a2

Crashing Thread
Frame 	Module 	Signature [Expand] 	Source
0 	xul.dll 	gfxHarfBuzzShaper::GetKerning 	gfx/thebes/gfxHarfBuzzShaper.cpp:545
1 	xul.dll 	HBGetKerning 	gfx/thebes/gfxHarfBuzzShaper.cpp:642
2 	xul.dll 	hb_truetype_kern 	gfx/harfbuzz/src/hb-ot-shape.cc:570
3 	xul.dll 	hb_ot_shape 	gfx/harfbuzz/src/hb-ot-shape.cc:639
4 	xul.dll 	gfxHarfBuzzShaper::InitTextRun 	gfx/thebes/gfxHarfBuzzShaper.cpp:852
5 	xul.dll 	gfxGDIFont::InitTextRun 	gfx/thebes/gfxGDIFont.cpp:162
6 	xul.dll 	gfxFontGroup::InitTextRun 	gfx/thebes/gfxFont.cpp:2245
7 	xul.dll 	gfxFontGroup::InitTextRun 	gfx/thebes/gfxFont.cpp:2213
8 	xul.dll 	gfxFontGroup::MakeTextRun 	gfx/thebes/gfxFont.cpp:2188
9 	xul.dll 	TextRunWordCache::MakeTextRun 	gfx/thebes/gfxTextRunWordCache.cpp:693
10 	xul.dll 	MakeTextRun 	layout/generic/nsTextFrameThebes.cpp:450
11 	xul.dll 	BuildTextRunsScanner::BuildTextRunForFrames 	layout/generic/nsTextFrameThebes.cpp:1810
12 	xul.dll 	BuildTextRunsScanner::FlushFrames 	layout/generic/nsTextFrameThebes.cpp:1242
13 	xul.dll 	BuildTextRuns 	layout/generic/nsTextFrameThebes.cpp:1176
14 	xul.dll 	nsTextFrame::EnsureTextRun 	layout/generic/nsTextFrameThebes.cpp:2043
15 	xul.dll 	nsTextFrame::AddInlineMinWidthForFlow 	layout/generic/nsTextFrameThebes.cpp:5797
16 	xul.dll 	nsTextFrame::AddInlineMinWidth 	layout/generic/nsTextFrameThebes.cpp:5909
17 	xul.dll 	nsContainerFrame::DoInlineIntrinsicWidth 	layout/generic/nsContainerFrame.cpp:647
18 	xul.dll 	nsInlineFrame::AddInlineMinWidth 	layout/generic/nsInlineFrame.cpp:209
19 	xul.dll 	nsBlockFrame::GetMinWidth 	layout/generic/nsBlockFrame.cpp:732
20 	xul.dll 	nsLayoutUtils::IntrinsicForContainer 	layout/base/nsLayoutUtils.cpp:1984
21 	xul.dll 	nsTableCellFrame::GetMinWidth 	layout/tables/nsTableCellFrame.cpp:726
22 	xul.dll 	GetWidthInfo 	layout/tables/BasicTableLayoutStrategy.cpp:113
23 	xul.dll 	BasicTableLayoutStrategy::ComputeColumnIntrinsicWidths 	layout/tables/BasicTableLayoutStrategy.cpp:308
24 	xul.dll 	BasicTableLayoutStrategy::ComputeIntrinsicWidths 	layout/tables/BasicTableLayoutStrategy.cpp:418
25 	xul.dll 	BasicTableLayoutStrategy::GetMinWidth 	layout/tables/BasicTableLayoutStrategy.cpp:74
26 	xul.dll 	nsTableFrame::GetMinWidth 	layout/tables/nsTableFrame.cpp:1491
27 	xul.dll 	nsTableFrame::TableShrinkWidthToFit 	layout/tables/nsTableFrame.cpp:1546
28 	xul.dll 	nsTableFrame::ComputeAutoSize 	layout/tables/nsTableFrame.cpp:1578
29 	xul.dll 	nsFrame::ComputeSize 	layout/generic/nsFrame.cpp:3192
30 	xul.dll 	nsFieldSetFrame::ComputeSize 	layout/tables/nsTableFrame.cpp:1531
31 	xul.dll 	ChildShrinkWrapWidth 	layout/tables/nsTableOuterFrame.cpp:584
32 	xul.dll 	nsTableOuterFrame::ComputeAutoSize 	layout/tables/nsTableOuterFrame.cpp:611
33 	xul.dll 	nsFrame::ComputeSize 	layout/generic/nsFrame.cpp:3192
34 	xul.dll 	nsBlockFrame::WidthToClearPastFloats 	layout/generic/nsBlockFrame.cpp:6920
35 	xul.dll 	nsBlockReflowState::ComputeBlockAvailSpace 	layout/generic/nsBlockReflowState.cpp:292
36 	xul.dll 	nsBlockFrame::ReflowBlockFrame 	layout/generic/nsBlockFrame.cpp:3087
37 		@0xb4303f7 

The regression window is :
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f47972d05473&tochange=dc2939f2640d
It could be a consequence of the bug 569531 fix
blocking2.0: --- → ?
BTW mine occurs only on netbooks , on my Desktop (ATI HD 5770 , Athlon II 620 X4) , all works perfect. So is it some sort of driver problem?
My crashes occur on cold startups if I uncheck the option to "Use hardware acceleration when possible". I have to do that because I will get content window freezing probably because I have an Intel (HD) Graphics display adapter on my system.
Now, I am getting them on cold starts with "Use hardware
acceleration when possible" checked even though hardware acceleration will be disabled because of my display adapter. Apparently, now I had to uncheck it so I could get Minefield to start.
This one reoccured on my Desktop this time (ATi 5770 with Cat 10.7 and Athlon II X620)

http://crash-stats.mozilla.com/report/index/5dc420d9-d660-472d-a63b-718332100910

This time , i disabled Hardware acceleration , restarted , my pages started loading , and boom.
This is the #2 topcrash on nightlies; it looks like it started in the builds of Sept 2, so probably a regression from bug 569531.  Would be good to get this fixed for beta6.
blocking2.0: ? → beta6+
Keywords: topcrash
If anyone has a testcase that consistently triggers this crash, that would be REALLY helpful. I have not yet been able to reproduce it locally.

I notice that ALL the crash reports I've checked indicate that the GDI text path is being used; I haven't seen this happen with D2D/DirectWrite. But despite explicitly setting my browser to use GDI, I have not encountered this crash. Maybe depends on the use of a particular font?
OS: Windows 7 → Windows XP
Assignee: nobody → jfkthame
blocking2.0: beta6+ → betaN+
moved to #4 topcrash on the trunk, but we should still try and get this fixed before b7.

208 total crashes for gfxHarfBuzzShaper::GetKerning.unsigned.short..unsigned.short. on 20100913-crashdata.csv
88 startup crashes inside 30 sec.
154 startup crashes inside 3 min.
74 repeated crashes inside 3 min. of last crash

os breakdown
gfxHarfBuzzShaper::GetKerning.unsigned.short..unsigned.short.Total 208
Win5.1  0.02
Win6.0  0.00
Win6.1  0.98

possible test urls from yesterday/

  20 http://g.mozest.com/
  13 \N
   7 http://vkontakte.ru/login.php
   6 http://www.cnbeta.com/
   6 http://www.chelmusic.ru/
   6 http://vkontakte.ru/
   6 http://us.mc396.mail.yahoo.com/mc/md.php?en=CP1252&v=1

seems to be associated with a lot of domains in russia and china.

  63 http://vkontakte.ru
  20 http://g.mozest.com
  13 \N//
  12 http://www.facebook.com
   8 http://www.cnbeta.com
   7 http://www.chelmusic.ru
   6 http://us.mc396.mail.yahoo.com
   5 http://www.odnoklassniki.ru
   4 http://www.google.co.in

   5 http://vkontakte.ru/index.php
   4 http://www.facebook.com/home.php?ref=logo
I still have not succeeded in reproducing this crash, but during code inspection I noticed one issue with management of the kern table: the harfbuzz shaper does not release it correctly. Maybe it's possible for this to lead to use of a stale entry from the gfxFontEntry table cache, in circumstances where the entry has been deleted? That cache is not used by the dwrite or OS X backends, which could explain why we've only seen this crash under GDI.

Not having a way to reproduce the crash, I can't be sure whether this will help, but it should be fixed for correctness and leak-prevention anyway. So as a first step, I suggest we land this fix on trunk and see whether it affects nightly crash-stats.
Attachment #475530 - Flags: review?(jdaggett)
Comment on attachment 475530 [details] [diff] [review]
patch, v1 - correctly release kern table

Looks fine.  I don't see how this could cause a crash however so I think it would be better to leave this open after you land this patch.
Attachment #475530 - Flags: review?(jdaggett) → review+
Pushed http://hg.mozilla.org/mozilla-central/rev/5b4969afa033

Keeping this bug open, at least until we see whether crash-stats change. The fix to kern table management should at least make any remaining issue easier to debug, as the refcount will now be updated correctly.
looks like volume might have actually increased on builds after the 16th.  I could be we are growing the number of users on those builds or sign of regression.

date     tl crashes at, count build, count build, ...
         gfxHarfBuzzShaper::GetKerning.unsigned.short..unsigned.short.

20100910 206 ,, 134 4.0b6pre2010090904, 28 4.0b6pre2010091004, ...
20100911 176 ,, 56 4.0b6pre2010091004, 52 4.0b6pre2010090904, ...
20100912 211 ,, 70 4.0b6pre2010091204, 58 4.0b6pre2010091104, ...
20100913 208 ,, 86 4.0b6pre2010091204, 63 4.0b6pre2010091304, ...
20100914 147 ,, 85 4.0b6pre2010091304, 24 4.0b7pre2010091404, ...
20100915 88 ,, 67 4.0b7pre2010091404, 13 4.0b6pre2010091304, ... 
20100916 185 ,, 60 4.0b7pre2010091404, 45 4.0b7pre2010091507, ...

20100917 163 ,, 52 4.0b7pre2010091704, 47 4.0b7pre2010091604, ...
20100918 298 ,, 152 4.0b7pre2010091704, 98 4.0b7pre2010091804, ...
20100919 425 ,, 210 4.0b7pre2010091804, 135 4.0b7pre2010091904, ...
I'm still not able to reproduce this, but it's obviously quite common in the wild - probably linked to particular fonts, though as yet I don't see what kind of font anomaly could lead to a crash here.

I'd be interested in hearing from anyone who can reliably trigger the crash, and would be willing to run a custom debugging build with extra logging that might help us identify the cause.
(In reply to comment #15)
> I'm still not able to reproduce this, but it's obviously quite common in the
> wild - probably linked to particular fonts, though as yet I don't see what kind
> of font anomaly could lead to a crash here.
> 
> I'd be interested in hearing from anyone who can reliably trigger the crash,
> and would be willing to run a custom debugging build with extra logging that
> might help us identify the cause.

We really need to analyze the crashdump minidump files.  I'm trying to do that but I'm getting stuck in IT limbo land, grumble.
The URL distribution for these crashes is sort of interesting (with a substantial Russian bias):

    148 vkontakte.ru
     94 www.orkut.com.br
     46 about:blank
     24 www.facebook.com
     19 
     12 www.odnoklassniki.ru
     10 forum.mozilla-russia.org
        ...

I'd note that ВКонтакте has a plugin, too, and I wonder if that could be involved.  (We don't have crash correlation reports for nightlies...)


Was there something in particular you wanted from a minidump?
Also, the distribution of Windows versions is interesting:
     16 5.1.2600 Service Pack 3
     10 6.1.7100
      3 6.1.7264
    437 6.1.7600
      6 6.1.7600 Service Pack 1
      8 6.1.7600 Service Pack 2
      1 6.1.7600 Service Pack 3

(Like the previous list, this is from the crashes on 2010-09-20.)
(In reply to comment #18)
 
> Was there something in particular you wanted from a minidump?

Looking at the code it's hard to see how exactly a crash could occur, so a minidump will help pinpoint what's causing the crash and then we can work back from there.
So I'm looking at a minidump now, for https://crash-stats.mozilla.com/report/index/1750dc12-9a3f-4ca9-9cc7-110c12100921 .  I think it looks like GetKernValueFmt0 got inlined, and the crash is inside of it.  MSVC inlines a *lot* with PGO, and leaves no debugging information for anything inside the inline.

In particular, the source-annotated disassembly looks like this:

   540:             PRUint8 format = (coverage >> 8);
   541:             switch (format) {
661A6359  test        ch,ch 
661A635B  jne         gfxHarfBuzzShaper::GetKerning+1A6h (661A63F6h) 
   542:             case 0:
   543:                 GetKernValueFmt0(st0 + 1, subtableLen - sizeof(*st0),
   544:                                  aFirstGlyph, aSecondGlyph, value,
   545:                                  (coverage & KERN0_COVERAGE_OVERRIDE) != 0,
   546:                                  (coverage & KERN0_COVERAGE_MINIMUM) != 0);
661A6361  xor         esi,esi 
661A6363  test        cl,2 
661A6366  jne         66560EC9 
661A636C  mov         dword ptr [esp+34h],esi 
661A6370  test        cl,8 
661A6373  jne         66560EE7 
661A6379  mov         dword ptr [esp+38h],esi 
661A637D  xor         ecx,ecx 
661A637F  mov         ch,byte ptr [eax+6] 
661A6382  lea         esi,[eax+0Eh] 
661A6385  add         edx,eax 
661A6387  mov         cl,byte ptr [eax+7] 
661A638A  lea         ecx,[ecx+ecx*2] 
661A638D  lea         edi,[esi+ecx*2] 
661A6390  cmp         edx,edi 
661A6392  jb          gfxHarfBuzzShaper::GetKerning+1A6h (661A63F6h) 
661A6394  mov         ebp,dword ptr [esp+20h] 
661A6398  shl         ebp,10h 
661A639B  add         ebp,dword ptr [esp+24h] 
661A639F  cmp         esi,edi 
661A63A1  jae         gfxHarfBuzzShaper::GetKerning+18Eh (661A63DEh) 
661A63A3  mov         ecx,edi 
661A63A5  sub         ecx,esi 
661A63A7  mov         eax,2AAAAAABh 
661A63AC  imul        ecx  
661A63AE  mov         eax,edx 
661A63B0  shr         eax,1Fh 
661A63B3  add         eax,edx 
661A63B5  cdq              
661A63B6  sub         eax,edx 
661A63B8  sar         eax,1 
661A63BA  lea         edx,[eax+eax*2] 
661A63BD  movzx       ecx,byte ptr [esi+edx*2+1] 
661A63C2  mov         ch,byte ptr [esi+edx*2] 
661A63C5  lea         eax,[esi+edx*2] 
661A63C8  movzx       edx,byte ptr [eax+3] 
661A63CC  mov         dh,byte ptr [eax+2] 
661A63CF  shl         ecx,10h 
661A63D2  add         ecx,edx 
661A63D4  cmp         ecx,ebp 
661A63D6  jb          gfxHarfBuzzShaper::GetKerning+1E2h (661A6432h) 
661A63D8  mov         edi,eax 
661A63DA  cmp         esi,edi 
661A63DC  jb          gfxHarfBuzzShaper::GetKerning+153h (661A63A3h) 
661A63DE  xor         eax,eax 
661A63E0  mov         ah,byte ptr [esi] 
661A63E2  xor         ecx,ecx 
661A63E4  mov         ch,byte ptr [esi+2] 
661A63E7  mov         al,byte ptr [esi+1] 
661A63EA  mov         cl,byte ptr [esi+3] 
661A63ED  shl         eax,10h 
661A63F0  add         eax,ecx 
661A63F2  cmp         eax,ebp 
661A63F4  je          gfxHarfBuzzShaper::GetKerning+1F1h (661A6441h) 
   515:         PRUint32 offs = sizeof(KernTableVersion0);
   516:         for (PRUint16 i = 0; i < nTables; ++i) {
661A63F6  mov         eax,dword ptr [esp+18h] 
   547:                 break;

(I think a few less-used bits of the inlined function probably got moved out-of-line below, though; this is probably only the most-common-codepath of it.)

And we're crashing with the following registers (note EIP in the middle of the code above):

EAX = 00000000 EBX = 00001FF8 ECX = 027401D9 EDX = 000001D9 
ESI = 13991000 EDI = 13991000 EIP = 661A63E0 ESP = 001F1C74 
EBP = 058B05D0 EFL = 00210246 

The crash is trying to dereference ESI, which is apparently not a good pointer.
Or, with a little more explanation:

661A6361  xor         esi,esi 

jump elsewhere if (coverage & KERN0_COVERAGE_MINIMUM)
661A6363  test        cl,2 
661A6366  jne         66560EC9 

save ESI on the stack
661A636C  mov         dword ptr [esp+34h],esi 

jump elsewhere if (coverage & KERN0_COVERAGE_OVERRIDE)
661A6370  test        cl,8 
661A6373  jne         66560EE7 

save ESI on the stack
661A6379  mov         dword ptr [esp+38h],esi 

Half of the swap to put hdr->nPairs in ECX (not sure why EAX is 6 below hdr, but anyway)
661A637D  xor         ecx,ecx 
661A637F  mov         ch,byte ptr [eax+6] 

This gets lo in ESI.
661A6382  lea         esi,[eax+0Eh] 

This changes EDX from aSubtableLen to aSubtable + aSubtableLen
661A6385  add         edx,eax 

The other half of the swap to put hdr->nPairs in ECX (ditto)
661A6387  mov         cl,byte ptr [eax+7] 

This gets hi into EDI (turning ECX into hdr->nPairs * 3 in the process)
661A638A  lea         ecx,[ecx+ecx*2] 
661A638D  lea         edi,[esi+ecx*2] 

288     if (reinterpret_cast<const char*>(aSubtable) + aSubtableLen <
289         reinterpret_cast<const char*>(hi)) {
290         // subtable is not large enough to contain the claimed number
291         // of kern pairs, so just ignore it
292         return;
293     }
661A6390  cmp         edx,edi 
661A6392  jb          gfxHarfBuzzShaper::GetKerning+1A6h (661A63F6h) 

This does key = KERN_PAIR_KEY(aFirstGlyph, aSecondGlyph) with the result in EBP.
661A6394  mov         ebp,dword ptr [esp+20h] 
661A6398  shl         ebp,10h 
661A639B  add         ebp,dword ptr [esp+24h] 

Compare lo and hi (top of while loop, to enter or not)
661A639F  cmp         esi,edi 
661A63A1  jae         gfxHarfBuzzShaper::GetKerning+18Eh (661A63DEh) 

{

(I won't annotate the loop other than marking where it starts and ends)

661A63A3  mov         ecx,edi 
661A63A5  sub         ecx,esi 
661A63A7  mov         eax,2AAAAAABh 
661A63AC  imul        ecx  
661A63AE  mov         eax,edx 
661A63B0  shr         eax,1Fh 
661A63B3  add         eax,edx 
661A63B5  cdq              
661A63B6  sub         eax,edx 
661A63B8  sar         eax,1 
661A63BA  lea         edx,[eax+eax*2] 
661A63BD  movzx       ecx,byte ptr [esi+edx*2+1] 
661A63C2  mov         ch,byte ptr [esi+edx*2] 
661A63C5  lea         eax,[esi+edx*2] 
661A63C8  movzx       edx,byte ptr [eax+3] 
661A63CC  mov         dh,byte ptr [eax+2] 
661A63CF  shl         ecx,10h 
661A63D2  add         ecx,edx 
661A63D4  cmp         ecx,ebp 
661A63D6  jb          gfxHarfBuzzShaper::GetKerning+1E2h (661A6432h) 
661A63D8  mov         edi,eax 

Compare lo and hi (bottom of while loop, to loop)
661A63DA  cmp         esi,edi 
661A63DC  jb          gfxHarfBuzzShaper::GetKerning+153h (661A63A3h) 

}

This is all KERN_PAIR_KEY(lo->left, lo->right), getting lo->left into EAX and lo->right into ECX and then adding them together into EAX.  So I think the crash is dereferencing a bad pointer for "lo" on line 307:
307     if (KERN_PAIR_KEY(lo->left, lo->right) == key) {
661A63DE  xor         eax,eax 
661A63E0  mov         ah,byte ptr [esi] 
661A63E2  xor         ecx,ecx 
661A63E4  mov         ch,byte ptr [esi+2] 
661A63E7  mov         al,byte ptr [esi+1] 
661A63EA  mov         cl,byte ptr [esi+3] 
661A63ED  shl         eax,10h 
661A63F0  add         eax,ecx 

and compare that result to EBP (key, still untouched), and go off elsewhere if they're actually equal (I guess that's the unusual case?)
661A63F2  cmp         eax,ebp 
661A63F4  je          gfxHarfBuzzShaper::GetKerning+1F1h (661A6441h)
Oh, and off the stack, I can read:

ESP+20H (aFirstGlyph): 0x58b
ESP+24H (aSecondGlyph): 0x5d0
... which also matches the value of key in EBP.

Could the problem be this: the binary search algorithm can end with lo and hi both pointing to the value that was in hi going into the loop, which (unlike all values x such that lo <= x < hi) is not a valid value.  If hi happens to be right at a page boundary, you can fault dereferencing it right below the loop.
Maybe the right fix is to put --hi right before entering the loop?  I *think* the loop looks like it assumes both lo and hi are valid possible values and can produce either as a final result (but somebody should check this), whereas (based on the nPairs name and the manner in which hi is checked against the subtable length) I think hi isn't a valid value.

(BTW, I like the page boundary theory since all the crash addresses are multiples of 0x1000.)
Does this make sense based on what this code is trying to do (which I'm only inferring from reading the code)?  And is my math right?
Attachment #477400 - Flags: review?(jfkthame)
And given that this is >20% of our crashes based on
http://crash-stats.mozilla.com/topcrasher/byversion/Firefox/4.0b7pre/3
I think this needs to block beta7.
blocking2.0: betaN+ → beta7+
Actually, also need to check for the 0-pairs case.
Attachment #477400 - Attachment is obsolete: true
Attachment #477403 - Flags: review?(jfkthame)
Attachment #477400 - Flags: review?(jfkthame)
> const KernHeaderFmt0* hdr =
>     reinterpret_cast<const KernHeaderFmt0*>(aSubtable);
> 
> const KernPair *lo = reinterpret_cast<const KernPair*>(hdr + 1);
> const KernPair *hi = lo + PRUint16(hdr->nPairs);
> 
> if (reinterpret_cast<const char*>(aSubtable) + aSubtableLen <
>     reinterpret_cast<const char*>(hi)) {
>     // subtable is not large enough to contain the claimed number
>     // of kern pairs, so just ignore it
>     return;
> }
> 
> #define KERN_PAIR_KEY(l,r) (PRUint32((PRUint16(l) << 16) + PRUint16(r)))
> 
> PRUint32 key = KERN_PAIR_KEY(aFirstGlyph, aSecondGlyph);
> while (lo < hi) {
>     const KernPair *mid = lo + (hi - lo) / 2;
>     if (KERN_PAIR_KEY(mid->left, mid->right) < key) {
>         lo = mid + 1;
>     } else {
>         hi = mid;
>     }
> }
> 
> if (KERN_PAIR_KEY(lo->left, lo->right) == key) {
>     if (aIsOverride) {
>         aValue = PRInt16(lo->value);
>     } else if (aIsMinimum) {
>         aValue = PR_MAX(aValue, PRInt16(lo->value));
>     } else {
>         aValue += PRInt16(lo->value);
>     }
> }

The 'hi' variable will always be 1 beyond the end of the table. 
Within the binary search loop an invalid access won't occur but it
would be possible in the '(KERN_PAIR_KEY(lo->left, lo->right) == key)'
expression.

Example scenario:

nPairs = 2
aFirstGlyph is a glyph index greater than any kern pair glyph index.

Within the binary search loop:

  mid = lo + 1
  KERN_PAIR_KEY(mid->left, mid->right) < key ==> true
  lo = mid + 1 ==> == hi
  lo < hi ==> false

At this point lo == hi and 'KERN_PAIR_KEY(lo->left, lo->right) == key'
will access beyond the end of the subtable.
Yes, the analysis here looks correct, and also explains the prevalence of Russian and other non-Latin pages in the crash stats. The case where the search ends with lo == initial value of hi will happen when the (left, right) glyph pair being searched-for is greater than any pair in the table. Some fonts that include both Latin and Cyrillic are not fully kerned but just have kern pairs for common Latin letters, and the Cyrillic glyphs are typically located at higher indexes.
OS: Windows XP → All
Attachment #477407 - Flags: review?(jfkthame)
Comment on attachment 477407 [details] [diff] [review]
don't read past end of table, alternate version

jdaggett's "alternate" patch seems the simplest/cleanest fix, so I think we should take this version.
Attachment #477407 - Flags: review?(jfkthame) → review+
Attachment #477403 - Flags: review?(jfkthame)
Pushed http://hg.mozilla.org/mozilla-central/rev/198709160138
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Checking crash-stats, I don't see any instances of this signature in builds from 2010-09-22 or later, so we should expect the numbers to drop pretty quickly as people update to fresh nightlies (or to beta7, once that's available).
Crash Signature: [@ gfxHarfBuzzShaper::GetKerning(unsigned short, unsigned short) ]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: