Bug 173138 – @font-face rules with invalid primary fonts never download their secondary fonts

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

@font-face rules with invalid primary fonts never download their secondary fonts

[Myles C. Maxfield]

Created attachment 312401 [details]
WIP

[Myles C. Maxfield]

<rdar://problem/32554450>

[Myles C. Maxfield]

Created attachment 312477 [details]
Broken test

[Myles C. Maxfield]

Created attachment 312478 [details]
Broken test

[Myles C. Maxfield]

Created attachment 312480 [details]
Broken test

[Myles C. Maxfield]

Created attachment 312482 [details]
Broken test

[Myles C. Maxfield]

Created attachment 312589 [details]
Patch

[Myles C. Maxfield]

Created attachment 312611 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 312611 [details]
Patch

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

> Source/WebCore/css/CSSFontFaceSource.cpp:199
> +        ASSERT(status() == Status::Success);

Is this assertion useful?

> Source/WebCore/css/CSSFontFaceSource.cpp:201
> +        ASSERT(result);

Is this assertion useful?

[Myles C. Maxfield]

Comment on attachment 312611 [details]
Patch

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

>> Source/WebCore/css/CSSFontFaceSource.cpp:199
>> +        ASSERT(status() == Status::Success);
> 
> Is this assertion useful?

Yes, this makes sure that the only way we could have gotten here is if we already checked that we won't return null. This check occurs when we transition into the Success state.

>> Source/WebCore/css/CSSFontFaceSource.cpp:201
>> +        ASSERT(result);
> 
> Is this assertion useful?

Yes, this function returning null is the cause of the bug.

[WebKit Commit Bot]

Comment on attachment 312611 [details]
Patch

Clearing flags on attachment: 312611

Committed r218157: <http://trac.webkit.org/changeset/218157>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Matt Lewis]

It looks like this patch https://trac.webkit.org/changeset/218157/webkit
may have caused several API Timeouts on iOS Simulator builds dealing with
WebKit2.LineBreaking

https://build.webkit.org/builders/Apple%20iOS%2010%20Simulator%20Release%20WK2%20%28Tests%29/builds/2203
https://build.webkit.org/builders/Apple%20iOS%2010%20Simulator%20Release%20WK2%20%28Tests%29/builds/2203/steps/run-api-tests/logs/stdio

[Ryan Haddad]

(In reply to Matt Lewis from comment #13)
> It looks like this patch https://trac.webkit.org/changeset/218157/webkit
> may have caused several API Timeouts on iOS Simulator builds dealing with
> WebKit2.LineBreaking
> 
> https://build.webkit.org/builders/
> Apple%20iOS%2010%20Simulator%20Release%20WK2%20%28Tests%29/builds/2203
> https://build.webkit.org/builders/
> Apple%20iOS%2010%20Simulator%20Release%20WK2%20%28Tests%29/builds/2203/steps/
> run-api-tests/logs/stdio

It looks like the API tests are hitting assertion failures on iOS simulator:
https://build.webkit.org/builders/Apple%20iOS%2010%20Simulator%20Debug%20WK2%20%28Tests%29/builds/2059/steps/run-api-tests/logs/stdio

ASSERTION FAILED: !m_deletionHasBegun
/Volumes/Data/slave/ios-simulator-10-debug/build/WebKitBuild/Debug-iphonesimulator/usr/local/include/wtf/RefCounted.h(43) : void WTF::RefCountedBase::ref() const
1   0x1103de64d WTFCrash
2   0x1103de669 WTFCrashWithSecurityImplication
3   0x1150ca872 WTF::RefCountedBase::ref() const
4   0x1155ca91a WTF::Ref<WebCore::CSSFontFace>::Ref(WebCore::CSSFontFace&)
5   0x1155c2d1d WTF::Ref<WebCore::CSSFontFace>::Ref(WebCore::CSSFontFace&)
6   0x1155c2c34 WebCore::CSSFontFace::fontLoadEventOccurred()
7   0x1155c41a0 WebCore::CSSFontFace::fontLoaded(WebCore::CSSFontFaceSource&)
8   0x1155eda07 WebCore::CSSFontFaceSource::fontLoaded(WebCore::CachedFont&)
9   0x11531738e WebCore::CachedFont::checkNotify()
10  0x1153167d6 WebCore::CachedFont::finishLoading(WebCore::SharedBuffer*)
11  0x11783a89a WebCore::SubresourceLoader::didFinishLoading(WebCore::NetworkLoadMetrics const&)
12  0x10799987d WebKit::WebResourceLoader::didFinishResourceLoad(WebCore::NetworkLoadMetrics const&)
13  0x10799e1f6 void IPC::callMemberFunctionImpl<WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&), std::__1::tuple<WebCore::NetworkLoadMetrics>, 0ul>(WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&), std::__1::tuple<WebCore::NetworkLoadMetrics>&&, std::__1::integer_sequence<unsigned long, 0ul>)
14  0x10799e068 void IPC::callMemberFunction<WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&), std::__1::tuple<WebCore::NetworkLoadMetrics>, std::__1::integer_sequence<unsigned long, 0ul> >(std::__1::tuple<WebCore::NetworkLoadMetrics>&&, WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&))
15  0x10799d342 void IPC::handleMessage<Messages::WebResourceLoader::DidFinishResourceLoad, WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&)>(IPC::Decoder&, WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&))
16  0x10799ca6c WebKit::WebResourceLoader::didReceiveWebResourceLoaderMessage(IPC::Connection&, IPC::Decoder&)
17  0x1072933f9 WebKit::NetworkProcessConnection::didReceiveMessage(IPC::Connection&, IPC::Decoder&)
18  0x1070478e3 IPC::Connection::dispatchMessage(IPC::Decoder&)
19  0x10703d568 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)
20  0x107047ee0 IPC::Connection::dispatchOneMessage()
21  0x1070600dd IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_14::operator()()
22  0x107060039 WTF::Function<void ()>::CallableWrapper<IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_14>::call()
23  0x11040d5ee WTF::Function<void ()>::operator()() const
24  0x11042d13d WTF::RunLoop::performWork()
25  0x11042d824 WTF::RunLoop::performWork(void*)
26  0x10a7aec01 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__
27  0x10a7940cf __CFRunLoopDoSources0
28  0x10a7935ff __CFRunLoopRun
29  0x10a793016 CFRunLoopRunSpecific
30  0x106a6e480 -[NSRunLoop(NSRunLoop) runMode:beforeDate:]
31  0x106a6e35b -[NSRunLoop(NSRunLoop) run]

[Matt Lewis]

Reverted r218157 for reason:

This patch caused multiple API failures on iOS Simulator.

Committed r218264: <http://trac.webkit.org/changeset/218264>

[Myles C. Maxfield]

I can't reproduce the failures on a locally-running iOS 11 simulator. I'll try iOS 10.

[Myles C. Maxfield]

I can't reproduce this on an iOS 10 simulator either.

[Myles C. Maxfield]

The assertion is saying that the CSSFontFace's superclass has had its destructor called, but the CSSFontFace owns a unique_ptr to the CSSFontFaceSource whose destructor unregisters it from the CachedResource.

[Myles C. Maxfield]

I tried to reproduce this for 3 hours, trying everything I can think of (including running the exact same sequence of commands that the bots are running), and I still can't reproduce this.

[Myles C. Maxfield]

CSSFontFaceSource::fontLoaded() is synchronously causing the CSSFontFace to be destroyed before it calls m_face.fontLoaded(*this);

[Myles C. Maxfield]

The CSSFontFace is being destroyed under SVGDocument::create()

[Myles C. Maxfield]

Committed r218733: <http://trac.webkit.org/changeset/218733>