1190560 - crash in SkScalerContext::MakeRec(SkPaint const&, SkDeviceProperties const*, SkMatrix const*, SkScalerContextRec*)

Status: RESOLVED WORKSFORME

(Core :: Graphics, defect)

Description

[(Away)]

This bug was filed from the Socorro interface and is 
report bp-966884b6-7634-44a2-a8f4-ded152150802.
=============================================================

New crash first seen in build 20150731030206: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=62469b20ec84&tochange=ca53d4297f02 

As best as I can tell, |paint| is a null reference on the call to getTextSize, which makes no sense because we survived the call to getTypeface. Hopefully I'm wrong.

0 	xul.dll 	SkScalerContext::MakeRec(SkPaint const&, SkDeviceProperties const*, SkMatrix const*, SkScalerContextRec*) 	gfx/skia/skia/src/core/SkPaint.cpp
1 	xul.dll 	SkPaint::descriptorProc(SkDeviceProperties const*, SkMatrix const*, void (*)(SkTypeface*, SkDescriptor const*, void*), void*, bool) 	gfx/skia/skia/src/core/SkPaint.cpp
2 	xul.dll 	SkPaint::detachCache(SkDeviceProperties const*, SkMatrix const*, bool) 	gfx/skia/skia/src/core/SkPaint.cpp
3 	xul.dll 	SkTextToPathIter::SkTextToPathIter(char const* const, unsigned int, SkPaint const&, bool) 	gfx/skia/skia/src/core/SkPaint.cpp
4 	xul.dll 	SkPaint::getPosTextPath(void const*, unsigned int, SkPoint const* const, SkPath*) 	gfx/skia/skia/src/core/SkPaint.cpp
5 	xul.dll 	mozilla::gfx::ScaledFontBase::GetSkiaPathForGlyphs(mozilla::gfx::GlyphBuffer const&) 	gfx/2d/ScaledFontBase.cpp
6 	xul.dll 	mozilla::gfx::ScaledFontBase::GetPathForGlyphs(mozilla::gfx::GlyphBuffer const&, mozilla::gfx::DrawTarget const*) 	gfx/2d/ScaledFontBase.cpp
7 	xul.dll 	mozilla::gfx::ScaledFontDWrite::GetPathForGlyphs(mozilla::gfx::GlyphBuffer const&, mozilla::gfx::DrawTarget const*) 	gfx/2d/ScaledFontDWrite.cpp
8 	xul.dll 	mozilla::dom::CanvasBidiProcessor::DrawText(int, int) 	dom/canvas/CanvasRenderingContext2D.cpp

Matt, possibly bug 1150944?

Comment 1

[Matt Woodrow (:mattwoodrow)]

That seems like the most likely candidate, yeah.

How did you come to that conclusion? Just that getTextSize() is the closest thing that will access x+0x28 in the near vicinity of the crashing line?

Comment 2

[(Away)]

Yeah. And we are just after the two previous lines that take typeface->uniqueID and stick it into two different fields of rec.

Comment 3

[(Away)]

5e8730d6 55              push    ebp
5e8730d7 8bec            mov     ebp,esp
5e8730d9 83ec10          sub     esp,10h
5e8730dc 53              push    ebx
5e8730dd 8bd9            mov     ebx,ecx  ; ebx is |paint|
5e8730df 8955f0          mov     dword ptr [ebp-10h],edx
5e8730e2 56              push    esi
5e8730e3 57              push    edi
5e8730e4 8b03            mov     eax,dword ptr [ebx]  ; this is |paint.getTypeface()|, seems fine 
5e8730e6 8945f8          mov     dword ptr [ebp-8],eax
5e8730e9 85c0            test    eax,eax
5e8730eb 7522            jne     xul!SkScalerContext::MakeRec+0x39 (5e87310f) ; did not take this jump
5e8730ed a10455195f      mov     eax,dword ptr [xul!`GrTBackendEffectFactory<XferEffect>::getInstance'::`2'::gInstanceMem+0x14 (5f195504)]
5e8730f2 8945f8          mov     dword ptr [ebp-8],eax
5e8730f5 85c0            test    eax,eax
5e8730f7 7516            jne     xul!SkScalerContext::MakeRec+0x39 (5e87310f) ; did not take this jump
5e8730f9 50              push    eax
5e8730fa e83baf0100      call    xul!SkTypeface::CreateDefault (5e88e03a)
5e8730ff 59              pop     ecx
5e873100 8bd0            mov     edx,eax
5e873102 b90455195f      mov     ecx,offset xul!`GrTBackendEffectFactory<XferEffect>::getInstance'::`2'::gInstanceMem+0x14 (5f195504)
5e873107 e836a30100      call    xul!Private::try_cas<SkTypeface *,&SkTypeface::DeleteDefault> (5e88d442)
5e87310c 8945f8          mov     dword ptr [ebp-8],eax
5e87310f 8b750c          mov     esi,dword ptr [ebp+0Ch]
5e873112 8b480c          mov     ecx,dword ptr [eax+0Ch]
5e873115 8b7d08          mov     edi,dword ptr [ebp+8] 
5e873118 890e            mov     dword ptr [esi],ecx    ; rec->fOrigFontID = typeface->uniqueID();
5e87311a 894e04          mov     dword ptr [esi+4],ecx  ; rec->fFontID = rec->fOrigFontID;
5e87311d d94328          fld     dword ptr [ebx+28h]    ; rec->fTextSize = paint.getTextSize();

We couldn't have taken either of the two jumps because then we'd have a sane ebx. At this point my best guess (although an unlikely one) is that one of those two "call" instructions failed to restore the ebx register.

Matt, any chance you could try out one of the affected builds, cause whatever condition requires "SkTypeface::GetDefaultTypeface()", and see what happens to ebx?

Comment 4

[Matt Woodrow (:mattwoodrow)]

(In reply to David Major [:dmajor] from comment #3)
> Matt, any chance you could try out one of the affected builds, cause
> whatever condition requires "SkTypeface::GetDefaultTypeface()", and see what
> happens to ebx?

I can't see any way to do that.

We unconditionally call SkPaint::setTypeface at [1], using the result of GetSkTypeface().

The implementation of that calls SkCreateTypefaceFromCTFont [2], which is infallible.

[1] http://hg.mozilla.org/mozilla-central/annotate/aeb85029c3b3/gfx/2d/ScaledFontBase.cpp#l56
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/skia/skia/src/ports/SkFontHost_mac.cpp#553

Comment 5

[(Away)]

Well, I can try meddling with values in the debugger, just to test the theory. How do I trigger this codepath? Do I need special prefs or gfx capabilities?

Comment 6

[Matt Woodrow (:mattwoodrow)]

Just drawing text from canvas on OSX nightly should get you there, pretty much any <canvas> demo should be fine.

Comment 7

[(Away)]

Um, but these are all Windows?

Comment 8

[Matt Woodrow (:mattwoodrow)]

Oh, I totally missed that.

My pref change for OSX seems unlikely to have caused this then!

Comment 9

[Matt Woodrow (:mattwoodrow)]

The rest of the things I said stand though, the windows implementation of the font lookup should also be infallible.

Comment 10

[(Away)]

I haven't been able to hit this breakpoint on Windows on various canvas text demos.

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

Closing because no crashes reported for 12 weeks.

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a regression, but please revert this change in case of error.