Bug 194251

Created attachment 361122 [details] WIP

Attachment 361122 [details] did not pass style-queue: ERROR: Source/WebCore/css/CSSFontSelector.cpp:126: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebCore/css/CSSFontSelector.cpp:148: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 3 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 361122 [details] WIP Attachment 361122 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/11034034 Number of test failures exceeded the failure limit.

Created attachment 361150 [details] Archive of layout-test-results from ews202 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews202 Port: win-future Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit

CachedResource::load: Failed security check -- state is provisional #0 0x0000000108b31c6d in WebCore::CSSFontFaceSource::fontLoaded(WebCore::CachedFont&) at /Users/litherum/src/WebKit/Source/WebCore/css/CSSFontFaceSource.cpp:140 #1 0x0000000109830f80 in WebCore::CachedFont::checkNotify() at /Users/litherum/src/WebKit/Source/WebCore/loader/cache/CachedFont.cpp:159 #2 0x00000001098429e1 in WebCore::CachedResource::error(WebCore::CachedResource::Status) at /Users/litherum/src/WebKit/Source/WebCore/loader/cache/CachedResource.cpp:379 #3 0x0000000109846212 in WebCore::CachedResource::failBeforeStarting() at /Users/litherum/src/WebKit/Source/WebCore/loader/cache/CachedResource.cpp:185 #4 0x000000010983d346 in WebCore::CachedResource::load(WebCore::CachedResourceLoader&) at /Users/litherum/src/WebKit/Source/WebCore/loader/cache/CachedResource.cpp:230 #5 0x000000010983078c in WebCore::CachedFont::beginLoadIfNeeded(WebCore::CachedResourceLoader&) at /Users/litherum/src/WebKit/Source/WebCore/loader/cache/CachedFont.cpp:82 #6 0x0000000108b336bf in WebCore::CSSFontSelector::beginLoadTimerFired() at /Users/litherum/src/WebKit/Source/WebCore/css/CSSFontSelector.cpp:381

That calls setStatus(Status::Failure) which means we never try to download the font again.

... Which means both the CSSFontFaceSource and the CSSFontFace call setStatus(Status::Failure).

When we go "back," we rebuild new CSSFontFace and CSSFontFaceSource objects. In CSSFontFaceSource's constructor, it adds itself as a client to the CachedFont. This synchronously calls CSSFontFaceSource::fontLoaded(), which checks m_font->errorOccurred(), which is true, so it setStatus(Status::Failure) again.

CSSFontFaceSrcValue::cachedFont() saves the CachedFont object, so the state is preserved (because the CSSFontFaceSrcValue is preserved)

I think the way to fix this is: 1) CachedFont overrides error(CachedResource::Status), and in the implementation, checks isLoading() to determine whether or not it failed before loading. 2) Even if the CachedResource failed, CachedFont remembers whether or not that failure was a failBeforeStarting or a real failure 3) CSSFontFaceSource treats a failBeforeStarted CachedFont as a yet-to-be-loaded CachedFont 4) CachedFont::beginLoadIfNeeded() allows for another load if the first one was a failBeforeStarted

Unfortunately, CachedFont::load() calls setLoading(true)

Created attachment 366460 [details] WIP

Created attachment 366461 [details] Patch

Attachment 366461 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 366536 [details] Patch

Created attachment 366537 [details] Patch

<rdar://problem/44808773>

This codepath probably kicks in too often - A content blocker probably causes a synchronous fail, and those shouldn't cause the font to be retried. Constantly retrying the font, means we never progress down the font-family list, which isn't an improvement from this bug.

Still relevant?

Comment on attachment 366537 [details] Patch Clearing old review flag. Please rebase and mark for review if necessary.