Bug 173138

Summary:

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

Product:

WebKit

Reporter:

Myles C. Maxfield <mmaxfield>

Component:

New Bugs

Assignee:

Myles C. Maxfield <mmaxfield>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, dino, jlewis3, jonlee, ryanhaddad, simon.fraser, thorton, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
WIP	none
Broken test	none
Broken test	none
Broken test	none
Broken test	none
Patch	none
Patch	none

Description Myles C. Maxfield 2017-06-09 00:26:31 PDT

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

Comment 1 Myles C. Maxfield 2017-06-09 00:28:05 PDT

Created attachment 312401 [details]
WIP

Comment 2 Myles C. Maxfield 2017-06-09 00:28:29 PDT

<rdar://problem/32554450>

Comment 3 Myles C. Maxfield 2017-06-09 13:15:46 PDT

Created attachment 312477 [details]
Broken test

Comment 4 Myles C. Maxfield 2017-06-09 13:21:15 PDT

Created attachment 312478 [details]
Broken test

Comment 5 Myles C. Maxfield 2017-06-09 13:22:24 PDT

Created attachment 312480 [details]
Broken test

Comment 6 Myles C. Maxfield 2017-06-09 13:26:44 PDT

Created attachment 312482 [details]
Broken test

Comment 7 Myles C. Maxfield 2017-06-10 21:31:25 PDT

Created attachment 312589 [details]
Patch

Comment 8 Myles C. Maxfield 2017-06-11 11:34:44 PDT

Created attachment 312611 [details]
Patch

Comment 9 Simon Fraser (smfr) 2017-06-12 15:47:26 PDT

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?

Comment 10 Myles C. Maxfield 2017-06-12 16:37:25 PDT

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.

Comment 11 WebKit Commit Bot 2017-06-12 17:05:59 PDT

Comment on attachment 312611 [details]
Patch

Clearing flags on attachment: 312611

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

Comment 12 WebKit Commit Bot 2017-06-12 17:06:01 PDT

All reviewed patches have been landed.  Closing bug.

Comment 13 Matt Lewis 2017-06-13 17:10:08 PDT

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

Comment 14 Ryan Haddad 2017-06-14 09:40:22 PDT

(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]

Comment 15 Matt Lewis 2017-06-14 09:40:31 PDT

Reverted r218157 for reason:

This patch caused multiple API failures on iOS Simulator.

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

Comment 16 Myles C. Maxfield 2017-06-21 11:21:40 PDT

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

Comment 17 Myles C. Maxfield 2017-06-21 15:51:53 PDT

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

Comment 18 Myles C. Maxfield 2017-06-21 16:22:08 PDT

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.

Comment 19 Myles C. Maxfield 2017-06-21 21:54:56 PDT

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.

Comment 20 Myles C. Maxfield 2017-06-22 17:36:00 PDT

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

Comment 21 Myles C. Maxfield 2017-06-22 18:15:45 PDT

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

Comment 22 Myles C. Maxfield 2017-06-22 19:17:27 PDT

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