880818 - Null cairo font face can crash B2G

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

I get the following when running test_canvas.html mochitest on Android:

#0  INT__moz_cairo_scaled_font_create (font_face=0x0, font_matrix=0x6a0dc300, ctm=0x6a0dc330, options=0x8a3ffb60)
    at /Users/snorp/source/mozilla-central/gfx/cairo/cairo/src/cairo-scaled-font.c:906
#1  0x6f9cc4a0 in FT2FontEntry::CreateScaledFont (this=0x768277e0, aStyle=<optimized out>)
    at /Users/snorp/source/mozilla-central/gfx/thebes/gfxFT2FontList.cpp:183
#2  0x6f9cc4c6 in FT2FontEntry::CreateFontInstance (this=0x768277e0, aFontStyle=0x8a5e6498, aNeedsBold=<optimized out>)
    at /Users/snorp/source/mozilla-central/gfx/thebes/gfxFT2FontList.cpp:208
#3  0x6f9dbde0 in gfxFontEntry::FindOrMakeFont (this=0x768277e0, aStyle=0x8a5e6498, aNeedsBold=<optimized out>)
    at /Users/snorp/source/mozilla-central/gfx/thebes/gfxFont.cpp:203
#4  0x6f9dc2a8 in gfxFontGroup::FindPlatformFont (aName=..., aGenericName=..., aUseFontSet=<optimized out>, aClosure=0x8a5e6480)
    at /Users/snorp/source/mozilla-central/gfx/thebes/gfxFont.cpp:3961
#5  0x6f9d7172 in gfxFontGroup::FontResolverProc (aName=..., aClosure=<optimized out>)
    at /Users/snorp/source/mozilla-central/gfx/thebes/gfxFont.cpp:4214
#6  0x6f9ca756 in gfxAndroidPlatform::ResolveFontName (this=<optimized out>, aFontName=..., 
    aCallback=0x6f9d7165 <gfxFontGroup::FontResolverProc(nsAString_internal const&, void*)>, aClosure=0x6a0dc67c, aAborted=@0x6a0dc4f6: false)
    at /Users/snorp/source/mozilla-central/gfx/thebes/gfxAndroidPlatform.cpp:253
#7  0x6f9d90b8 in gfxFontGroup::ForEachFontInternal (this=0x8a5e6480, aFamilies=..., aLanguage=<optimized out>, aResolveGeneric=<optimized out>, 
    aResolveFontName=true, aUseFontSet=false, 
    fc=0x6f9dc21d <gfxFontGroup::FindPlatformFont(nsAString_internal const&, nsACString_internal const&, bool, void*)>, closure=0x8a5e6480)
    at /Users/snorp/source/mozilla-central/gfx/thebes/gfxFont.cpp:4178
#8  0x6f9d901c in gfxFontGroup::ForEachFontInternal (this=0x8a5e6480, aFamilies=..., aLanguage=<optimized out>, aResolveGeneric=<optimized out>, 
    aResolveFontName=true, aUseFontSet=true, 
    fc=0x6f9dc21d <gfxFontGroup::FindPlatformFont(nsAString_internal const&, nsACString_internal const&, bool, void*)>, closure=0x8a5e6480)
    at /Users/snorp/source/mozilla-central/gfx/thebes/gfxFont.cpp:4151
#9  0x6f9d91ec in gfxFontGroup::ForEachFont (this=<optimized out>, 
    fc=0x6f9dc21d <gfxFontGroup::FindPlatformFont(nsAString_internal const&, nsACString_internal const&, bool, void*)>, closure=<optimized out>)
    at /Users/snorp/source/mozilla-central/gfx/thebes/gfxFont.cpp:4020
#10 0x6f9dd824 in gfxFontGroup::BuildFontList (this=0x8a5e6480) at /Users/snorp/source/mozilla-central/gfx/thebes/gfxFont.cpp:3846
#11 0x6f9dda92 in gfxFontGroup::gfxFontGroup (this=0x8a5e6480, aFamilies=..., aStyle=0x6a0dcff8, aUserFontSet=0x0)
    at /Users/snorp/source/mozilla-central/gfx/thebes/gfxFont.cpp:3837
#12 0x6f9ca7de in gfxAndroidPlatform::CreateFontGroup (this=<optimized out>, aFamilies=..., aStyle=0x6a0dcff8, aUserFontSet=0x0)
    at /Users/snorp/source/mozilla-central/gfx/thebes/gfxAndroidPlatform.cpp:303
#13 0x6f3731be in mozilla::dom::CanvasRenderingContext2D::SetFont (this=0x8a5e9c00, font=..., error=...)
    at /Users/snorp/source/mozilla-central/content/canvas/src/CanvasRenderingContext2D.cpp:2120
#14 0x6f831fb6 in mozilla::dom::CanvasRenderingContext2DBinding::set_font (cx=0x76d66dc0, obj=..., self=0x8a5e9c00, argv=<optimized out>)
    at /Users/snorp/source/mozilla-central/objdir-android/dom/bindings/CanvasRenderingContext2DBinding.cpp:3503
#15 0x6f8339f2 in mozilla::dom::CanvasRenderingContext2DBinding::genericSetter (cx=0x76d66dc0, argc=<optimized out>, vp=0x72800158)
    at /Users/snorp/source/mozilla-central/objdir-android/dom/bindings/CanvasRenderingContext2DBinding.cpp:4177
#16 0x6fdaea22 in CallJSNative (args=..., native=<optimized out>, cx=<optimized out>)
    at /Users/snorp/source/mozilla-central/js/src/jscntxtinlines.h:349
#17 js::Invoke (cx=0x76d66dc0, args=..., construct=js::NO_CONSTRUCT) at /Users/snorp/source/mozilla-central/js/src/vm/Interpreter.cpp:395
#18 0x6fdaef52 in Invoke (rval=0x6a0dd2d8, argv=0x6a0dd2d8, argc=<optimized out>, fval=..., thisv=..., cx=0x76d66dc0)
    at /Users/snorp/source/mozilla-central/js/src/vm/Interpreter.cpp:441
#19 js::InvokeGetterOrSetter (cx=0x76d66dc0, obj=0x7ec3bd30, fval=..., argc=<optimized out>, argv=0x6a0dd2d8, rval=0x6a0dd2d8)
    at /Users/snorp/source/mozilla-central/js/src/vm/Interpreter.cpp:512
#20 0x6fe55730 in js::Shape::set (this=0x731ee430, cx=0x76d66dc0, obj=..., receiver=..., strict=false, vp=...)
    at /Users/snorp/source/mozilla-central/js/src/vm/Shape-inl.h:326
#21 0x6fe5ff06 in js::baseops::SetPropertyHelper (cx=0x76d66dc0, obj=..., receiver=..., id=..., defineHow=1, vp=..., strict=0)
    at /Users/snorp/source/mozilla-central/js/src/jsobj.cpp:4340
#22 0x6fda2c62 in js::SetPropertyOperation (cx=<optimized out>, pc=<optimized out>, lval=..., rval=...)
    at /Users/snorp/source/mozilla-central/js/src/vm/Interpreter-inl.h:368
#23 0x6fdaab04 in js::Interpret (cx=0x76d66dc0, entryFrame=0x728000b8, interpMode=js::JSINTERP_NORMAL, useNewType=<optimized out>)
    at /Users/snorp/source/mozilla-central/js/src/vm/Interpreter.cpp:2109
#24 0x6fdae88a in js::RunScript (cx=0x76d66dc0, fp=0x728000b8) at /Users/snorp/source/mozilla-central/js/src/vm/Interpreter.cpp:352

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Guard against null cairo font face

This fixes the crash, but I think there is probably a deeper issue here. Why do we have a null font face?

Comment 2

[George Wright (:gw280) (needinfo me!)]

Is this with the graphics branch?

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to George Wright (:gw280) from comment #2)
> Is this with the graphics branch?

Yup.

Comment 4

[George Wright (:gw280) (needinfo me!)]

Probably worth having a null check there, but I'll investigate deeper.

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

George, is that a r+?

Comment 6

[George Wright (:gw280) (needinfo me!)]

Doh. I forgot about this. Yes, r+ this for now, and we should investigate the root cause as well.

Comment 7

[Scoobidiver (away)]

It's #1 top crasher in FxOS 1.0.1 and accounts for 90% of crashes on ZTE roamer2.

Comment 8

[Robert Kaiser]

(In reply to Scoobidiver from comment #7)
> It's #1 top crasher in FxOS 1.0.1 and accounts for 90% of crashes on ZTE
> roamer2.

That actually looks to me like it's a spike over a few hours, but not an ongoing thing.

Comment 9

[Scoobidiver (away)]

(In reply to Robert Kaiser (:[e-mail]) from comment #8)
> That actually looks to me like it's a spike over a few hours, but not an
> ongoing thing.
It's indeed the same install time but how can it be trusted, January 1980 is in the past? 1778 crashes from the same user within 45 minutes is a record.

Comment 10

[Robert Kaiser]

(In reply to Scoobidiver from comment #9)
> (In reply to Robert Kaiser (:[e-mail]) from comment #8)
> > That actually looks to me like it's a spike over a few hours, but not an
> > ongoing thing.
> It's indeed the same install time but how can it be trusted, January 1980 is
> in the past? 1778 crashes from the same user within 45 minutes is a record.

I didn't mean the install time - it's pretty clear that when a few thousand devices are all delivered with identical factory-installed builds that they would all have the same install time, bogus or not.

What I meant is the actual crash time, which as you also saw is within a very short period on a single day.

Comment 11

[Wayne Chang [:wchang]]

Scoobidiver, James,

Can you suggest if this impacts v1.1 (leo)? 

Triage needs to know if this is applicable to v1.1.

Comment 12

[Scoobidiver (away)]

(In reply to Wayne Chang [:wchang] from comment #11)
> Can you suggest if this impacts v1.1 (leo)? 
I think so. Anyway, the only risk of a null check is to crash later.

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

It is really cool that a crash found on the graphics branch using SkiaGL on Android, on the trunk, would match a 1.0.1/1.1 crash on B2G on Gecko 18.

Comment 14

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Wayne Chang [:wchang] from comment #11)
> Scoobidiver, James,
> 
> Can you suggest if this impacts v1.1 (leo)? 
> 
> Triage needs to know if this is applicable to v1.1.

SkiaGL definitely does not impact this, as my understanding is that it uses Gecko 18. The SkiaGL changes only recently went into trunk (Gecko 25).

Comment 15

[Scoobidiver (away)]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #14)
> SkiaGL definitely does not impact this, as my understanding is that it uses
> Gecko 18.
This crash exists in Firefox for Android before SkiaGL. Here is a crash report in Firefox 18.0.2 for Android with the same eleven first frames: bp-3cd9c446-ff55-47b8-902e-680d52130214.
So the patch here helps with or without SkiaGL as the null check is in frame 1.

Comment 16

[bhavana bajaj [:bajaj]]

:snorp can you please help with what are next steps would be here ? Anything obvious given comment #15 ?

Comment 17

[James Willcox (:snorp) ([e-mail]) (he/him)]

This isn't SkiaGL related at all, so I have no idea what could be going on. We did have a similar bug with SkiaGL but one of the font guys fixed some stuff. George do you remember who did that or when the change occurred?

Comment 18

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 762734 [details] [diff] [review]
Guard against null cairo font face

Review of attachment 762734 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFT2FontList.cpp
@@ +179,5 @@
>      }
>  
> +    if (!CairoFontFace()) {
> +        return nullptr;
> +    }

Do we need to destroy font options in the early exit case?

Comment 19

[George Wright (:gw280) (needinfo me!)]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #17)
> This isn't SkiaGL related at all, so I have no idea what could be going on.
> We did have a similar bug with SkiaGL but one of the font guys fixed some
> stuff. George do you remember who did that or when the change occurred?

Yes, Jonathan Kew landed a change in trunk a while back that addressed this particular nullptr issue. See http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFT2FontList.cpp#197

I believe you will want to therefore take the patch from bug 878674 rather than this one.

Comment 20

[Jonathan Kew [:jfkthame]]

Yes, the restructuring of the FT2 font code in bug 878674 did address this, AFAICT; in particular, it included an early null check in FT2FontEntry::CreateScaledFont (before the cairo_font_options has been created, so the issue in comment #18 here doesn't arise).

If you're looking to backport to b2g-XXX, I think it would be pretty safe to take the patch from bug 878674; however, it would be considerably more code churn than just a targeted null-check, and AFAIK the main changes there (to support using APK-bundled fonts without copying them out to the filesystem) are irrelevant on b2g as we (presumably) don't use that font-bundling mechanism anywhere except the Firefox/Android APK.

The alternative, especially if bug 878674 doesn't backport cleanly (I'm not sure if there'll be conflicts with other changes that have landed in the same areas of code) would be to just fix up the small patch here, and ignore the APK font-loading changes for b2g-backporting purposes. I'd suggest putting the CairoFontFace null-check and early return up at the top of FT2FontEntry::CreateScaledFont, as in current trunk, and also copying the null-check that was added to FT2FontEntry::CreateFontInstance (see http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFT2FontList.cpp#262), to avoid the risk that we just move the crash from the current function up to its caller.

Comment 21

[Milan Sreckovic [:milan] (needinfo for best results)]

Sounds good.  George, can you create the patch as Jonathan suggests and we'll uplift it to b2g18?

Comment 22

[Scoobidiver (away)]

It's no longer a top crasher in B2G 18.0 but still good to take.

Comment 23

[Robert Kaiser]

(In reply to Scoobidiver from comment #22)
> It's no longer a top crasher in B2G 18.0 but still good to take.

"B2G 18.0" is no useful classification for B2G crashes, actually. And according to https://crash-analysis.mozilla.com/rkaiser/2013-08-02/2013-08-02.b2g.topcrashes.weekly.html#b2g-1.0.1.0-prerelease this is still #5 on 1.0.1, so let's leave topcrash on it for now.

Comment 24

[Preeti Raghunath(:Preeti)]

Hi Milan,

Do we have a patch for this bug?

Please also provide risk analysis of the pact in question.

Comment 25

[Milan Sreckovic [:milan] (needinfo for best results)]

Not yet, no, some traveling getting in the way.

Comment 26

[Jonathan Kew [:jfkthame]]

Attachment: check for failure when creating cairo font-face and scaled-font objects

I think this is the patch you're looking for.

Comment 27

[Jonathan Kew [:jfkthame]]

Attachment: check for failure when creating cairo font-face and scaled-font objects

On inspecting the code further, there's also a missing null-check for cairo_scaled_font creation in gfxFT2Font::GetOrMakeFont. That wasn't the call stack reported here in comment #0, but we should fix it too.

Comment 28

[Jonathan Kew [:jfkthame]]

Attachment: check for failure when creating the cairo_scaled_font in gfxFT2Font::GetOrMakeFont

And that (latent?) bug is still present on trunk, too, so here's the corresponding patch for m-c. I don't know whether we're currently hitting this at all, but failure to check the result of CreateScaledFont before using it is certainly a crash waiting to happen.

Comment 29

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 787419 [details] [diff] [review]
check for failure when creating cairo font-face and scaled-font objects

Review of attachment 787419 [details] [diff] [review]:
-----------------------------------------------------------------

I like it.  I'll also get one of the graphics peers to review this to make it official.

::: gfx/thebes/gfxFT2FontList.cpp
@@ +95,5 @@
> +    cairo_font_face_t *cairoFace = CairoFontFace();
> +    if (!cairoFace) {
> +        return nullptr;
> +    }
> +

It seems it is not too early to do this right up front.  CairoFontFace() doesn't seem to depend on the font options set further down, so if we fail to make it here, we'll fail to make it in time to call cairo_scaled_font_create() below.

@@ +129,5 @@
>      if (gfxPlatform::GetPlatform()->RequiresLinearZoom()) {
>          cairo_font_options_set_hint_metrics(fontOptions, CAIRO_HINT_METRICS_OFF);
>      }
>  
> +    scaledFont = cairo_scaled_font_create(cairoFace,

Stylistically, I like the temporary cairoFace and reusing it, but CairoFontFace() does cache the result, so if it returns non-null, the second call would be cheap (just returns mFontFace).  So, I can see the argument for just putting:

if (!CairoFontFace()) {
    return nullptr;
}

at the start of the function, and then leaving this as it is.

Comment 30

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 787422 [details] [diff] [review]
check for failure when creating the cairo_scaled_font in gfxFT2Font::GetOrMakeFont

Review of attachment 787422 [details] [diff] [review]:
-----------------------------------------------------------------

Nice catch.

Comment 31

[John Daggett (:jtd)]

Comment on attachment 787422 [details] [diff] [review]
check for failure when creating the cairo_scaled_font in gfxFT2Font::GetOrMakeFont

Do we have a repeatable testcase for this?  It would be nice to have a crashtest for this, if one can be found.

Comment 32

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #31)
> Comment on attachment 787422 [details] [diff] [review]
> check for failure when creating the cairo_scaled_font in
> gfxFT2Font::GetOrMakeFont
> 
> Do we have a repeatable testcase for this?  It would be nice to have a
> crashtest for this, if one can be found.

Not AFAIK; this was found by inspection, while reviewing the callers of FT2FontEntry::CreateScaledFont().

Comment 33

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/90943a435604

Comment 34

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/45480132106b

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/45480132106b

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/90943a435604