456899 - ASSERTION: forgot to short-circuit a text run with zero-sized font?: 'GetStyle()->size != 0', file gfxFT2FontBase.cpp, line 178

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Mats Palmgren (inactive)]

STEPS TO REPRODUCE
1. start Firefox and load the attached testcase
  (only occurs once, need to restart Firefox to trigger it again)

ACTUAL RESULT
ASSERTION: forgot to short-circuit a text run with zero-sized font?: 'GetStyle()->size != 0', file gfxPangoFonts.h, line 94

Comment 1

[Mats Palmgren (inactive)]

Attachment: Testcase

Comment 2

[Mats Palmgren (inactive)]

Attachment: stack

It's the GetSpaceGlyph() call here that causes it:
http://hg.mozilla.org/mozilla-central/annotate/4c3dccca8505/gfx/thebes/src/gfxFont.cpp#l615

The reason we're in GetOrCreateGlyphExtents() is because
NeedsGlyphExtents() is true (TEXT_NEED_BOUNDING_BOX):
http://hg.mozilla.org/mozilla-central/annotate/4c3dccca8505/gfx/thebes/src/gfxFont.cpp#l519

Comment 3

[Mats Palmgren (inactive)]

Attachment: wip

Possible fix.
Alternatively, we could move the assertion to some other place.

We should fix gfxTextRun::FetchGlyphExtents() anyway since it
could crash if 'extents' is null for some reason such as OOM.

Comment 4

[Mats Palmgren (inactive)]

Attachment: wip

Comment 5

[Jesse Ruderman]

Attachment: testcase 2

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Happens on tinderbox reliably with testcase:
layout/reftests/bugs/355548-3.xml

Comment 7

[Bas Schouten (:bas.schouten)]

Karl, is this bug still relevant?

Comment 8

[Karl Tomlinson (:karlt)]

Assertion still fires with the first testcase.
The approach in the patch looks sensible (from quick inspection).

Comment 9

[Geoff Brown [:gbrown]]

Attachment: un-bitrotted "wip" patch

:mats -- I am trying to resolve bug 1019178, a reftest that hits this assertion on Android. Un-bitrotting your wip patch resolves that issue, and also allows us to remove an expected assertion notation from a mochitest. 

Try run looks promising:

https://tbpl.mozilla.org/?tree=Try&rev=9f12ffd3922e

(I had not updated the mochitest yet in that push.)

Is there more to do here, or can your patch be reviewed and checked in?

Comment 10

[Mats Palmgren (inactive)]

Comment on attachment 8443498 [details] [diff] [review]
un-bitrotted "wip" patch

I don't remember why I didn't submit it for review.
I suspect I wanted to investigate some detail first...

Anyway, looking at the code again I think this should work;
although in FetchGlyphExtents it would probably be better
to just do the same "GetStyle()->size == 0" early return
instead of looping without doing anything (AFAICT).
(and please wrap the condition with MOZ_UNLIKELY in
both places)

(I think I might have added the 'extents' null-check there
partly b/c GetOrCreateGlyphExtents could OOM back then.
I think that allocation is now infallible.)

I suggest you ask Jonathan Kew <[e-mail]>
for review.

Comment 11

[Geoff Brown [:gbrown]]

(In reply to Mats Palmgren (:mats) from comment #10)

Thanks Mats.

> although in FetchGlyphExtents it would probably be better
> to just do the same "GetStyle()->size == 0" early return
> instead of looping without doing anything (AFAICT).

GetStyle() applies to a gfxFont instance, and we are looping through potentially different fonts in FetchGlyphExtents, so I think I cannot do an early return, and your original extents null-check is still the best approach.

Comment 12

[Mats Palmgren (inactive)]

Ah, OK, it makes sense as is then.  Just add MOZ_UNLIKELY in both places.

Comment 13

[Geoff Brown [:gbrown]]

Attachment: :mats patch resurrected + update test assertion expectations

This is very nearly the same as :mats original patch from years past, updated for bitrot (see Comments 9 - 12) and some minor changes to assertion expectations for a couple of tests.

New try run at https://tbpl.mozilla.org/?tree=Try&rev=fdf6a9484403.

Comment 14

[Jonathan Kew [:jfkthame]]

Comment on attachment 8443765 [details] [diff] [review]
:mats patch resurrected + update test assertion expectations

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

I don't much like changing GetOrCreateGlyphExtents such that it may return null; this seems unexpected (now that the memory allocation it may need to do is infallible), and hence perhaps a footgun for the future.

How about just adding

    if (MOZ_UNLIKELY(font->GetStyle()->size == 0)) {
        continue;
    }

early in the FetchGlyphExtents() loop, to immediately short-circuit there?

Comment 15

[Geoff Brown [:gbrown]]

Attachment: :mats patch resurrected + update test assertion expectations

Updated as suggested. 

There is another caller of GetOrCreateGlyphExtents, so I had to add a guard there too (otherwise, some tests still hit the assertion).

Comment 16

[Geoff Brown [:gbrown]]

(In reply to Jonathan Kew (:jfkthame) from comment #14)
> null; this seems unexpected (now that the memory allocation it may need to
> do is infallible)

Out of curiosity, how is GetOrCreateGlyphExtents infallible? Can it not OOM at http://hg.mozilla.org/mozilla-central/annotate/366b5c0c02d3/gfx/thebes/gfxFont.cpp#l4337 ?

Comment 17

[Jonathan Kew [:jfkthame]]

It can OOM, yes. "Infallible" here meaning that if it can't allocate the memory it needs, it'll abort. So callers shouldn't have to worry about it returning a bogus result (such as an unexpectedly null pointer) - if it returns at all, the object it returns is safe to use.

Comment 18

[Jonathan Kew [:jfkthame]]

Comment on attachment 8444014 [details] [diff] [review]
:mats patch resurrected + update test assertion expectations

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

Hmmm. On reflection, I sort of think that assertion should move -- into GetOrCreateGlyphExtents, maybe -- so that it's applicable on all platforms. But that could be a separate followup (if indeed it makes sense). For now, this should be fine.

Comment 19

[Geoff Brown [:gbrown]]

https://tbpl.mozilla.org/?tree=Try&rev=90de02bf41d5

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f8c69149b7e

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9f8c69149b7e