Bug 156605 – [CSS Font Loading] FontFace's promise may never be resolved/rejected if Content Security Policy blocks all the URLs

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Myles C. Maxfield]

Created attachment 276444 [details]
Run this inside Github.com

Run the attached JavaScript file inside the inspector when it's attached to github.com (github has CSP).

[Myles C. Maxfield]

This was reported in https://twitter.com/nekrtemplar/status/720348427415109632 and https://gist.github.com/NekR/0dbc3ccdc76e10a6b8c9dfcfa12de3e3

[Myles C. Maxfield]

If CSP blocks all the URLs, we'll create a CSSFontFace without any CSSFontFaceSources.

[Myles C. Maxfield]

This means that CSSFontFace::pump() will never execute its loop, so the CSSFontFace's status will still be Pending. Therefore, we never call setStatus() with a terminal state, so we never resolve / reject the promise.

[Myles C. Maxfield]

The spec isn't quite clear what to do in this case. It seems like the load should fail, causing the promise to be rejected.

[Myles C. Maxfield]

Created attachment 276447 [details]
Needs a test

[Myles C. Maxfield]

(In reply to comment #5)
> Created attachment 276447 [details]
> Needs a test

Talking with dbates, the load should fail.

[Myles C. Maxfield]

Created attachment 276490 [details]
Patch

[Myles C. Maxfield]

Created attachment 276491 [details]
Patch

[Daniel Bates]

Comment on attachment 276491 [details]
Patch

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

> LayoutTests/fast/text/font-loading-csp-block-all.html:16
> +var globalVariable;

The name of this variable is generic and does not convey its purpose. Can we come up with a better name for this variable? Maybe exception?

> LayoutTests/fast/text/font-loading-csp-block-all.html:33
> +}, 10000);

A timeout of 10 seconds is excessive. I am not at a computer with a checkout at he moment.  What do we do in other tests that use promises?

[Myles C. Maxfield]

Created attachment 276504 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 276504 [details]
Patch

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

> LayoutTests/fast/text/font-loading-csp-block-all.html:15
> +var promiseFired = false;

This is unnecessary anymore.

[Myles C. Maxfield]

Created attachment 276506 [details]
Patch

[Daniel Bates]

Comment on attachment 276506 [details]
Patch

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

> Source/WebCore/css/CSSFontFace.cpp:462
> +    if (!m_sources.size() && m_status == Status::Pending)

Can m_sources.size() change while executing this function? I assume it can since we repeatedly compute it on each loop iteration, here, and on line 464 (below). If we can assume that it will not change, thence should cache its value once and use it throughout this function. Otherwise, we should use m_sources.isEmpty() here instead of checking if the size of m_sources is zero.

> Source/WebCore/css/CSSFontFace.cpp:466
>      return m_sources.size();

I know this this is not part of the patch. Is it necessary to compute m_sources.size() here?

> LayoutTests/fast/text/font-loading-csp-block-all.html:4
> +<meta http-equiv="Content-Security-Policy" content="font-src 'none';">

Nit: The trailing semicolon is not necessary since the policy consists of exactly one directive. Semicolons are used to separate directives.

> LayoutTests/fast/text/font-loading-csp-block-all.html:13
> +var face = new FontFace('WebFont', "url(../../resources/Ahem.woff) format('woff')", { });

Nit: 'WebFont' => "WebFont"
(notice the use of double quotes to be consistent with the quoting style used for other string literals in this patch)

[Myles C. Maxfield]

Comment on attachment 276506 [details]
Patch

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

>> Source/WebCore/css/CSSFontFace.cpp:462
>> +    if (!m_sources.size() && m_status == Status::Pending)
> 
> Can m_sources.size() change while executing this function? I assume it can since we repeatedly compute it on each loop iteration, here, and on line 464 (below). If we can assume that it will not change, thence should cache its value once and use it throughout this function. Otherwise, we should use m_sources.isEmpty() here instead of checking if the size of m_sources is zero.

It can't change here, but a cache is unnecessary because the call is inlined.

But you're right about isEmpty(), that is a better name.

[Myles C. Maxfield]

Committed r199611: <http://trac.webkit.org/changeset/199611>