Bug 180164

Summary: Free FontFaceSets may include fonts that were never actually added to them
Product: WebKit Reporter: Myles C. Maxfield <mmaxfield>
Component: TextAssignee: Myles C. Maxfield <mmaxfield>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dino, ews-watchlist, jonlee, koivisto, mmaxfield, simon.fraser, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 180062    
Attachments:
Description Flags
Reproduction. Should alert("0")
none
WIP
none
Patch none

Myles C. Maxfield
Reported 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.
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
DetailsFormatted DiffDiff
Patch (10.77 KB, patch)
2017-11-30 13:18 PST, Myles C. Maxfield 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Myles C. Maxfield
Comment 1 2017-11-29 13:20:52 PST
Created attachment 327892 [details] Reproduction. Should alert("0")
Myles C. Maxfield
Comment 2 2017-11-29 15:38:01 PST
Looks like neither Firefox nor Chrome support constructing free FontFaceSets?
Myles C. Maxfield
Comment 3 2017-11-29 15:38:15 PST
at least, "new FontFaceSet([])" gives an error in those browsers.
Myles C. Maxfield
Comment 4 2017-11-29 20:57:09 PST
Created attachment 327950 [details] WIP
EWS Watchlist
Comment 5 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.
Myles C. Maxfield
Comment 6 2017-11-30 13:18:58 PST
Created attachment 328011 [details] Patch
Simon Fraser (smfr)
Comment 7 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?
Myles C. Maxfield
Comment 8 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.
WebKit Commit Bot
Comment 9 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>
WebKit Commit Bot
Comment 10 2017-12-01 14:13:42 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 11 2017-12-01 14:19:38 PST
<rdar://problem/35805626>
Note You need to log in before you can comment on or make changes to this bug.