Bug 65123 - Font loading during layout can cause layout code to be re-entered via resource load delegate
: Font loading during layout can cause layout code to be re-entered via resourc...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: mitz@webkit.org
: InRadar
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-25 11:09 PDT by mitz@webkit.org
Modified: 2011-11-11 08:36 PST (History)
3 users (show)

See Also:


Attachments
Defer font loading using a zero-delay timer so that it does not happen during layout (4.26 KB, patch)
2011-07-25 11:33 PDT, mitz@webkit.org
no flags Details | Formatted Diff | Diff
Patch (17.33 KB, patch)
2011-07-25 17:45 PDT, mitz@webkit.org
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz@webkit.org 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@webkit.org 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@webkit.org 2011-07-25 13:01:32 PDT
Fixed in <http://trac.webkit.org/r91699>.
Comment 5 mitz@webkit.org 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@webkit.org 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@webkit.org 2011-07-25 20:22:38 PDT
Committed <http://trac.webkit.org/r91738>.
Comment 9 mitz@webkit.org 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@webkit.org 2011-11-11 08:36:02 PST
Sure would be nice to have a way to do this without a timer.