Bug 205770

Summary: new FontFace() should not throw when failing to parse arguments
Product: WebKit Reporter: Sam Weinig <sam>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, dino, esprehn+autocc, ews-feeder, ews-watchlist, glenn, gyuyoung.kim, kondapallykalyan, macpherson, megan_gardner, menard, mmaxfield, nmouchtaris, webkit-bug-importer
Priority: P2 Keywords: GoodFirstBug, InRadar, WPTImpact
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Test case
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from webkit-cq-01 for mac-mojave none

Description Sam Weinig 2020-01-04 16:21:39 PST
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".
Comment 1 Sam Weinig 2020-01-04 16:24:04 PST
Created attachment 386773 [details]
Test case
Comment 2 Sam Weinig 2020-01-04 16:27:13 PST
This causes a failure (unrelated to what it is intending to test) in http://wpt.live/WebIDL/current-realm.html
Comment 3 Myles C. Maxfield 2020-01-05 08:39:17 PST
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.
Comment 4 Nikos Mouchtaris 2020-02-04 17:21:12 PST
Created attachment 389749 [details]
Patch
Comment 5 Myles C. Maxfield 2020-02-05 17:28:50 PST
Comment on attachment 389749 [details]
Patch

r- for red EWS bubbles.
Comment 6 Nikos Mouchtaris 2020-02-10 18:29:54 PST
Created attachment 390334 [details]
Patch
Comment 7 Nikos Mouchtaris 2020-02-10 19:40:01 PST
Created attachment 390338 [details]
Patch
Comment 8 Myles C. Maxfield 2020-02-11 10:05:58 PST
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.
Comment 9 Nikos Mouchtaris 2020-02-11 14:18:05 PST
Created attachment 390423 [details]
Patch
Comment 10 Myles C. Maxfield 2020-02-12 09:28:22 PST
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.
Comment 11 Nikos Mouchtaris 2020-02-12 09:46:33 PST
Created attachment 390529 [details]
Patch
Comment 12 Nikos Mouchtaris 2020-02-14 12:53:30 PST
Created attachment 390800 [details]
Patch
Comment 13 EWS 2020-02-14 13:13:32 PST
Comment on attachment 390800 [details]
Patch

Rejecting attachment 390800 [details] from commit-queue.

nmouchtaris@apple.com 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.
Comment 14 WebKit Commit Bot 2020-02-14 14:29:38 PST
The commit-queue encountered the following flaky tests while processing attachment 390800 [details]:

The commit-queue is continuing to process your patch.
Comment 15 WebKit Commit Bot 2020-02-14 14:30:05 PST
The commit-queue encountered the following flaky tests while processing attachment 390800 [details]:

editing/spelling/spellcheck-attribute.html bug 206178 (authors: g.czajkowski@samsung.com, mark.lam@apple.com, and rniwa@webkit.org)
The commit-queue is continuing to process your patch.
Comment 16 WebKit Commit Bot 2020-02-14 14:46:18 PST
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
Comment 17 WebKit Commit Bot 2020-02-14 14:46:20 PST
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
Comment 18 Sam Weinig 2020-02-14 16:31:46 PST
Very nice comment art!
Comment 19 WebKit Commit Bot 2020-02-14 17:18:10 PST
The commit-queue encountered the following flaky tests while processing attachment 390800 [details]:

editing/spelling/spellcheck-attribute.html bug 206178 (authors: g.czajkowski@samsung.com, mark.lam@apple.com, and rniwa@webkit.org)
The commit-queue is continuing to process your patch.
Comment 20 WebKit Commit Bot 2020-02-14 17:18:55 PST
Comment on attachment 390800 [details]
Patch

Clearing flags on attachment: 390800

Committed r256659: <https://trac.webkit.org/changeset/256659>
Comment 21 WebKit Commit Bot 2020-02-14 17:18:57 PST
All reviewed patches have been landed.  Closing bug.
Comment 22 Radar WebKit Bug Importer 2020-02-14 17:19:20 PST
<rdar://problem/59479370>