- Revision
- 201394
- Author
- [email protected]
- Date
- 2016-05-25 12:08:02 -0700 (Wed, 25 May 2016)
Log Message
[Font Loading] ASSERT if calling FontFace.loaded twice with a garbage collection between them
https://bugs.webkit.org/show_bug.cgi?id=158015
Reviewed by Darin Adler.
Source/WebCore:
The following scenario may occur:
1. We create a FontFace object
2. We create an associated JSFontFace object
3. We start loading the FontFace, which causes an extra ref to hang around until loading finishes
4. _javascript_ calls the "loaded" attribute on the FontFace, which saves a promise inside the FontFace
5. The FontFace goes out of scope in _javascript_
6. A garbage collection occurs, causing us to delete the JSFontFace object
7. _javascript_ then encounters the FontFace object without first going through a reference to a JSFontFace.
It can do this via iterating through a FontFaceSet. We respond to this situation by creating a new
JSFontFace object and associating it with the existing FontFace.
8. _javascript_ calls the "loaded" attribute
In this situation, the newer JSFontFace object is out of sync with the older FontFace object. In
particular, the FontFace has a saved promise, but the JSFontFace doesn't know about it. Therefore,
the JSFontFace should be flexible to the presence of this member.
Test: fast/text/font-face-crash-2.html
* bindings/js/JSDOMPromise.h:
(WebCore::DOMPromise::deferredWrapper):
* bindings/js/JSFontFaceCustom.cpp:
(WebCore::JSFontFace::loaded):
* css/FontFace.h:
LayoutTests:
* fast/text/font-face-crash-2-expected.txt: Added.
* fast/text/font-face-crash-2.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (201393 => 201394)
--- trunk/LayoutTests/ChangeLog 2016-05-25 19:04:47 UTC (rev 201393)
+++ trunk/LayoutTests/ChangeLog 2016-05-25 19:08:02 UTC (rev 201394)
@@ -1,3 +1,13 @@
+2016-05-25 Myles C. Maxfield <[email protected]>
+
+ [Font Loading] ASSERT if calling FontFace.loaded twice with a garbage collection between them
+ https://bugs.webkit.org/show_bug.cgi?id=158015
+
+ Reviewed by Darin Adler.
+
+ * fast/text/font-face-crash-2-expected.txt: Added.
+ * fast/text/font-face-crash-2.html: Added.
+
2016-05-25 Antti Koivisto <[email protected]>
Shadow DOM: RenderTreePosition should determine if element has display:contents from new style
Added: trunk/LayoutTests/fast/text/font-face-crash-2-expected.txt (0 => 201394)
--- trunk/LayoutTests/fast/text/font-face-crash-2-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/text/font-face-crash-2-expected.txt 2016-05-25 19:08:02 UTC (rev 201394)
@@ -0,0 +1 @@
+This test makes sure that if we construct two JSFontFaces for the same FontFace, and call "loaded" on both of them, that there is no ASSERT.
Property changes on: trunk/LayoutTests/fast/text/font-face-crash-2-expected.txt
___________________________________________________________________
Added: svn:keywords
Added: svn:eol-style
Added: trunk/LayoutTests/fast/text/font-face-crash-2.html (0 => 201394)
--- trunk/LayoutTests/fast/text/font-face-crash-2.html (rev 0)
+++ trunk/LayoutTests/fast/text/font-face-crash-2.html 2016-05-25 19:08:02 UTC (rev 201394)
@@ -0,0 +1,24 @@
+<!DOCTYPE html>
+<html>
+<head>
+</head>
+<body>
+<script>
+if (window.testRunner)
+ testRunner.dumpAsText();
+(function() {
+ var font = new FontFace("WebFont", "url('notarealfont')", {});
+ font.loaded;
+ font.load();
+ document.fonts.add(font);
+})();
+if (window.GCController)
+ GCController.collect();
+document.fonts.forEach(function(font) {
+ if (font.family == "WebFont")
+ font.loaded;
+});
+</script>
+This test makes sure that if we construct two JSFontFaces for the same FontFace, and call "loaded" on both of them, that there is no ASSERT.
+</body>
+</html>
\ No newline at end of file
Modified: trunk/Source/WebCore/ChangeLog (201393 => 201394)
--- trunk/Source/WebCore/ChangeLog 2016-05-25 19:04:47 UTC (rev 201393)
+++ trunk/Source/WebCore/ChangeLog 2016-05-25 19:08:02 UTC (rev 201394)
@@ -1,3 +1,35 @@
+2016-05-25 Myles C. Maxfield <[email protected]>
+
+ [Font Loading] ASSERT if calling FontFace.loaded twice with a garbage collection between them
+ https://bugs.webkit.org/show_bug.cgi?id=158015
+
+ Reviewed by Darin Adler.
+
+ The following scenario may occur:
+
+ 1. We create a FontFace object
+ 2. We create an associated JSFontFace object
+ 3. We start loading the FontFace, which causes an extra ref to hang around until loading finishes
+ 4. _javascript_ calls the "loaded" attribute on the FontFace, which saves a promise inside the FontFace
+ 5. The FontFace goes out of scope in _javascript_
+ 6. A garbage collection occurs, causing us to delete the JSFontFace object
+ 7. _javascript_ then encounters the FontFace object without first going through a reference to a JSFontFace.
+ It can do this via iterating through a FontFaceSet. We respond to this situation by creating a new
+ JSFontFace object and associating it with the existing FontFace.
+ 8. _javascript_ calls the "loaded" attribute
+
+ In this situation, the newer JSFontFace object is out of sync with the older FontFace object. In
+ particular, the FontFace has a saved promise, but the JSFontFace doesn't know about it. Therefore,
+ the JSFontFace should be flexible to the presence of this member.
+
+ Test: fast/text/font-face-crash-2.html
+
+ * bindings/js/JSDOMPromise.h:
+ (WebCore::DOMPromise::deferredWrapper):
+ * bindings/js/JSFontFaceCustom.cpp:
+ (WebCore::JSFontFace::loaded):
+ * css/FontFace.h:
+
2016-05-25 Antti Koivisto <[email protected]>
Shadow DOM: RenderTreePosition miscomputed when display:contents value changes
Modified: trunk/Source/WebCore/bindings/js/JSDOMPromise.h (201393 => 201394)
--- trunk/Source/WebCore/bindings/js/JSDOMPromise.h 2016-05-25 19:04:47 UTC (rev 201393)
+++ trunk/Source/WebCore/bindings/js/JSDOMPromise.h 2016-05-25 19:08:02 UTC (rev 201394)
@@ -165,6 +165,8 @@
template<typename... ErrorType> void reject(ErrorType&&... error) { m_wrapper.reject(std::forward<ErrorType>(error)...); }
+ DeferredWrapper& deferredWrapper() { return m_wrapper; }
+
private:
DeferredWrapper m_wrapper;
};
Modified: trunk/Source/WebCore/bindings/js/JSFontFaceCustom.cpp (201393 => 201394)
--- trunk/Source/WebCore/bindings/js/JSFontFaceCustom.cpp 2016-05-25 19:04:47 UTC (rev 201393)
+++ trunk/Source/WebCore/bindings/js/JSFontFaceCustom.cpp 2016-05-25 19:08:02 UTC (rev 201394)
@@ -38,9 +38,12 @@
JSC::JSValue JSFontFace::loaded(JSC::ExecState& state) const
{
if (!m_loaded) {
- DeferredWrapper promise(&state, globalObject(), JSC::JSPromiseDeferred::create(&state, globalObject()));
- m_loaded.set(state.vm(), this, promise.promise());
- wrapped().registerLoaded(WTFMove(promise));
+ if (!wrapped().promise()) {
+ DeferredWrapper promise(&state, globalObject(), JSC::JSPromiseDeferred::create(&state, globalObject()));
+ m_loaded.set(state.vm(), this, promise.promise());
+ wrapped().registerLoaded(WTFMove(promise));
+ } else
+ m_loaded.set(state.vm(), this, wrapped().promise().value().deferredWrapper().promise());
}
return m_loaded.get();
}
Modified: trunk/Source/WebCore/css/FontFace.h (201393 => 201394)
--- trunk/Source/WebCore/css/FontFace.h 2016-05-25 19:04:47 UTC (rev 201393)
+++ trunk/Source/WebCore/css/FontFace.h 2016-05-25 19:08:02 UTC (rev 201394)
@@ -66,6 +66,7 @@
LoadStatus status() const;
typedef DOMPromise<FontFace&> Promise;
+ Optional<Promise>& promise() { return m_promise; }
void registerLoaded(Promise&&);
void load();
