Bug 211680

Created attachment 398976 [details]
Test reduction

This looks like some fast path run measuring issue (either we incorrectly use the fast path for this font, or the measure-with-trailing-space codepath does not work well with this font)

<rdar://problem/63091370>

Created attachment 399035 [details]
Fixed space width

incorrect whitespace width is used in fast path.

Created attachment 399191 [details]
Patch

I should talk to Myles to figure out how to create a font with no whitespace glyph in it.

(In reply to zalan from comment #5)
> I should talk to Myles to figure out how to create a font with no whitespace
> glyph in it.

https://github.com/litherum/AhemMaker

(In reply to Myles C. Maxfield from comment #6)
> (In reply to zalan from comment #5)
> > I should talk to Myles to figure out how to create a font with no whitespace
> > glyph in it.
> 
> https://github.com/litherum/AhemMaker
v.cool. thanks.

I am a bit confused about these failures. I can repro them locally even with a one-liner. All I have to do is access the primary font.

diff --git a/Source/WebCore/rendering/RenderText.cpp b/Source/WebCore/rendering/RenderText.cpp
index 1ccf96724e4..4369341f9b4 100644
--- a/Source/WebCore/rendering/RenderText.cpp
+++ b/Source/WebCore/rendering/RenderText.cpp
@@ -1240,6 +1240,8 @@ void RenderText::secureText(UChar maskingCharacter)
 
 bool RenderText::computeCanUseSimplifiedTextMeasuring() const
 {
+    style().fontCascade().primaryFont();
+
     if (!m_canUseSimpleFontCodePath)
         return false;

*** Bug 211679 has been marked as a duplicate of this bug. ***

(In reply to zalan from comment #8)
> I am a bit confused about these failures. I can repro them locally even with
> a one-liner. All I have to do is access the primary font.
> 
> diff --git a/Source/WebCore/rendering/RenderText.cpp
> b/Source/WebCore/rendering/RenderText.cpp
> index 1ccf96724e4..4369341f9b4 100644
> --- a/Source/WebCore/rendering/RenderText.cpp
> +++ b/Source/WebCore/rendering/RenderText.cpp
> @@ -1240,6 +1240,8 @@ void RenderText::secureText(UChar maskingCharacter)
>  
>  bool RenderText::computeCanUseSimplifiedTextMeasuring() const
>  {
> +    style().fontCascade().primaryFont();
> +
>      if (!m_canUseSimpleFontCodePath)
>          return false;

Without the modification,

DYLD_FRAMEWORK_PATH=/Users/mmaxfield/Build/Products/Debug  --pixel  >   0.33s user 0.16s system 89% cpu 0.548 total

With the modification,

DYLD_FRAMEWORK_PATH=/Users/mmaxfield/Build/Products/Debug  --pixel  >   0.34s user 0.15s system 31% cpu 1.563 total

With the modification, running the test takes almost exactly 1 second longer than running it without your modification. Also, LayoutTests/http/tests/webfont/slow-ahem-loading.cgi has:

sleep(1);

This indicates that starting the download delays DOMContentLoaded, thereby delaying the time the test runner takes the snapshot until after all the webfonts are loaded, and the test realizes that none of them supports U+306E, so it finds some other font to render the character (visibly). But if the download isn't started, then DOMContentLoaded fires before the layout that causes the second font to start downloading, and the snapshot is taken during font loading, and therefore the text is invisible.

So, it appears the snapshot is waiting for the second font to finish loading

 11 @font-face {
 12     font-family: "WebFont";
 13     src: url("../resources/Ahem.woff");
 14 }
 15 
 16 @font-face {
 17     font-family: "WebFont2";
 18     src: url("slow-ahem-loading.cgi");
 19 }
...
 23 <div style="font: 48px 'WebFont', 'WebFont2', 'Helvetica';">&#x306E;</div>

trunk:
127.0.0.1 - - [01/Aug/2021:08:51:21 -0700] "GET /webfont/loading/font-loading-system-fallback-visibility.html HTTP/1.1" 200 668 767
127.0.0.1 - - [01/Aug/2021:08:51:21 -0700] "GET /resources/Ahem.woff HTTP/1.1" 200 2972 334
127.0.0.1 - - [01/Aug/2021:08:51:21 -0700] "GET /webfont/loading/font-loading-system-fallback-visibility-expected.html HTTP/1.1" 200 406 166

with early accessing the primary font 
127.0.0.1 - - [01/Aug/2021:08:53:02 -0700] "GET /webfont/loading/font-loading-system-fallback-visibility.html HTTP/1.1" 200 668 527
127.0.0.1 - - [01/Aug/2021:08:53:02 -0700] "GET /resources/Ahem.woff HTTP/1.1" 200 2972 189
127.0.0.1 - - [01/Aug/2021:08:53:02 -0700] "GET /webfont/loading/slow-ahem-loading.cgi HTTP/1.1" 200 - 1014800
127.0.0.1 - - [01/Aug/2021:08:53:03 -0700] "GET /webfont/loading/font-loading-system-fallback-visibility-expected.html HTTP/1.1" 200 406 200

Apparently we don't even start loading the slow-font on trunk.

while running DumpRenderTree:
- DOMContentLoaded event is dispatched (parsing is done, DOM is built)
- window load event is dispatched (js, css are finished loading)
- dispatchDidFinishLoad is called (this is when all the loads are complete, including web fonts -Safari turns cancel button to reload)
^^ This is when DumpRenderTree declares the test complete and takes the snapshot (paint).

Now the early font access (while building the render tree) triggers the font load which in turn delays the dispatchDidFinishLoad callback which also means the snapshot is taken at a later time -in this case when the font load is complete.

Without the early font access, the dispatchDidFinishLoad happens at the same time as the window load event (no pending load, since we haven't yet initiated the font load). As dispatchDidFinishLoad triggers the snapshot (which initiates the layout-paint sequence), we try to load the font, but at this point the test is already declared complete so we are not going to wait for it.

Right. So my previous comment was mostly right, but got the event wrong.

Ideally we would be using a standard event to end layout tests. But modifying the test infrastructure to do that would probably be a task that's bigger than the one already at hand.

*** This bug has been marked as a duplicate of bug 228617 ***