Bug 205770 – new FontFace() should not throw when failing to parse arguments

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Sam Weinig]

According to the spec, https://drafts.csswg.org/css-font-loading/#dom-fontface-fontface, the FontFace constructor should not throw when it encounters a parse error from its arguments, but rather, should set the "status" to "error" and reject [[FontStatusPromise]] with a DOMException named "SyntaxError".

[Sam Weinig]

Created attachment 386773 [details]
Test case

[Sam Weinig]

This causes a failure (unrelated to what it is intending to test) in http://wpt.live/WebIDL/current-realm.html

[Myles C. Maxfield]

An older version of the spec dictated that we should throw. The spec got updated. We should fix this bug in WebKit. Should be easy.

[Nikos Mouchtaris]

Created attachment 389749 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 389749 [details]
Patch

r- for red EWS bubbles.

[Nikos Mouchtaris]

Created attachment 390334 [details]
Patch

[Nikos Mouchtaris]

Created attachment 390338 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 390338 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        No longer throws, rejects promise, sets status to error, and 

"(No longer throws), (rejects promise), (sets status to error)" ..... or "No longer (throws, rejects promise, sets status to error)" ?

> Source/WebCore/css/CSSFontFace.cpp:719
> +    setStatus(Status::Loading);
> +    setStatus(Status::Failure);
> +    m_inErrorState = true;

Is this legal regardless of what state the CSSFontFace happens to be in? It seems wrong to not even consult the current state. Perhaps ASSERT that there is a path from whatever the current state is to ::Failure.

> Source/WebCore/css/CSSFontFace.h:86
> +    const Optional<CSSValueList*> families() const { return m_inErrorState ? WTF::nullopt : static_cast<Optional<CSSValueList*>>(m_families.get()); }
> +    Optional<FontSelectionRange> weight() const { return m_inErrorState ? WTF::nullopt : static_cast<Optional<FontSelectionRange>>(m_fontSelectionCapabilities.computeWeight()); }
> +    Optional<FontSelectionRange> stretch() const { return m_inErrorState ? WTF::nullopt : static_cast<Optional<FontSelectionRange>>(m_fontSelectionCapabilities.computeWidth()); }
> +    Optional<FontSelectionRange> italic() const { return m_inErrorState ? WTF::nullopt : static_cast<Optional<FontSelectionRange>>(m_fontSelectionCapabilities.computeSlope()); }
> +    Optional<FontSelectionCapabilities> fontSelectionCapabilities() const { return m_inErrorState ? WTF::nullopt : static_cast<Optional<FontSelectionCapabilities>>(m_fontSelectionCapabilities.computeFontSelectionCapabilities()); }
> +    const Optional<Vector<UnicodeRange>> ranges() const { return m_inErrorState ? WTF::nullopt : static_cast<Optional<Vector<UnicodeRange>>>(m_ranges); }
> +    const Optional<FontFeatureSettings> featureSettings() const { return m_inErrorState ? WTF::nullopt : static_cast<Optional<FontFeatureSettings>>(m_featureSettings); }
> +    Optional<FontLoadingBehavior> loadingBehavior() const { return m_inErrorState ? WTF::nullopt :  static_cast<Optional<FontLoadingBehavior>>(m_loadingBehavior); }

I'd appreciate a comment describing what it means for these accessors to return nullopt. (It means that the fields represent the empty string, but the backing store can't actually represent the empty string, so we use optionals here.) If the backing store *can* represent an empty string, there's no need to use an Optional here.

> Source/WebCore/css/CSSFontFace.h:196
> +    bool m_inErrorState { false };

The spec doesn't say anything about a new state. We already have CSSFontFace::Status::Failure. Why do we need a new, super-failure, state? What's the difference between this bool and being in the failure state? Why do these two situations need to be distinct?

I also think this doesn't quite follow the spec. It says "If any of them fail to parse correctly, ... set font face’s corresponding attributes to the empty string." I think "corresponding attributes" means "just the ones that failed to parse." There should be tests added for this (hopefully contributed to WPT).

> Source/WebCore/css/CSSFontFaceSet.cpp:163
> +    if (!familiesWrapped.hasValue())

I would just merge this with the (!face.families()) guard above.

> Source/WebCore/css/CSSFontFaceSet.cpp:244
> +    if (face.families().hasValue())

You removed the check for whether or not the CSSValueList* is null. Please add a test which would have caught this bug.

> Source/WebCore/css/CSSFontFaceSet.cpp:461
> +            if (!fontSelectionCapabilitiesWrapped.hasValue())
> +                continue;

This should just be an ASSERT(). You already made sure that nothing gets added to candidateFontFaces which has a nullopt fontSelectionCapabilities.

> Source/WebCore/css/CSSFontFaceSet.cpp:472
> +            auto firstCapabilitiesWrapped = first.fontSelectionCapabilities();
> +            auto secondCapabilitiesWrapped = second.fontSelectionCapabilities();
> +            if (!firstCapabilitiesWrapped.hasValue())
> +                return false;
> +            if (!secondCapabilitiesWrapped.hasValue())
> +                return false;

Ditto.

> Source/WebCore/css/CSSSegmentedFontFace.cpp:123
> +            if (!selectionCapabilitiesWrapped.hasValue())
> +                continue;

You should be able to ASSERT(selectionCapabilitiesWrapped.hasValue()) here. The code you wrote in CSSFontFaceSet::fontFace() should disallow any error-ed fonts to make it into this collection.

> Source/WebCore/css/FontFace.cpp:271
> +    auto familiesUnrwapped = families.value();

typo: unrwapped

> Source/WebCore/css/FontFace.cpp:390
> +    auto featureSettings = featureSettingsWrapped.value();

auto&

No need to make a copy

Also applicable to everywhere above

> LayoutTests/http/tests/css/font-face-constructor-expected.txt:11
> +
> +
> +
> +
> +
> +
> +

This indicates a bad design. A test should say PASS for each check it makes.

> LayoutTests/http/tests/css/font-face-constructor.html:26
> +        debug(testFontFace.family);
> +        debug(testFontFace.style);
> +        debug(testFontFace.weight);
> +        debug(testFontFace.stretch);
> +        debug(testFontFace.unicodeRange);
> +        debug(testFontFace.featureSettings);
> +        debug(testFontFace.display);
> +        document.fonts.add(testFontFace);
> +        debug("PASS: Did not throw");

Instead of "debug" you should use "shouldBeEqualToString" instead.

[Nikos Mouchtaris]

Created attachment 390423 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 390423 [details]
Patch

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

> Source/WebCore/css/CSSFontFace.cpp:728
> +    setStatus(Status::Failure);

What if it's already in the failure state? I don't think the state diagram has loops in it.

> Source/WebCore/css/FontFace.cpp:374
> +        return "U+0-10FFFF";

Shouldn't this be the empty string?

> Source/WebCore/css/FontFace.cpp:389
> +        return "normal"_s;

Ditto.

[Nikos Mouchtaris]

Created attachment 390529 [details]
Patch

[Nikos Mouchtaris]

Created attachment 390800 [details]
Patch

[EWS]

Comment on attachment 390800 [details]
Patch

Rejecting attachment 390800 [details] from commit-queue.

[e-mail] does not have committer permissions according to https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 390800 [details]:

The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 390800 [details]:

editing/spelling/spellcheck-attribute.html bug 206178 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 390800 [details]
Patch

Rejecting attachment 390800 [details] from commit-queue.

New failing tests:
imported/w3c/web-platform-tests/web-animations/timing-model/timelines/update-and-send-events.html
Full output: https://webkit-queues.webkit.org/results/13323017

[WebKit Commit Bot]

Created attachment 390825 [details]
Archive of layout-test-results from webkit-cq-01 for mac-mojave

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: webkit-cq-01  Port: mac-mojave  Platform: Mac OS X 10.14.6

[Sam Weinig]

Very nice comment art!

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 390800 [details]:

editing/spelling/spellcheck-attribute.html bug 206178 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 390800 [details]
Patch

Clearing flags on attachment: 390800

Committed r256659: <https://trac.webkit.org/changeset/256659>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59479370>