Bug 65123 – Font loading during layout can cause layout code to be re-entered via resource load delegate

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

Font loading during layout can cause layout code to be re-entered via resource load delegate

Here is an example:

#import <Foundation/Foundation.h>
#import <WebKit/WebKit.h>

@interface ResourceLoadDelegate : NSObject {
}
@end

@implementation ResourceLoadDelegate

- (NSURLRequest *)webView:(WebView *)sender resource:(id)identifier willSendRequest:(NSURLRequest *)request redirectResponse:(NSURLResponse *)redirectResponse fromDataSource:(WebDataSource *)dataSource
{
    if ([[[request URL] path] hasSuffix:@".ttf"]) {
        DOMElement *documentElement = [[[sender mainFrame] DOMDocument] documentElement];
        [[documentElement style] setProperty:@"display" value:@"none" priority:@""];
        [documentElement offsetTop];
    }
    return request;
}

@end

int main (int argc, const char * argv[])
{
    @autoreleasepool {        
        WebView *webView = [[WebView alloc] initWithFrame:NSZeroRect frameName:nil groupName:[[NSBundle mainBundle] bundleIdentifier]];
        ResourceLoadDelegate *delegate = [[ResourceLoadDelegate alloc] init];
        webView.resourceLoadDelegate = delegate;
        [[webView mainFrame] loadRequest:[NSURLRequest requestWithURL:[NSURL URLWithString:@"http://www.alistapart.com/d/cssatten/heid.html"]]];

        [[NSRunLoop mainRunLoop] run];
    }
    return 0;
}

[Radar WebKit Bug Importer]

<rdar://problem/9835028>

Created attachment 101886 [details]
Defer font loading using a zero-delay timer so that it does not happen during layout

[Anders Carlsson]

Comment on attachment 101886 [details]
Defer font loading using a zero-delay timer so that it does not happen during layout

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

> Source/WebCore/css/CSSFontFaceSource.cpp:205
> +    m_fontSelector.clear();

please use

m_fontSelector = nullptr;

here.

Fixed in <http://trac.webkit.org/r91699>.

I reverted r91699 in <http://trac.webkit.org/r91710> because some tests had relied on the synchronous behavior in the cached, error, and local cases.

Created attachment 101952 [details]
Patch

[Darin Adler]

Comment on attachment 101952 [details]
Patch

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

> Source/WebCore/css/CSSFontFaceSource.cpp:178
> +        // the middle of layout, and the loader may invoke aribtrary delegate or event handler code.

typo: aribtrary

> Source/WebCore/css/CSSFontFaceSource.cpp:181
> +        if (!m_startLoadingTimer.isActive())
> +            m_startLoadingTimer.startOneShot(0);

Sometimes it seems to me that these zero-length timers aren’t really timers.

> Source/WebCore/css/CSSFontFaceSource.cpp:203
> +    if (CachedResourceLoader* cachedResourceLoader = m_fontSelector->cachedResourceLoader())
> +        m_font->beginLoadIfNeeded(cachedResourceLoader);

Could just call it loader.

> Source/WebCore/css/CSSFontFaceSource.h:81
> +    Timer<CSSFontFaceSource> m_startLoadingTimer;

That’s a verb phrase, not a noun phrase. Maybe m_loadStartTimer?

> LayoutTests/ChangeLog:10
> +        Unfortunately, font loading does not fire any DOM events, so in most cases
> +        a constant timeout had to be introduced.

Wah! Well at least the timeout is only for test failures.

Committed <http://trac.webkit.org/r91738>.

Build fix in <http://trac.webkit.org/r91739>.

[Adam Barth]

This patch appears to have made these tests flaky:

svg/custom/svg-fonts-with-no-element-reference.html
svg/custom/svg-fonts-without-missing-glyph.xhtml

[Nikolas Zimmermann]

Comment on attachment 101952 [details]
Patch

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

> LayoutTests/fast/css/custom-font-xheight.html:59
> +    }, 100);
>  }

I just noticed Darins comment "Wah! Well at least the timeout is only for test failures.".
I think this observation is wrong, the timeout affects these tests in general.

According to chromiums flakiness dashboard
Recent examples of failure from their dashboard:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=svg%2Fcustom%2Fsvg-fonts-fallback.xhtml%2Csvg%2Fcustom%2Fsvg-fonts-segmented.xhtml%2Csvg%2Ftext%2Ftext-overflow-ellipsis-svgfont.html&showExpectations=true

I've spotted this while working with bashi on his testcase in bug 71765.

To fix this fully we need to be able to listen for events indicating the custom font has finished loading.
I think we could do this through window.internals, what do you think?

Sure would be nice to have a way to do this without a timer.