686190 - Crash [@ gfxFont::Draw]

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Might be the same as bug 685548, but this testcase crashes on a Mac, which should make it easier to debug.

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Jesse Ruderman]

bp-49ec9a55-6b17-49d0-a2d8-bc7242110910

Comment 3

[Jonathan Kew [:jfkthame]]

Thanks for the testcase. I understand why this is crashing, and will try to work up a patch shortly. It's a regression from bug 674909.

Bug 685548 is very likely the same, but I haven't specifically confirmed this yet.

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: patch, avoid risk of overrunning glyph buffer

There was an assumption in GlyphBuffer::Flush that no more than two glyphs would be appended between Flush() calls. With the enhanced synthetic-bold implementation, this broke because synthetic bold on large font sizes does "multi-strike" with additional copies of the glyph.

So the testcase crashes because (on a default OS X configuration) the Korean character resolves to a font that doesn't have a bold face, so synthetic bold gets used; and at such a huge size (due to font-size-adjust), the number of multistrike glyphs causes a buffer overflow, corrupting mNumGlyphs and whatever follows on the stack.

A simple solution is to ensure that Flush() is called for every added glyph, by calling it within the synthetic bold multi-strike loop.

(We should also add the testcase as a crashtest - actually, by using an @font-face rule we can ensure that synthetic bold will definitely be used, independent of the OS font collection and prefs. I'll adapt it accordingly.)

Comment 5

[John Daggett (:jtd)]

Why do the Flush within the do..while loops in the if (synthetic) blocks?  Seems like you could just a well do that at the end of the loop, no?

Comment 6

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #5)
> Why do the Flush within the do..while loops in the if (synthetic) blocks? 
> Seems like you could just a well do that at the end of the loop, no?

But the GlyphBuffer might reach the "full" mark on one of the intermediate iterations, in the case where we're doing a number of strikes.

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: crashtest

Simplified testcase that triggers this crash on OS X (and presumably Android, and any other platform that uses our multi-strike synthetic bolding rather than relying on platform API support for it). This uses @font-face to avoid being dependent on the local system's font configuration.

Comment 9

[Jonathan Kew [:jfkthame]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/91906f410681

Comment 11

[Jonathan Kew [:jfkthame]]

Also pushed crashtest to -inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/d3af41c6bf0b

Comment 12

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/mozilla-central/rev/91906f410681 (patch)
https://hg.mozilla.org/mozilla-central/rev/d3af41c6bf0b (crashtest)

Comment 13

[Martijn Wargers (dead)]

There was a crashtest checked in for this bug, so no need for in-litmus.

Comment 14

[Jeff Muizelaar [:jrmuizel]]

Clang warns about this expression:
        if (!aFinish && mNumGlyphs < GLYPH_BUFFER_SIZE || !mNumGlyphs) {
where it's not clear if it should be:
        if ((!aFinish && mNumGlyphs < GLYPH_BUFFER_SIZE) || !mNumGlyphs) {
or:
        if (!aFinish && (mNumGlyphs < GLYPH_BUFFER_SIZE || !mNumGlyphs)) {

can you add the parenthesis to make it clear which one you mean.