Bug 60917 – [GTK][Cairo] Twitter rendering breaks

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Martin Robinson]

Created attachment 93685 [details]
Image of twitter issue

Steps to reproduce:

1. View your Twitter stream.
2. Click on the name of one of the twitterers.
3. Rendering should break dramatically.

I've attached a screenshot of this. After an extensive amount of bisecting, I've narrowed the issue down to fonts with a 0 pixel font size. It seems that rendering text with a zero pixel font size seems to corrupt the Cairo context in some way. Unfortunately, I have not had success isolating this bug with a test case.

[Martin Robinson]

Created attachment 93687 [details]
Patch

[Benjamin Otte]

Do you check cairo_status() anywhere in the code? It might be that cairo silently sets an error status somewhere... And if it does that, it stops rendering anything.

Fwiw, you can break on _cairo_error in gdb to see where exactly the error happens.

[Dirk Schulze]

Comment on attachment 93687 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=93687&action=review

If you can't reproduce it with normal font-size:0, how can you be sure that there isn't another problem?

> Source/WebCore/platform/graphics/cairo/FontCairo.cpp:103
> +    // 0-pixel sized fonts seem to corrupt the Cairo context. Since rendering them should
> +    // effectively be a no-op anyway, it's safer to just bail out early.

Is this comment really necessary? If you think so:  Early return for fonts size of 0. Isn't this enough? It's up to you.

[Martin Robinson]

Created attachment 94010 [details]
Patch

[Martin Robinson]

(In reply to comment #4)
> Created an attachment (id=94010) [details]
> Patch

After some experimentation I detected that the scaled font itself calls _cairo_error so it makes more sense to simply avoid making these failed fonts. I've updated the patch and I think this new method is more correct.

[Dirk Schulze]

Comment on attachment 94010 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=94010&action=review

> Source/WebCore/ChangeLog:17
> +        No new tests. I have not had luck reproducing the render corruption outside Twitter, but
> +        the fix can be observed by breaking on _cairo_error.

Does the font matter? Can you reproduce it on Twitter if you use other fonts (by userscripts or what ever)? I'm really not happy about the missing test. Did you try to save the current state locally? Did it crash locally? If so, have you tried to reduce the content of the page to a minimum? Would be great if we at least have a pixel test, that just works with your fix.

> Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:98
> +                                                        m_platformData.syntheticOblique()),
> +                                       isCustomFont(), false));

Alignment problem.

[Martin Robinson]

Created attachment 94891 [details]
Patch

[Martin Robinson]

(In reply to comment #6)
> (From update of attachment 94010 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94010&action=review
> 
> > Source/WebCore/ChangeLog:17
> > +        No new tests. I have not had luck reproducing the render corruption outside Twitter, but
> > +        the fix can be observed by breaking on _cairo_error.
> 
> Does the font matter? Can you reproduce it on Twitter if you use other fonts (by userscripts or what ever)? I'm really not happy about the missing test. Did you try to save the current state locally? Did it crash locally? If so, have you tried to reduce the content of the page to a minimum? Would be great if we at least have a pixel test, that just works with your fix.

You are absolutely right. I worked a bit harder today trying to isolate the issue and I was able to do it after carving away the rest of the Twitter interface. I've included the test in my latest patch.

> 
> > Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:98
> > +                                                        m_platformData.syntheticOblique()),
> > +                                       isCustomFont(), false));
> 
> Alignment problem.

The alignment is odd here because the last two are actually associated with the outer constructor, while the previous lines are associated with an an inner constructor. Let me know if you would prefer some other alignment.

[Dirk Schulze]

Comment on attachment 94891 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=94891&action=review

LGTM. r=me

> Source/WebCore/ChangeLog:8
> +        When instantiating a cairo_scaled_font_t font would pus the font in an error state,

s/pus/push/ ?

> Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:98
> +    return adoptPtr(new SimpleFontData(FontPlatformData(m_platformData.m_font.get(),
> +                                                        scaleFactor * fontDescription.computedSize(),
> +                                                        m_platformData.syntheticBold(),
> +                                                        m_platformData.syntheticOblique()),
> +                                       isCustomFont(), false));

ah ok, the alignment is correct.

[Martin Robinson]

Committed r87523: <http://trac.webkit.org/changeset/87523>