Bug 65123

Summary: Font loading during layout can cause layout code to be re-entered via resource load delegate
Product: WebKit Reporter: mitz
Component: Layout and RenderingAssignee: mitz
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Defer font loading using a zero-delay timer so that it does not happen during layout
none
Patch darin: review+

Description mitz 2011-07-25 11:09:02 PDT
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;
}
Comment 1 Radar WebKit Bug Importer 2011-07-25 11:09:33 PDT
<rdar://problem/9835028>
Comment 2 mitz 2011-07-25 11:33:42 PDT
Created attachment 101886 [details]
Defer font loading using a zero-delay timer so that it does not happen during layout
Comment 3 Anders Carlsson 2011-07-25 12:04:56 PDT
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.
Comment 4 mitz 2011-07-25 13:01:32 PDT
Fixed in <http://trac.webkit.org/r91699>.
Comment 5 mitz 2011-07-25 15:26:26 PDT
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.
Comment 6 mitz 2011-07-25 17:45:34 PDT
Created attachment 101952 [details]
Patch
Comment 7 Darin Adler 2011-07-25 18:29:00 PDT
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.
Comment 8 mitz 2011-07-25 20:22:38 PDT
Committed <http://trac.webkit.org/r91738>.
Comment 9 mitz 2011-07-25 20:37:52 PDT
Build fix in <http://trac.webkit.org/r91739>.
Comment 10 Adam Barth 2011-08-04 14:01:42 PDT
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
Comment 11 Nikolas Zimmermann 2011-11-11 01:53:51 PST
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?
Comment 12 mitz 2011-11-11 08:36:02 PST
Sure would be nice to have a way to do this without a timer.