- Revision
- 225414
- Author
- [email protected]
- Date
- 2017-12-01 14:13:37 -0800 (Fri, 01 Dec 2017)
Log Message
Free FontFaceSets may include fonts that were never actually added to them
https://bugs.webkit.org/show_bug.cgi?id=180164
Reviewed by Simon Fraser.
Source/WebCore:
There are two circumstances where this can occur:
- If script makes a so-called "free" FontFaceSet, by using "new FontFaceSet". This object is not
associated with the document, and should therefore only include fonts which have been manually
added to it from script. However, today, this object includes preinstalled fonts which have the
same names as any fonts manually added to it. (So, if you manually add "Helvetica", the object
would have two objects - the one you just added and the preinstalled version too).
- For the document's FontFaceSet, the same thing would happen. This one is a little trickier
because the spec is not clear whether or not the document's FontFaceSet should include these
preinstalled fonts. However, running this test in Firefox and Chrome, they both agree that
preinstalled fonts should not be present, so this patch adheres to this behavior.
We can't actually remove the preinstalled fonts from the document's FontFaceSet (because that's
how normal font lookups are performed), but we can filter them out at the point they meet the
_javascript_ API. And, for "free" FontFaceSets, we can avoid adding them in the first place for
performance.
Test: fast/text/font-face-api-preinstalled.html
* css/CSSFontFaceSet.cpp:
(WebCore::CSSFontFaceSet::CSSFontFaceSet):
(WebCore::CSSFontFaceSet::ensureLocalFontFacesForFamilyRegistered):
(WebCore::CSSFontFaceSet::addToFacesLookupTable):
(WebCore::CSSFontFaceSet::matchingFacesExcludingPreinstalledFonts):
(WebCore::CSSFontFaceSet::check):
(WebCore::CSSFontFaceSet::matchingFaces): Deleted.
* css/CSSFontFaceSet.h:
* css/CSSFontSelector.cpp:
(WebCore::CSSFontSelector::CSSFontSelector):
* css/FontFaceSet.cpp:
(WebCore::FontFaceSet::load):
LayoutTests:
* fast/text/font-face-api-preinstalled-expected.txt: Added.
* fast/text/font-face-api-preinstalled.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (225413 => 225414)
--- trunk/LayoutTests/ChangeLog 2017-12-01 22:12:01 UTC (rev 225413)
+++ trunk/LayoutTests/ChangeLog 2017-12-01 22:13:37 UTC (rev 225414)
@@ -1,3 +1,13 @@
+2017-12-01 Myles C. Maxfield <[email protected]>
+
+ Free FontFaceSets may include fonts that were never actually added to them
+ https://bugs.webkit.org/show_bug.cgi?id=180164
+
+ Reviewed by Simon Fraser.
+
+ * fast/text/font-face-api-preinstalled-expected.txt: Added.
+ * fast/text/font-face-api-preinstalled.html: Added.
+
2017-12-01 Ryan Haddad <[email protected]>
Update TestExpectations for various editing tests on iOS.
Added: trunk/LayoutTests/fast/text/font-face-api-preinstalled-expected.txt (0 => 225414)
--- trunk/LayoutTests/fast/text/font-face-api-preinstalled-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/text/font-face-api-preinstalled-expected.txt 2017-12-01 22:13:37 UTC (rev 225414)
@@ -0,0 +1,11 @@
+This test makes sure that preinstalled fonts are never returned from the CSS Font Loading API.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS result.length is 0
+PASS result.length is 0
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/fast/text/font-face-api-preinstalled.html (0 => 225414)
--- trunk/LayoutTests/fast/text/font-face-api-preinstalled.html (rev 0)
+++ trunk/LayoutTests/fast/text/font-face-api-preinstalled.html 2017-12-01 22:13:37 UTC (rev 225414)
@@ -0,0 +1,35 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src=""
+</head>
+<body>
+<script>
+window.jsTestIsAsync = true;
+
+description("This test makes sure that preinstalled fonts are never returned from the CSS Font Loading API.");
+var result;
+
+var fontFaceSet = new FontFaceSet([]);
+var fontFace = new FontFace("Helvetica", "url('../../resources/Ahem.ttf')", {unicodeRange: "U+10FFFF"});
+fontFaceSet.add(fontFace);
+fontFaceSet.load("16px Helvetica", "test").then(function(r) {
+ result = r;
+ shouldBe("result.length", "0");
+}).then(function() {
+ fontFaceSet = document.fonts;
+ fontFaceSet.add(fontFace);
+ return fontFaceSet.load("16px Helvetica", "test")
+}).then(function(r) {
+ result = r;
+ shouldBe("result.length", "0");
+ finishJSTest();
+}, function(result) {
+ testFailed("Load should be successful.");
+});
+</script>
+<script src=""
+</script>
+</div>
+</body>
+</html>
Modified: trunk/Source/WebCore/ChangeLog (225413 => 225414)
--- trunk/Source/WebCore/ChangeLog 2017-12-01 22:12:01 UTC (rev 225413)
+++ trunk/Source/WebCore/ChangeLog 2017-12-01 22:13:37 UTC (rev 225414)
@@ -1,3 +1,43 @@
+2017-12-01 Myles C. Maxfield <[email protected]>
+
+ Free FontFaceSets may include fonts that were never actually added to them
+ https://bugs.webkit.org/show_bug.cgi?id=180164
+
+ Reviewed by Simon Fraser.
+
+ There are two circumstances where this can occur:
+
+ - If script makes a so-called "free" FontFaceSet, by using "new FontFaceSet". This object is not
+ associated with the document, and should therefore only include fonts which have been manually
+ added to it from script. However, today, this object includes preinstalled fonts which have the
+ same names as any fonts manually added to it. (So, if you manually add "Helvetica", the object
+ would have two objects - the one you just added and the preinstalled version too).
+
+ - For the document's FontFaceSet, the same thing would happen. This one is a little trickier
+ because the spec is not clear whether or not the document's FontFaceSet should include these
+ preinstalled fonts. However, running this test in Firefox and Chrome, they both agree that
+ preinstalled fonts should not be present, so this patch adheres to this behavior.
+
+ We can't actually remove the preinstalled fonts from the document's FontFaceSet (because that's
+ how normal font lookups are performed), but we can filter them out at the point they meet the
+ _javascript_ API. And, for "free" FontFaceSets, we can avoid adding them in the first place for
+ performance.
+
+ Test: fast/text/font-face-api-preinstalled.html
+
+ * css/CSSFontFaceSet.cpp:
+ (WebCore::CSSFontFaceSet::CSSFontFaceSet):
+ (WebCore::CSSFontFaceSet::ensureLocalFontFacesForFamilyRegistered):
+ (WebCore::CSSFontFaceSet::addToFacesLookupTable):
+ (WebCore::CSSFontFaceSet::matchingFacesExcludingPreinstalledFonts):
+ (WebCore::CSSFontFaceSet::check):
+ (WebCore::CSSFontFaceSet::matchingFaces): Deleted.
+ * css/CSSFontFaceSet.h:
+ * css/CSSFontSelector.cpp:
+ (WebCore::CSSFontSelector::CSSFontSelector):
+ * css/FontFaceSet.cpp:
+ (WebCore::FontFaceSet::load):
+
2017-12-01 Dean Jackson <[email protected]>
Attempted build fix.
Modified: trunk/Source/WebCore/css/CSSFontFaceSet.cpp (225413 => 225414)
--- trunk/Source/WebCore/css/CSSFontFaceSet.cpp 2017-12-01 22:12:01 UTC (rev 225413)
+++ trunk/Source/WebCore/css/CSSFontFaceSet.cpp 2017-12-01 22:13:37 UTC (rev 225414)
@@ -40,7 +40,8 @@
namespace WebCore {
-CSSFontFaceSet::CSSFontFaceSet()
+CSSFontFaceSet::CSSFontFaceSet(CSSFontSelector* owningFontSelector)
+ : m_owningFontSelector(owningFontSelector)
{
}
@@ -98,6 +99,7 @@
void CSSFontFaceSet::ensureLocalFontFacesForFamilyRegistered(const String& familyName)
{
+ ASSERT(m_owningFontSelector);
if (m_locallyInstalledFacesLookupTable.contains(familyName))
return;
@@ -164,7 +166,8 @@
if (addResult.isNewEntry) {
// m_locallyInstalledFontFaces grows without bound, eventually encorporating every font installed on the system.
// This is by design.
- ensureLocalFontFacesForFamilyRegistered(familyName);
+ if (m_owningFontSelector)
+ ensureLocalFontFacesForFamilyRegistered(familyName);
familyFontFaces = { };
}
@@ -328,7 +331,7 @@
return result;
}
-ExceptionOr<Vector<std::reference_wrapper<CSSFontFace>>> CSSFontFaceSet::matchingFaces(const String& font, const String& string)
+ExceptionOr<Vector<std::reference_wrapper<CSSFontFace>>> CSSFontFaceSet::matchingFacesExcludingPreinstalledFonts(const String& font, const String& string)
{
auto style = MutableStyleProperties::create();
auto parseResult = CSSParser::parseValue(style, CSSPropertyFont, font, true, HTMLStandardMode);
@@ -360,6 +363,8 @@
if (!faces)
continue;
for (auto& constituentFace : faces->constituentFaces()) {
+ if (constituentFace->isLocalFallback())
+ continue;
if (constituentFace->rangesMatchCodePoint(codePoint)) {
resultConstituents.add(constituentFace.ptr());
found = true;
@@ -380,7 +385,7 @@
ExceptionOr<bool> CSSFontFaceSet::check(const String& font, const String& text)
{
- auto matchingFaces = this->matchingFaces(font, text);
+ auto matchingFaces = this->matchingFacesExcludingPreinstalledFonts(font, text);
if (matchingFaces.hasException())
return matchingFaces.releaseException();
Modified: trunk/Source/WebCore/css/CSSFontFaceSet.h (225413 => 225414)
--- trunk/Source/WebCore/css/CSSFontFaceSet.h 2017-12-01 22:12:01 UTC (rev 225413)
+++ trunk/Source/WebCore/css/CSSFontFaceSet.h 2017-12-01 22:13:37 UTC (rev 225414)
@@ -46,9 +46,9 @@
class CSSFontFaceSet final : public RefCounted<CSSFontFaceSet>, public CSSFontFace::Client {
public:
- static Ref<CSSFontFaceSet> create()
+ static Ref<CSSFontFaceSet> create(CSSFontSelector* owningFontSelector = nullptr)
{
- return adoptRef(*new CSSFontFaceSet());
+ return adoptRef(*new CSSFontFaceSet(owningFontSelector));
}
~CSSFontFaceSet();
@@ -75,7 +75,7 @@
bool hasActiveFontFaces() { return status() == Status::Loading; }
- ExceptionOr<Vector<std::reference_wrapper<CSSFontFace>>> matchingFaces(const String& font, const String& text);
+ ExceptionOr<Vector<std::reference_wrapper<CSSFontFace>>> matchingFacesExcludingPreinstalledFonts(const String& font, const String& text);
// CSSFontFace::Client needs to be able to be held in a RefPtr.
void ref() final { RefCounted::ref(); }
@@ -82,7 +82,7 @@
void deref() final { RefCounted::deref(); }
private:
- CSSFontFaceSet();
+ CSSFontFaceSet(CSSFontSelector*);
void removeFromFacesLookupTable(const CSSFontFace&, const CSSValueList& familiesToSearchFor);
void addToFacesLookupTable(CSSFontFace&);
@@ -107,6 +107,7 @@
size_t m_facesPartitionIndex { 0 }; // All entries in m_faces before this index are CSS-connected.
Status m_status { Status::Loaded };
HashSet<CSSFontFaceSetClient*> m_clients;
+ CSSFontSelector* m_owningFontSelector;
unsigned m_activeCount { 0 };
};
Modified: trunk/Source/WebCore/css/CSSFontSelector.cpp (225413 => 225414)
--- trunk/Source/WebCore/css/CSSFontSelector.cpp 2017-12-01 22:12:01 UTC (rev 225413)
+++ trunk/Source/WebCore/css/CSSFontSelector.cpp 2017-12-01 22:13:37 UTC (rev 225414)
@@ -60,7 +60,7 @@
CSSFontSelector::CSSFontSelector(Document& document)
: m_document(&document)
- , m_cssFontFaceSet(CSSFontFaceSet::create())
+ , m_cssFontFaceSet(CSSFontFaceSet::create(this))
, m_beginLoadingTimer(*this, &CSSFontSelector::beginLoadTimerFired)
, m_uniqueId(++fontSelectorId)
, m_version(0)
Modified: trunk/Source/WebCore/css/FontFaceSet.cpp (225413 => 225414)
--- trunk/Source/WebCore/css/FontFaceSet.cpp 2017-12-01 22:12:01 UTC (rev 225413)
+++ trunk/Source/WebCore/css/FontFaceSet.cpp 2017-12-01 22:13:37 UTC (rev 225414)
@@ -125,7 +125,7 @@
void FontFaceSet::load(const String& font, const String& text, LoadPromise&& promise)
{
- auto matchingFacesResult = m_backing->matchingFaces(font, text);
+ auto matchingFacesResult = m_backing->matchingFacesExcludingPreinstalledFonts(font, text);
if (matchingFacesResult.hasException()) {
promise.reject(matchingFacesResult.releaseException());
return;
