Bug 180164 - Free FontFaceSets may include fonts that were never actually added to them
Summary: Free FontFaceSets may include fonts that were never actually added to them
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks: 180062
  Show dependency treegraph
 
Reported: 2017-11-29 13:18 PST by Myles C. Maxfield
Modified: 2017-12-01 14:19 PST (History)
9 users (show)

See Also:


Attachments
Reproduction. Should alert("0") (716 bytes, text/html)
2017-11-29 13:20 PST, Myles C. Maxfield
no flags Details
WIP (4.63 KB, patch)
2017-11-29 20:57 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (10.77 KB, patch)
2017-11-30 13:18 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2017-11-29 13:18:43 PST
We always add preinstalled fonts to CSSFontFaceSets, even if that CSSFontFaceSet was created by script and isn't owned by the Document. This is visible if you try to load one of these fonts.
Comment 1 Myles C. Maxfield 2017-11-29 13:20:52 PST
Created attachment 327892 [details]
Reproduction. Should alert("0")
Comment 2 Myles C. Maxfield 2017-11-29 15:38:01 PST
Looks like neither Firefox nor Chrome support constructing free FontFaceSets?
Comment 3 Myles C. Maxfield 2017-11-29 15:38:15 PST
at least, "new FontFaceSet([])" gives an error in those browsers.
Comment 4 Myles C. Maxfield 2017-11-29 20:57:09 PST
Created attachment 327950 [details]
WIP
Comment 5 EWS Watchlist 2017-11-29 20:59:49 PST
Attachment 327950 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Myles C. Maxfield 2017-11-30 13:18:58 PST
Created attachment 328011 [details]
Patch
Comment 7 Simon Fraser (smfr) 2017-12-01 13:49:00 PST
Comment on attachment 328011 [details]
Patch

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

> Source/WebCore/css/CSSFontFaceSet.cpp:169
> +            if (m_owningFontSelector)

Why the if check if it's always non-null?

> Source/WebCore/css/CSSFontFaceSet.h:110
> +    CSSFontSelector* m_owningFontSelector;

If this is always non-null why not use a ref?
Comment 8 Myles C. Maxfield 2017-12-01 13:52:21 PST
Comment on attachment 328011 [details]
Patch

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

>> Source/WebCore/css/CSSFontFaceSet.h:110
>> +    CSSFontSelector* m_owningFontSelector;
> 
> If this is always non-null why not use a ref?

One client calls CSSFontFaceSet::create() with no arguments, and the (only) other client calls it with (this) as an argument.
Comment 9 WebKit Commit Bot 2017-12-01 14:13:41 PST
Comment on attachment 328011 [details]
Patch

Clearing flags on attachment: 328011

Committed r225414: <https://trac.webkit.org/changeset/225414>
Comment 10 WebKit Commit Bot 2017-12-01 14:13:42 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2017-12-01 14:19:38 PST
<rdar://problem/35805626>