Bug 60917 - [GTK][Cairo] Twitter rendering breaks
Summary: [GTK][Cairo] Twitter rendering breaks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P3 Normal
Assignee: Martin Robinson
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2011-05-16 13:39 PDT by Martin Robinson
Modified: 2011-05-27 10:40 PDT (History)
3 users (show)

See Also:


Attachments
Image of twitter issue (77.93 KB, image/png)
2011-05-16 13:39 PDT, Martin Robinson
no flags Details
Patch (2.05 KB, patch)
2011-05-16 13:43 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (11.10 KB, patch)
2011-05-18 17:35 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (28.48 KB, patch)
2011-05-25 17:45 PDT, Martin Robinson
krit: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Robinson 2011-05-16 13:39:14 PDT
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.
Comment 1 Martin Robinson 2011-05-16 13:43:27 PDT
Created attachment 93687 [details]
Patch
Comment 2 Benjamin Otte 2011-05-16 14:49:09 PDT
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.
Comment 3 Dirk Schulze 2011-05-16 14:56:03 PDT
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.
Comment 4 Martin Robinson 2011-05-18 17:35:40 PDT
Created attachment 94010 [details]
Patch
Comment 5 Martin Robinson 2011-05-18 17:37:46 PDT
(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.
Comment 6 Dirk Schulze 2011-05-18 23:29:46 PDT
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.
Comment 7 Martin Robinson 2011-05-25 17:45:05 PDT
Created attachment 94891 [details]
Patch
Comment 8 Martin Robinson 2011-05-25 17:46:53 PDT
(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.
Comment 9 Dirk Schulze 2011-05-25 22:06:19 PDT
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.
Comment 10 Martin Robinson 2011-05-27 10:40:07 PDT
Committed r87523: <http://trac.webkit.org/changeset/87523>