Make FontFaceCache::version return unique number across all instances (issue 2653773006 by ksakamoto@chromium.org)
0 views
Skip to first unread message
ksak...@chromium.org
Jan 24, 2017, 4:36:08 AM1/24/17
to e...@chromium.org, chromium...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, apavlo...@chromium.org, alexis...@intel.com, blink-...@chromium.org, rob....@samsung.com, drot...@chromium.org, ko...@chromium.org
Reviewers: eae
CL: https://codereview.chromium.org/2653773006/
Description:
Make FontFaceCache::version return unique number across all instances
The global FallbackListShapeCache may contain entries created in
multiple documents that have different @font-face rules. Those entries
are versioned by FontFaceCache version, but entries from different
FontFaceCache instances can have same version number.
This patch makes sure that ShapeCache is invalidated when cached data is
created within the context of different FontFaceCache instance.
BUG=682999
Affected files (+68, -4 lines):
A third_party/WebKit/LayoutTests/fast/css/font-face-cache-version.html
A third_party/WebKit/LayoutTests/fast/css/resources/font-face-cache-version-frame1.html
A third_party/WebKit/LayoutTests/fast/css/resources/font-face-cache-version-frame2.html
M third_party/WebKit/Source/core/css/FontFaceCache.h
M third_party/WebKit/Source/core/css/FontFaceCache.cpp
Index: third_party/WebKit/LayoutTests/fast/css/font-face-cache-version.html
diff --git a/third_party/WebKit/LayoutTests/fast/css/font-face-cache-version.html b/third_party/WebKit/LayoutTests/fast/css/font-face-cache-version.html
new file mode 100644
index 0000000000000000000000000000000000000000..c23d9addaad7d4052859c66d2fcdf5959813a09b
--- /dev/null
+++ b/third_party/WebKit/LayoutTests/fast/css/font-face-cache-version.html
@@ -0,0 +1,22 @@
+<!DOCTYPE html>
+<script src="../../resources/testharness.js"></script>
+<script src="../../resources/testharnessreport.js"></script>
+<script>
+
+async_test(function(t) {
+ var messages = [];
+ window.addEventListener('message', (evt) => {
+ messages.push(evt.data);
+ if (messages.length == 2) {
+ t.step(() => {
+ assert_not_equals(messages[0], messages[1],
+ 'The texts in two iframes should not be identical');
+ t.done();
+ });
+ }
+ });
+}, 'Cached font from differnt document should not be used');
+
+</script>
+<iframe id='frame' src='resources/font-face-cache-version-frame1.html'></iframe>
+<iframe id='frame' src='resources/font-face-cache-version-frame2.html'></iframe>
Index: third_party/WebKit/LayoutTests/fast/css/resources/font-face-cache-version-frame1.html
diff --git a/third_party/WebKit/LayoutTests/fast/css/resources/font-face-cache-version-frame1.html b/third_party/WebKit/LayoutTests/fast/css/resources/font-face-cache-version-frame1.html
new file mode 100644
index 0000000000000000000000000000000000000000..7666ce65bc9d1e34d51f3a4d0c0cf04cd523097a
--- /dev/null
+++ b/third_party/WebKit/LayoutTests/fast/css/resources/font-face-cache-version-frame1.html
@@ -0,0 +1,18 @@
+<!DOCTYPE html>
+<style>
+@font-face {
+ font-family: CustomFont;
+ src: url('../../../resources/SpaceOnly.otf');
+}
+span {
+ font: 48px CustomFont;
+}
+</style>
+<span id="text"></span>
+<script>
+document.fonts.load('48px CustomFont').then(() => {
+ var span = document.getElementById('text');
+ span.innerText = 'Hello';
+ window.parent.postMessage(span.offsetWidth, '*');
+});
+</script>
Index: third_party/WebKit/LayoutTests/fast/css/resources/font-face-cache-version-frame2.html
diff --git a/third_party/WebKit/LayoutTests/fast/css/resources/font-face-cache-version-frame2.html b/third_party/WebKit/LayoutTests/fast/css/resources/font-face-cache-version-frame2.html
new file mode 100644
index 0000000000000000000000000000000000000000..ed5330132601d3356cf23b3741b9ba002e5a7cce
--- /dev/null
+++ b/third_party/WebKit/LayoutTests/fast/css/resources/font-face-cache-version-frame2.html
@@ -0,0 +1,18 @@
+<!DOCTYPE html>
+<style>
+@font-face {
+ font-family: CustomFont;
+ src: url('../../../resources/Ahem.ttf');
+}
+span {
+ font: 48px CustomFont;
+}
+</style>
+<span id="text"></span>
+<script>
+document.fonts.load('48px CustomFont').then(() => {
+ var span = document.getElementById('text');
+ span.innerText = 'Hello';
+ window.parent.postMessage(span.offsetWidth, '*');
+});
+</script>
Index: third_party/WebKit/Source/core/css/FontFaceCache.cpp
diff --git a/third_party/WebKit/Source/core/css/FontFaceCache.cpp b/third_party/WebKit/Source/core/css/FontFaceCache.cpp
index d13b86e8ef93156ca4d2b4d4428bb1632837350e..2b7ac476eced570faff87448b9fa8bb17f0ccfcc 100644
--- a/third_party/WebKit/Source/core/css/FontFaceCache.cpp
+++ b/third_party/WebKit/Source/core/css/FontFaceCache.cpp
@@ -40,6 +40,8 @@
namespace blink {
+static unsigned s_version = 0;
+
FontFaceCache::FontFaceCache() : m_version(0) {}
void FontFaceCache::add(CSSFontSelector* cssFontSelector,
@@ -71,7 +73,7 @@ void FontFaceCache::addFontFace(CSSFontSelector* cssFontSelector,
m_cssConnectedFontFaces.add(fontFace);
m_fonts.remove(fontFace->family());
- ++m_version;
+ incrementVersion();
}
void FontFaceCache::remove(const StyleRuleFontFace* fontFaceRule) {
@@ -105,7 +107,7 @@ void FontFaceCache::removeFontFace(FontFace* fontFace, bool cssConnected) {
if (cssConnected)
m_cssConnectedFontFaces.remove(fontFace);
- ++m_version;
+ incrementVersion();
}
void FontFaceCache::clearCSSConnected() {
@@ -122,7 +124,11 @@ void FontFaceCache::clearAll() {
m_fonts.clear();
m_styleRuleToFontFace.clear();
m_cssConnectedFontFaces.clear();
- ++m_version;
+ incrementVersion();
+}
+
+void FontFaceCache::incrementVersion() {
+ m_version = ++s_version;
}
CSSSegmentedFontFace* FontFaceCache::get(const FontDescription& fontDescription,
Index: third_party/WebKit/Source/core/css/FontFaceCache.h
diff --git a/third_party/WebKit/Source/core/css/FontFaceCache.h b/third_party/WebKit/Source/core/css/FontFaceCache.h
index 0ff19534749b19c876a50fff611eb8f87037900f..ab0b07bdc4d047dd88d859e9d6d71c72585eb62a 100644
--- a/third_party/WebKit/Source/core/css/FontFaceCache.h
+++ b/third_party/WebKit/Source/core/css/FontFaceCache.h
@@ -65,7 +65,7 @@ class FontFaceCache final {
}
unsigned version() const { return m_version; }
- void incrementVersion() { ++m_version; }
+ void incrementVersion();
DECLARE_TRACE();
CL: https://codereview.chromium.org/2653773006/
Description:
Make FontFaceCache::version return unique number across all instances
The global FallbackListShapeCache may contain entries created in
multiple documents that have different @font-face rules. Those entries
are versioned by FontFaceCache version, but entries from different
FontFaceCache instances can have same version number.
This patch makes sure that ShapeCache is invalidated when cached data is
created within the context of different FontFaceCache instance.
BUG=682999
Affected files (+68, -4 lines):
A third_party/WebKit/LayoutTests/fast/css/font-face-cache-version.html
A third_party/WebKit/LayoutTests/fast/css/resources/font-face-cache-version-frame1.html
A third_party/WebKit/LayoutTests/fast/css/resources/font-face-cache-version-frame2.html
M third_party/WebKit/Source/core/css/FontFaceCache.h
M third_party/WebKit/Source/core/css/FontFaceCache.cpp
Index: third_party/WebKit/LayoutTests/fast/css/font-face-cache-version.html
diff --git a/third_party/WebKit/LayoutTests/fast/css/font-face-cache-version.html b/third_party/WebKit/LayoutTests/fast/css/font-face-cache-version.html
new file mode 100644
index 0000000000000000000000000000000000000000..c23d9addaad7d4052859c66d2fcdf5959813a09b
--- /dev/null
+++ b/third_party/WebKit/LayoutTests/fast/css/font-face-cache-version.html
@@ -0,0 +1,22 @@
+<!DOCTYPE html>
+<script src="../../resources/testharness.js"></script>
+<script src="../../resources/testharnessreport.js"></script>
+<script>
+
+async_test(function(t) {
+ var messages = [];
+ window.addEventListener('message', (evt) => {
+ messages.push(evt.data);
+ if (messages.length == 2) {
+ t.step(() => {
+ assert_not_equals(messages[0], messages[1],
+ 'The texts in two iframes should not be identical');
+ t.done();
+ });
+ }
+ });
+}, 'Cached font from differnt document should not be used');
+
+</script>
+<iframe id='frame' src='resources/font-face-cache-version-frame1.html'></iframe>
+<iframe id='frame' src='resources/font-face-cache-version-frame2.html'></iframe>
Index: third_party/WebKit/LayoutTests/fast/css/resources/font-face-cache-version-frame1.html
diff --git a/third_party/WebKit/LayoutTests/fast/css/resources/font-face-cache-version-frame1.html b/third_party/WebKit/LayoutTests/fast/css/resources/font-face-cache-version-frame1.html
new file mode 100644
index 0000000000000000000000000000000000000000..7666ce65bc9d1e34d51f3a4d0c0cf04cd523097a
--- /dev/null
+++ b/third_party/WebKit/LayoutTests/fast/css/resources/font-face-cache-version-frame1.html
@@ -0,0 +1,18 @@
+<!DOCTYPE html>
+<style>
+@font-face {
+ font-family: CustomFont;
+ src: url('../../../resources/SpaceOnly.otf');
+}
+span {
+ font: 48px CustomFont;
+}
+</style>
+<span id="text"></span>
+<script>
+document.fonts.load('48px CustomFont').then(() => {
+ var span = document.getElementById('text');
+ span.innerText = 'Hello';
+ window.parent.postMessage(span.offsetWidth, '*');
+});
+</script>
Index: third_party/WebKit/LayoutTests/fast/css/resources/font-face-cache-version-frame2.html
diff --git a/third_party/WebKit/LayoutTests/fast/css/resources/font-face-cache-version-frame2.html b/third_party/WebKit/LayoutTests/fast/css/resources/font-face-cache-version-frame2.html
new file mode 100644
index 0000000000000000000000000000000000000000..ed5330132601d3356cf23b3741b9ba002e5a7cce
--- /dev/null
+++ b/third_party/WebKit/LayoutTests/fast/css/resources/font-face-cache-version-frame2.html
@@ -0,0 +1,18 @@
+<!DOCTYPE html>
+<style>
+@font-face {
+ font-family: CustomFont;
+ src: url('../../../resources/Ahem.ttf');
+}
+span {
+ font: 48px CustomFont;
+}
+</style>
+<span id="text"></span>
+<script>
+document.fonts.load('48px CustomFont').then(() => {
+ var span = document.getElementById('text');
+ span.innerText = 'Hello';
+ window.parent.postMessage(span.offsetWidth, '*');
+});
+</script>
Index: third_party/WebKit/Source/core/css/FontFaceCache.cpp
diff --git a/third_party/WebKit/Source/core/css/FontFaceCache.cpp b/third_party/WebKit/Source/core/css/FontFaceCache.cpp
index d13b86e8ef93156ca4d2b4d4428bb1632837350e..2b7ac476eced570faff87448b9fa8bb17f0ccfcc 100644
--- a/third_party/WebKit/Source/core/css/FontFaceCache.cpp
+++ b/third_party/WebKit/Source/core/css/FontFaceCache.cpp
@@ -40,6 +40,8 @@
namespace blink {
+static unsigned s_version = 0;
+
FontFaceCache::FontFaceCache() : m_version(0) {}
void FontFaceCache::add(CSSFontSelector* cssFontSelector,
@@ -71,7 +73,7 @@ void FontFaceCache::addFontFace(CSSFontSelector* cssFontSelector,
m_cssConnectedFontFaces.add(fontFace);
m_fonts.remove(fontFace->family());
- ++m_version;
+ incrementVersion();
}
void FontFaceCache::remove(const StyleRuleFontFace* fontFaceRule) {
@@ -105,7 +107,7 @@ void FontFaceCache::removeFontFace(FontFace* fontFace, bool cssConnected) {
if (cssConnected)
m_cssConnectedFontFaces.remove(fontFace);
- ++m_version;
+ incrementVersion();
}
void FontFaceCache::clearCSSConnected() {
@@ -122,7 +124,11 @@ void FontFaceCache::clearAll() {
m_fonts.clear();
m_styleRuleToFontFace.clear();
m_cssConnectedFontFaces.clear();
- ++m_version;
+ incrementVersion();
+}
+
+void FontFaceCache::incrementVersion() {
+ m_version = ++s_version;
}
CSSSegmentedFontFace* FontFaceCache::get(const FontDescription& fontDescription,
Index: third_party/WebKit/Source/core/css/FontFaceCache.h
diff --git a/third_party/WebKit/Source/core/css/FontFaceCache.h b/third_party/WebKit/Source/core/css/FontFaceCache.h
index 0ff19534749b19c876a50fff611eb8f87037900f..ab0b07bdc4d047dd88d859e9d6d71c72585eb62a 100644
--- a/third_party/WebKit/Source/core/css/FontFaceCache.h
+++ b/third_party/WebKit/Source/core/css/FontFaceCache.h
@@ -65,7 +65,7 @@ class FontFaceCache final {
}
unsigned version() const { return m_version; }
- void incrementVersion() { ++m_version; }
+ void incrementVersion();
DECLARE_TRACE();
dr...@chromium.org
Jan 24, 2017, 7:16:20 AM1/24/17
to ksak...@chromium.org, e...@chromium.org, alexis...@intel.com, apavlo...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, ko...@chromium.org, rob....@samsung.com
LGTM, great! Thanks for figuring this out and fixing this, I also like the
compact test case.
https://codereview.chromium.org/2653773006/diff/1/third_party/WebKit/Source/core/css/FontFaceCache.cpp
File third_party/WebKit/Source/core/css/FontFaceCache.cpp (right):
https://codereview.chromium.org/2653773006/diff/1/third_party/WebKit/Source/core/css/FontFaceCache.cpp#newcode131
third_party/WebKit/Source/core/css/FontFaceCache.cpp:131: m_version =
++s_version;
I assume multiple documents in the same renderer would all still be on
the main thread? We don't need anything like an atomic increment here?
https://codereview.chromium.org/2653773006/
compact test case.
https://codereview.chromium.org/2653773006/diff/1/third_party/WebKit/Source/core/css/FontFaceCache.cpp
File third_party/WebKit/Source/core/css/FontFaceCache.cpp (right):
https://codereview.chromium.org/2653773006/diff/1/third_party/WebKit/Source/core/css/FontFaceCache.cpp#newcode131
third_party/WebKit/Source/core/css/FontFaceCache.cpp:131: m_version =
++s_version;
I assume multiple documents in the same renderer would all still be on
the main thread? We don't need anything like an atomic increment here?
https://codereview.chromium.org/2653773006/
e...@chromium.org
Jan 24, 2017, 11:49:29 AM1/24/17
to ksak...@chromium.org, drott+r...@chromium.org, drott+r...@chromium.org, alexis...@intel.com, apavlo...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, ko...@chromium.org, rob....@samsung.com
commit-bot@chromium.org via codereview.chromium.org
Jan 24, 2017, 11:49:52 AM1/24/17
to ksak...@chromium.org, e...@chromium.org, drott+r...@chromium.org, commi...@chromium.org, drott+r...@chromium.org, alexis...@intel.com, apavlo...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, ko...@chromium.org, rob....@samsung.com
CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2653773006/1
https://codereview.chromium.org/2653773006/
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2653773006/1
https://codereview.chromium.org/2653773006/
commit-bot@chromium.org via codereview.chromium.org
Jan 24, 2017, 11:56:18 AM1/24/17
to ksak...@chromium.org, e...@chromium.org, drott+r...@chromium.org, commi...@chromium.org, drott+r...@chromium.org, alexis...@intel.com, apavlo...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, ko...@chromium.org, rob....@samsung.com
Reply all
Reply to author
Forward
0 new messages