<rdar://problem/9835028> Font loading during layout can cause layout code to be re...
authormitz@apple.com <mitz@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 26 Jul 2011 03:21:52 +0000 (03:21 +0000)
committermitz@apple.com <mitz@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 26 Jul 2011 03:21:52 +0000 (03:21 +0000)
https://bugs.webkit.org/show_bug.cgi?id=65123

Source/WebCore:

Reviewed by Anders Carlsson and Darin Adler.

Since CSSFontFaceSource::getFontData() can get called during layout, avoid calling out to loader
code from under it, and instead defer that work using a zero-delay timer.

* css/CSSFontFaceSource.cpp:
(WebCore::CSSFontFaceSource::CSSFontFaceSource):
(WebCore::CSSFontFaceSource::~CSSFontFaceSource):
(WebCore::CSSFontFaceSource::getFontData): Rather than starting the font load here, schedule
a zero-delay timer to do it.
(WebCore::CSSFontFaceSource::startLoadingTimerFired): Added. Starts loading the font if needed.
* css/CSSFontFaceSource.h:

LayoutTests:

Reviewed by Darin Adler.

Updated tests that depended on fonts loading synchronously during layout.
Unfortunately, font loading does not fire any DOM events, so in most cases
a constant timeout had to be introduced.

* fast/blockflow/broken-ideograph-small-caps.html:
* fast/css/color-leakage.html:
* fast/css/custom-font-xheight.html:
* fast/css/font-face-multiple-faces.html:
* fast/css/font-face-multiple-remote-sources.html:
* fast/css/font-face-remote.html:
* fast/css/font-face-woff.html:
* svg/W3C-SVG-1.1-SE/text-intro-09-b.svg:
* svg/W3C-SVG-1.1/fonts-elem-07-b.svg:
* svg/custom/svg-fonts-fallback.xhtml:
* svg/custom/svg-fonts-in-html.html:
* svg/custom/svg-fonts-segmented.xhtml:
* svg/custom/svg-fonts-with-no-element-reference.html:
* svg/custom/svg-fonts-without-missing-glyph.xhtml:
* svg/text/text-overflow-ellipsis-svgfont.html:

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@91738 268f45cc-cd09-0410-ab3c-d52691b4dbfc

19 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/blockflow/broken-ideograph-small-caps.html
LayoutTests/fast/css/color-leakage.html
LayoutTests/fast/css/custom-font-xheight.html
LayoutTests/fast/css/font-face-multiple-faces.html
LayoutTests/fast/css/font-face-multiple-remote-sources.html
LayoutTests/fast/css/font-face-remote.html
LayoutTests/fast/css/font-face-woff.html
LayoutTests/svg/W3C-SVG-1.1-SE/text-intro-09-b.svg
LayoutTests/svg/W3C-SVG-1.1/fonts-elem-07-b.svg
LayoutTests/svg/custom/svg-fonts-fallback.xhtml
LayoutTests/svg/custom/svg-fonts-in-html.html
LayoutTests/svg/custom/svg-fonts-segmented.xhtml
LayoutTests/svg/custom/svg-fonts-with-no-element-reference.html
LayoutTests/svg/custom/svg-fonts-without-missing-glyph.xhtml
LayoutTests/svg/text/text-overflow-ellipsis-svgfont.html
Source/WebCore/ChangeLog
Source/WebCore/css/CSSFontFaceSource.cpp
Source/WebCore/css/CSSFontFaceSource.h

index 2cc99f7..c638bf3 100644 (file)
@@ -1,3 +1,30 @@
+2011-07-25  Dan Bernstein  <mitz@apple.com>
+
+        <rdar://problem/9835028> Font loading during layout can cause layout code to be re-entered via resource load delegate
+        https://bugs.webkit.org/show_bug.cgi?id=65123
+
+        Reviewed by Darin Adler.
+
+        Updated tests that depended on fonts loading synchronously during layout.
+        Unfortunately, font loading does not fire any DOM events, so in most cases
+        a constant timeout had to be introduced.
+
+        * fast/blockflow/broken-ideograph-small-caps.html:
+        * fast/css/color-leakage.html:
+        * fast/css/custom-font-xheight.html:
+        * fast/css/font-face-multiple-faces.html:
+        * fast/css/font-face-multiple-remote-sources.html:
+        * fast/css/font-face-remote.html:
+        * fast/css/font-face-woff.html:
+        * svg/W3C-SVG-1.1-SE/text-intro-09-b.svg:
+        * svg/W3C-SVG-1.1/fonts-elem-07-b.svg:
+        * svg/custom/svg-fonts-fallback.xhtml:
+        * svg/custom/svg-fonts-in-html.html:
+        * svg/custom/svg-fonts-segmented.xhtml:
+        * svg/custom/svg-fonts-with-no-element-reference.html:
+        * svg/custom/svg-fonts-without-missing-glyph.xhtml:
+        * svg/text/text-overflow-ellipsis-svgfont.html:
+
 2011-07-25  Adrienne Walker  <enne@google.com>
 
         [chromium] Mark fast/cyclic-prototypes.html as a flaky crasher.
index 547ffb8..cb0641d 100644 (file)
@@ -57,10 +57,6 @@ p {
 
 </style>
   
-<script type="text/javascript">
-
-</script>
-  
 </head>
 <body>
 
@@ -69,5 +65,12 @@ p {
 <div class="basic d1 vert"><p>第一段落 Paragraph 1</p><p>第二段落 Paragraph 2</p></div>
 </div>
 
+<script>
+    if (window.layoutTestController) {
+        layoutTestController.waitUntilDone();
+        document.body.offsetTop;
+        setTimeout(function() { layoutTestController.notifyDone(); }, 100);
+    }
+</script>
 </body>
-</html>
\ No newline at end of file
+</html>
index 0e591d1..2264262 100644 (file)
@@ -2,10 +2,6 @@
   <head>
   <!-- The Ahem font rendered in this test should be displayed black, not blue. -->
     <style>
-      @font-face {
-        font-family: 'Ahem';
-        src: url('resources/Ahem.ttf');
-      }
       div {
         display: block;
         font-family: Ahem;
index a967dbc..0552c90 100644 (file)
@@ -37,17 +37,25 @@ WebKit nightly adding insane height to div at random.</p>
 </div>
 
 <script>
-if (window.layoutTestController)
+if (window.layoutTestController) {
     layoutTestController.dumpAsText();
+    layoutTestController.waitUntilDone();
+}
 
 function test()
 {
-    var totalHeight = document.defaultView.getComputedStyle(document.getElementById("test"), null).getPropertyCSSValue("height");
-    totalHeight = totalHeight.getFloatValue(CSSPrimitiveValue.CSS_PX);
-    if (totalHeight > 150 && totalHeight < 300)
-        document.getElementById("result").innerHTML = "PASS";
-    else
-        document.getElementById("result").innerHTML = "FAIL: " + totalHeight + "px";
+    document.body.offsetTop;
+    setTimeout(function() {
+        var totalHeight = document.defaultView.getComputedStyle(document.getElementById("test"), null).getPropertyCSSValue("height");
+        totalHeight = totalHeight.getFloatValue(CSSPrimitiveValue.CSS_PX);
+        if (totalHeight > 150 && totalHeight < 300)
+            document.getElementById("result").innerHTML = "PASS";
+        else
+            document.getElementById("result").innerHTML = "FAIL: " + totalHeight + "px";
+
+        if (window.layoutTestController)
+            layoutTestController.notifyDone();
+    }, 100);
 }
 </script>
 </body>
index 5e1c531..c202386 100644 (file)
 <div style="font-family: webkit-four;">
     AHEM <i>AHEM</i>
 </div>
+<script>
+    if (window.layoutTestController) {
+        layoutTestController.waitUntilDone();
+        document.body.offsetTop;
+        setTimeout(function() { layoutTestController.notifyDone(); }, 100);
+    }
+</script>
index c678f55..0183da1 100644 (file)
         _fail
     </div>
     <script>
-        document.body.offsetTop;
+        if (window.layoutTestController) {
+            layoutTestController.waitUntilDone();
+            document.body.offsetTop;
+            setTimeout(function() { layoutTestController.notifyDone(); }, 100);
+        }
     </script>
 </body>
index 1d5807d..b0d891b 100644 (file)
@@ -8,7 +8,7 @@
         div { width: 100px; height: 100px; background-color: red; font-family: 'remote'; font-size: 20px; color: green; }
     </style>
 </head>
-<body onload="finished()">
+<body>
     <div>
         FAIL_<br>
         XXXXX<br>
         _FAIL<br>
     </div>
     <script>
-        if (window.layoutTestController)
+        if (window.layoutTestController) {
             layoutTestController.waitUntilDone();
-
-        // Kick off loading of the font
-        document.body.offsetTop;
-
-        function finished()
-        {
-            if (window.layoutTestController)
-                layoutTestController.notifyDone();
+            document.body.offsetTop;
+            setTimeout(function() { layoutTestController.notifyDone(); }, 100);
         }
     </script>
 </body>
index abdf527..e0be808 100644 (file)
@@ -8,3 +8,10 @@
 <p>This test tries to render the following text with Ahem, loaded from a WOFF file. The text below should be a series of black boxes.</p>
 
 <p style="font-family: family1; font-size: 4em;">Failure</p>
+<script>
+    if (window.layoutTestController) {
+        layoutTestController.waitUntilDone();
+        document.body.offsetTop;
+        setTimeout(function() { layoutTestController.notifyDone(); }, 100);
+    }
+</script>
index d122102..5ec28d8 100644 (file)
     <text font-family="SVGFreeSansASCII,sans-serif" font-weight="bold" font-size="20" x="240"
       text-anchor="middle" y="18" stroke-width="0.5" stroke="black" fill="white">DRAFT</text>
   </g>-->
+<script>
+    if (window.layoutTestController) {
+        layoutTestController.waitUntilDone();
+        document.documentElement.offsetTop;
+        setTimeout(function() { layoutTestController.notifyDone(); }, 100);
+    }
+</script>
 </svg>
index f3f996e..085bd19 100644 (file)
        </g>
        <text id="revision" x="10" y="340" font-size="40" stroke="none" fill="black">$Revision: 1.7 $</text>
        <rect id="test-frame" x="1" y="1" width="478" height="358" fill="none" stroke="#000000"/>
+        <script>
+            if (window.layoutTestController) {
+                layoutTestController.waitUntilDone();
+                document.documentElement.offsetTop;
+                setTimeout(function() { layoutTestController.notifyDone(); }, 100);
+            }
+        </script>
 </svg>
index cd9f8c4..2718aa8 100644 (file)
     <!-- 'a', ' ', 'o' from TestFont2, '&#xbe2;' can't be rendered, as none of the default fonts defines it -->
     <span style='font-family: TestFont2; font-size: 40px; '>a &#xbe2; o</span><br/>
 </p>
-
+<script>
+    if (window.layoutTestController) {
+        layoutTestController.waitUntilDone();
+        document.documentElement.offsetTop;
+        setTimeout(function() { layoutTestController.notifyDone(); }, 100);
+    }
+</script>
 </body>
 </html>
index 13c01fa..74705a6 100644 (file)
 <!-- Add a background image to each and use width and height to control sizing, place with absolute positioning -->\r
 <div id="extraDiv1"><span></span></div><div id="extraDiv2"><span></span></div><div id="extraDiv3"><span></span></div>\r
 <div id="extraDiv4"><span></span></div><div id="extraDiv5"><span></span></div><div id="extraDiv6"><span></span></div>\r
-\r
+<script>\r
+    if (window.layoutTestController) {\r
+        layoutTestController.waitUntilDone();\r
+        document.documentElement.offsetTop;\r
+        setTimeout(function() { layoutTestController.notifyDone(); }, 100);\r
+    }\r
+</script>\r
 </body>\r
 </html>\r
index 4288964..7d8244f 100644 (file)
 
 <!-- 'ABC' should be rendered using Times, 'def' using Courier, 'o' using the SVG Font, and 'O' again by Times -->
 <p style='font-family: TestFont; font-size: 40px; '>ABCdefoooO</p>
-
+<script>
+    if (window.layoutTestController) {
+        layoutTestController.waitUntilDone();
+        document.documentElement.offsetTop;
+        setTimeout(function() { layoutTestController.notifyDone(); }, 100);
+    }
+</script>
 </body>
 </html>
index 9aa0303..8ea400a 100644 (file)
 <body>\r
 <p class="first">This text should be rendered with a first font.</p>\r
 <p class="second">This text should be rendered with a second font.</p>\r
+<script>\r
+    if (window.layoutTestController) {\r
+        layoutTestController.waitUntilDone();\r
+        document.documentElement.offsetTop;\r
+        setTimeout(function() { layoutTestController.notifyDone(); }, 100);\r
+    }\r
+</script>\r
 </body>\r
 \r
 </html>\r
index b7cc4fc..b304f0f 100644 (file)
 <p class="target">AXX</p>
 <p class="target">XXX</p>
 <p class="target">AAA</p>
-
+<script>
+    if (window.layoutTestController) {
+        layoutTestController.waitUntilDone();
+        document.documentElement.offsetTop;
+        setTimeout(function() { layoutTestController.notifyDone(); }, 100);
+    }
+</script>
 </body>
 </html>
index a141441..1cc58ab 100644 (file)
@@ -36,5 +36,12 @@ Pass if some text is shown followed by an ellipsis (NOT just the ellipsis by its
        abc abc abc abc abc abc abc abc abc abc
 </div>
 
+<script>
+    if (window.layoutTestController) {
+        layoutTestController.waitUntilDone();
+        document.documentElement.offsetTop;
+        setTimeout(function() { layoutTestController.notifyDone(); }, 100);
+    }
+</script>
 </body>
 </html>
index 57f664e..80b620f 100644 (file)
@@ -1,3 +1,21 @@
+2011-07-25  Dan Bernstein  <mitz@apple.com>
+
+        <rdar://problem/9835028> Font loading during layout can cause layout code to be re-entered via resource load delegate
+        https://bugs.webkit.org/show_bug.cgi?id=65123
+
+        Reviewed by Anders Carlsson and Darin Adler.
+
+        Since CSSFontFaceSource::getFontData() can get called during layout, avoid calling out to loader
+        code from under it, and instead defer that work using a zero-delay timer.
+
+        * css/CSSFontFaceSource.cpp:
+        (WebCore::CSSFontFaceSource::CSSFontFaceSource):
+        (WebCore::CSSFontFaceSource::~CSSFontFaceSource):
+        (WebCore::CSSFontFaceSource::getFontData): Rather than starting the font load here, schedule
+        a zero-delay timer to do it.
+        (WebCore::CSSFontFaceSource::startLoadingTimerFired): Added. Starts loading the font if needed.
+        * css/CSSFontFaceSource.h:
+
 2011-07-25  Al Patrick  <apatrick@chromium.org>
 
         Removed support for the GL_latch_CHROMIUM extension which Chromium no longer supports.
index 4a851ac..ab29a3e 100644 (file)
@@ -50,6 +50,7 @@ CSSFontFaceSource::CSSFontFaceSource(const String& str, CachedFont* font)
     : m_string(str)
     , m_font(font)
     , m_face(0)
+    , m_loadStartTimer(this, &CSSFontFaceSource::startLoadingTimerFired)
 #if ENABLE(SVG_FONTS)
     , m_hasExternalSVGFont(false)
 #endif
@@ -60,6 +61,7 @@ CSSFontFaceSource::CSSFontFaceSource(const String& str, CachedFont* font)
 
 CSSFontFaceSource::~CSSFontFaceSource()
 {
+    m_loadStartTimer.stop();
     if (m_font)
         m_font->removeClient(this);
     pruneTable();
@@ -172,9 +174,12 @@ SimpleFontData* CSSFontFaceSource::getFontData(const FontDescription& fontDescri
 #endif
         }
     } else {
-        // Kick off the load now.
-        if (CachedResourceLoader* cachedResourceLoader = fontSelector->cachedResourceLoader())
-            m_font->beginLoadIfNeeded(cachedResourceLoader);
+        // Kick off the load now. Do it on a zero-delay timer rather than synchronously, because we may be in
+        // the middle of layout, and the loader may invoke arbitrary delegate or event handler code.
+        m_fontSelector = fontSelector;
+        if (!m_loadStartTimer.isActive())
+            m_loadStartTimer.startOneShot(0);
+
         // FIXME: m_string is a URL so it makes no sense to pass it as a family name.
         SimpleFontData* tempData = fontCache()->getCachedFontData(fontDescription, m_string);
         if (!tempData)
@@ -189,6 +194,17 @@ SimpleFontData* CSSFontFaceSource::getFontData(const FontDescription& fontDescri
     return fontDataRawPtr;
 }
 
+void CSSFontFaceSource::startLoadingTimerFired(Timer<WebCore::CSSFontFaceSource>*)
+{
+    ASSERT(m_font);
+    ASSERT(m_fontSelector);
+
+    if (CachedResourceLoader* cachedResourceLoader = m_fontSelector->cachedResourceLoader())
+        m_font->beginLoadIfNeeded(cachedResourceLoader);
+
+    m_fontSelector = nullptr;
+}
+
 #if ENABLE(SVG_FONTS)
 SVGFontFaceElement* CSSFontFaceSource::svgFontFaceElement() const
 {
index e87fbfc..9831bcd 100644 (file)
@@ -28,6 +28,7 @@
 
 #include "CachedResourceClient.h"
 #include "CachedResourceHandle.h"
+#include "Timer.h"
 #include <wtf/HashMap.h>
 #include <wtf/text/AtomicString.h>
 
@@ -70,11 +71,16 @@ public:
 #endif
 
 private:
+    void startLoadingTimerFired(Timer<CSSFontFaceSource>*);
+
     AtomicString m_string; // URI for remote, built-in font name for local.
     CachedResourceHandle<CachedFont> m_font; // For remote fonts, a pointer to our cached resource.
     CSSFontFace* m_face; // Our owning font face.
     HashMap<unsigned, SimpleFontData*> m_fontDataTable; // The hash key is composed of size synthetic styles.
 
+    Timer<CSSFontFaceSource> m_startLoadingTimer;
+    RefPtr<CSSFontSelector> m_fontSelector;
+
 #if ENABLE(SVG_FONTS)
     RefPtr<SVGFontFaceElement> m_svgFontFaceElement;
     RefPtr<SVGFontElement> m_externalSVGFontElement;