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
: WebKit
Layout and Rendering
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
: InRadar
:
:
  Show dependency treegraph
 
Reported: 2011-07-25 11:09 PST by
Modified: 2011-11-11 08:36 PST (History)


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 PST, mitz@webkit.org
no flags Review Patch | Details | Formatted Diff | Diff
Patch (17.33 KB, patch)
2011-07-25 17:45 PST, mitz@webkit.org
darin: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-07-25 11:09:02 PST
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 From 2011-07-25 11:09:33 PST -------
<rdar://problem/9835028>
------- Comment #2 From 2011-07-25 11:33:42 PST -------
Created an attachment (id=101886) [details]
Defer font loading using a zero-delay timer so that it does not happen during layout
------- Comment #3 From 2011-07-25 12:04:56 PST -------
(From update of attachment 101886 [details])
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 From 2011-07-25 13:01:32 PST -------
Fixed in <http://trac.webkit.org/r91699>.
------- Comment #5 From 2011-07-25 15:26:26 PST -------
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 From 2011-07-25 17:45:34 PST -------
Created an attachment (id=101952) [details]
Patch
------- Comment #7 From 2011-07-25 18:29:00 PST -------
(From update of attachment 101952 [details])
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 From 2011-07-25 20:22:38 PST -------
Committed <http://trac.webkit.org/r91738>.
------- Comment #9 From 2011-07-25 20:37:52 PST -------
Build fix in <http://trac.webkit.org/r91739>.
------- Comment #10 From 2011-08-04 14:01:42 PST -------
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 From 2011-11-11 01:53:51 PST -------
(From update of attachment 101952 [details])
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 From 2011-11-11 08:36:02 PST -------
Sure would be nice to have a way to do this without a timer.