Bug 86700 – Showing error message when CachedFont::ensureCustomFontData() fails

• Component: Text
• Status: RESOLVED INVALID

[Kenichi Ishibashi]

If CachedFont::ensureCustomFontData() fails, WebKit simply ignore the webfont and use fallback fonts without any error messages. This behavior often confuses page authors and end users. I think it would be nice to show an error message when it fails. Chromium uses font sanitizer for webfonts to prevent activating malicious fonts, so showing such kind of error message would help page authors to find problems if they use invalid fonts.

What do you think? I'll upload a proposed patch.

[Kenichi Ishibashi]

Created attachment 142405 [details]
proposed patch

[Alexey Proskuryakov]

Not commenting on the idea, I think that the message could use some improvement. First, it should say "cannot", not "failed". And it would be helpful to at least tell which font could not be activated.

[Kenichi Ishibashi]

Sorry for late response (I was absent last two weeks).

(In reply to comment #2)
> Not commenting on the idea, I think that the message could use some improvement. First, it should say "cannot", not "failed".

Will do.

> And it would be helpful to at least tell which font could not be activated.

CSSFontFaceSource::m_string contains the URI of the font and it is passed to addConsoleMessage() so we can see which font could not be activated.

[Kenichi Ishibashi]

Created attachment 145533 [details]
Patch

[WebKit Review Bot]

Comment on attachment 145533 [details]
Patch

Attachment 145533 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12899201

New failing tests:
fast/images/style-access-during-imageChanged-crash.html
fast/text/custom-font-data-crash2.html
fast/text/custom-font-data-crash.html
fast/loader/font-face-empty.html
fast/loader/goto-anchor-infinite-layout.html
http/tests/css/font-face-src-loading.html
fast/css/font-face-data-uri-invalid.html

[WebKit Review Bot]

Created attachment 145559 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Alexis Menard (darktears)]

Comment on attachment 145533 [details]
Patch

As the EWS pointed out this will create lot of failures.

[Kenichi Ishibashi]

Created attachment 145676 [details]
Rebaselined affected test expectations

[Alexis Menard (darktears)]

Comment on attachment 145676 [details]
Rebaselined affected test expectations

This is completely wrong. What is the point of this debug output? Why you need that? I don't think we want to pollute the console with this especially in release.  Also it is really wrong to update the expected files. Your Changelog fails to explain why you do this.

[Alexis Menard (darktears)]

Comment on attachment 145676 [details]
Rebaselined affected test expectations

This is completely wrong. What is the point of this debug output? Why you need that? I don't think we want to pollute the console with this especially in release.  Also it is really wrong to update the expected files. Your Changelog fails to explain why you do this.

[Kenichi Ishibashi]

(In reply to comment #9)
> (From update of attachment 145676 [details])
> This is completely wrong. What is the point of this debug output? Why you need that? I don't think we want to pollute the console with this especially in release.  Also it is really wrong to update the expected files. Your Changelog fails to explain why you do this.

The message isn't a debug message. See the description of the motivation. From web page authors and users point of view, failing activating webfonts almost the same as failing loading webfonts, and we show a error message when we fail to load webfonts. Firefox shows an error message if it fails to activate webfont. Attaching a screenshot. I checked all rebaselined tests try to load invalid fonts or URI.

[Kenichi Ishibashi]

Created attachment 145681 [details]
Screenshot of Firefox

[Alexis Menard (darktears)]

(In reply to comment #11)
> (In reply to comment #9)
> > (From update of attachment 145676 [details] [details])
> > This is completely wrong. What is the point of this debug output? Why you need that? I don't think we want to pollute the console with this especially in release.  Also it is really wrong to update the expected files. Your Changelog fails to explain why you do this.
> 
> The message isn't a debug message. See the description of the motivation. From web page authors and users point of view, failing activating webfonts almost the same as failing loading webfonts, and we show a error message when we fail to load webfonts. Firefox shows an error message if it fails to activate webfont. Attaching a screenshot. I checked all rebaselined tests try to load invalid fonts or URI.

Nevermind then I went directly through the review mode, you should definitively put that in the Changelog (sometimes people don't read the bug history) otherwise It's hard to understand what you are trying to achieve (and also important for the history inside the repo). Still the debug output is not as complete as FF it seems.

> CSSFontFaceSource::m_string contains the URI of the font and it is passed to addConsoleMessage() so we can see which font could not be activated.

Well according to the modification of the expected files it doesn't work as I can't see which font couldn't be loaded but I maybe missing something here.

I'm still confused on why you need to modify the expected files of the tests, read why these tests fails to load the web font (is that even intended on all tests??)?. Do you guarantee that they pass on other port than Chromium, like the console message will be printed. I will test on Qt tomorrow. I'm not a font expert, I just stumbled across your patch so maybe someone with a better knowledge can review that.

[Kenichi Ishibashi]

(In reply to comment #13)
> Nevermind then I went directly through the review mode, you should definitively put that in the Changelog (sometimes people don't read the bug history) otherwise It's hard to understand what you are trying to achieve (and also important for the history inside the repo). Still the debug output is not as complete as FF it seems.

You are right. I'll explain the intent of the patch on the ChangeLog. To make the error message more precise, we have to modify platform-dependent code like FontCustomPlatformData, that sounds over-engineering to me for this purpose.

> I'm still confused on why you need to modify the expected files of the tests, read why these tests fails to load the web font (is that even intended on all tests??)?. Do you guarantee that they pass on other port than Chromium, like the console message will be printed. I will test on Qt tomorrow. I'm not a font expert, I just stumbled across your patch so maybe someone with a better knowledge can review that.

Most of these tests check whether WebKit doesn't crash when @font-face contains invalid URIs. http/tests/css/font-face-src-loading.html loads an empty webfont, and the purpose of the test is independent of whether the font is activated or not. I don't have other port environment, so I would like to see EWS results.

[Kenichi Ishibashi]

Created attachment 145686 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 145686 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145686&action=review

> Source/WebCore/css/CSSFontFaceSource.cpp:167
> +                    fontSelector->document()->addConsoleMessage(NetworkMessageSource, LogMessageType, ErrorMessageLevel, "Cannot activate the font.", m_string);

I've never seen the use of the word "activate" in this context. Perhaps something like "Cannot use the font" might be more user friendly?

> Source/WebCore/css/CSSFontFaceSource.cpp:172
>                                                                                     fontDescription.textOrientation(), fontDescription.widthVariant(), fontDescription.renderingMode()), true, false));

I see a really bad indentation here :(

[Kenichi Ishibashi]

Created attachment 157575 [details]
Patch

[Kenichi Ishibashi]

Comment on attachment 145686 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145686&action=review

Thank you for review!

>> Source/WebCore/css/CSSFontFaceSource.cpp:167
>> +                    fontSelector->document()->addConsoleMessage(NetworkMessageSource, LogMessageType, ErrorMessageLevel, "Cannot activate the font.", m_string);
> 
> I've never seen the use of the word "activate" in this context. Perhaps something like "Cannot use the font" might be more user friendly?

Done.

>> Source/WebCore/css/CSSFontFaceSource.cpp:172
>>                                                                                     fontDescription.textOrientation(), fontDescription.widthVariant(), fontDescription.renderingMode()), true, false));
> 
> I see a really bad indentation here :(

I just concatenated the lines.

[WebKit Review Bot]

Comment on attachment 157575 [details]
Patch

Attachment 157575 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13476020

New failing tests:
http/tests/css/link-css-disabled-value-with-slow-loading-sheet-in-error.html

[WebKit Review Bot]

Created attachment 157587 [details]
Archive of layout-test-results from gce-cr-linux-06

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Darin Adler]

Comment on attachment 157575 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157575&action=review

> Source/WebCore/css/CSSFontFaceSource.cpp:168
> +                        fontSelector->document()->addConsoleMessage(NetworkMessageSource, LogMessageType, ErrorMessageLevel, "Cannot use the font.", m_string);

Seems like a good idea, but generally seems like too vague a message. This would be much better with a message that cites the URL for the font.