Bug 73419 – WebFonts are re-fetched from the server upon Document::styleSelectorChanged call.

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Kenichi Ishibashi]

See http://crbug.com/103510 for details.

This problem could happen when the response header of a webfont contains 'Cache-control: no-cache'. When style recalculation occurs, styleSelector is recreated. During styleSelector recreation, the webfont is re-validated via CSSFontSelector::addFontFaceRule(). As the result, the webfont is reloaded. WebKit shouldn't re-validate the cache in this case.

I think we can avoid re-validation by holding a CachedFont handle in CSSFontFaceSrcValue object, like CSSImageValue does.

[Kenichi Ishibashi]

Created attachment 117157 [details]
Patch

[Kenichi Ishibashi]

(In reply to comment #1)
> Created an attachment (id=117157) [details]
> Patch

No tests so far. How can we test this?

[Kenichi Ishibashi]

Hi mitz,

Could you please take a look?

[Alexey Proskuryakov]

See also: bug 27971, bug 43704.

[Kenichi Ishibashi]

Created attachment 117843 [details]
Patch

[Alexey Proskuryakov]

Does this also fix bug 27971?

[Kenichi Ishibashi]

Moritta suggested me in person that resource should be acquired in CSSFontFaceSrcValue. Could you take a look? (bdakin?)

(In reply to comment #6)
> Does this also fix bug 27971?

Thanks for letting me know the bug. I couldn't reproduce the problem with my Safari, but I think this patch can fix bug 27971 if the corresponding document object is not discarded.

[Kenichi Ishibashi]

Ping?

[Kenichi Ishibashi]

(In reply to comment #7)
> (In reply to comment #6)
> > Does this also fix bug 27971?
> 
> Thanks for letting me know the bug. I couldn't reproduce the problem with my Safari, but I think this patch can fix bug 27971 if the corresponding document object is not discarded.

As far as I checked with WebInspector (on Chromium Linux build), WebKit doesn't try to reload the fonts when 'Back' is pressed. I also confirmed that the fonts were reloaded when 'Reload' is pressed in accordance with the response header.

[Julien Chaffraix]

> As far as I checked with WebInspector (on Chromium Linux build), WebKit doesn't try to reload the fonts when 'Back' is pressed. I also confirmed that the fonts were reloaded when 'Reload' is pressed in accordance with the response header.

It sounds like you could test your change if you use a PHP script that remembers that it was called (look at LayoutTests/http/tests/xmlhttprequest/resources/access-control-basic-preflight-cache-invalidation.php for an example) and use a location.reload(). If that's not possible, your ChangeLog should mention why you could not land a test case.

[Alexey Proskuryakov]

> As far as I checked with WebInspector (on Chromium Linux build), WebKit doesn't try to reload the fonts when 'Back' is pressed. I also confirmed that the fonts were reloaded when 'Reload' is pressed in accordance with the response header.

This is not what I'm seeing in Safari with ToT WebKit. The fonts are reloaded on Back. The fact that the test passes appears to mean that it no longer functions properly (perhaps a workaround was added server side).

[Kenichi Ishibashi]

(In reply to comment #10)

> It sounds like you could test your change if you use a PHP script that remembers that it was called (look at LayoutTests/http/tests/xmlhttprequest/resources/access-control-basic-preflight-cache-invalidation.php for an example) and use a location.reload(). If that's not possible, your ChangeLog should mention why you could not land a test case.

Thank you for the information. I'll try it (page navigation behavior looks different between ports as I mention below, though).

> This is not what I'm seeing in Safari with ToT WebKit. The fonts are reloaded on Back. The fact that the test passes appears to mean that it no longer functions properly (perhaps a workaround was added server side).

You are right. I confirmed the behavior in Safari with ToT WebKit. And my patch doesn't solve the problem.  It seems the cause of bug 27971 is different from this bug.

[Hajime Morrita]

As jchaffraix@ pointed, having a test case makes this change more solid.
Using some server is good for end-to-end perspective.
Another idea is to add some Internals API to purge caches,
then use it in the test.

[Kenichi Ishibashi]

Created attachment 118758 [details]
Patch

[Kenichi Ishibashi]

(In reply to comment #13)
> As jchaffraix@ pointed, having a test case makes this change more solid.
> Using some server is good for end-to-end perspective.
> Another idea is to add some Internals API to purge caches,
> then use it in the test.

Thank you for the suggestion. LayoutTests/http/tests/xmlhttprequest/resources/access-control-basic-preflight-cache-invalidation.php is an useful reference and I wrote a test case (The test uses ugly setTimeout() to check whether font re-fetching has happened, though).

[Kenichi Ishibashi]

Could someone review the patch?

[Alexey Proskuryakov]

One question someone asked me is why we need a new reference to CachedFont, given that document already has a CachedResource for it.

[Kenichi Ishibashi]

(In reply to comment #17)
> One question someone asked me is why we need a new reference to CachedFont, given that document already has a CachedResource for it.

"document already has a CachedResource for it" means we can get the CachedFont by calling document->cachedResourceLoader()->requestFont()? If so, the problem is that requestFont() re-validates the cache, as I described in the bug description. Holding a reference to CachedFont avoids it, as the same manner as CSSImageValue does.

[Kenichi Ishibashi]

Created attachment 119804 [details]
Patch

[Kenichi Ishibashi]

(In reply to comment #19)
> Created an attachment (id=119804) [details]
> Patch

Just rebased to ToT.

[Eric Seidel (no email)]

I know we've had issues before with cache sharing between documents.  I'm not sure if that is still a risk or not.

Comment on attachment 119804 [details]
Patch

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

> Source/WebCore/css/CSSFontFaceSrcValue.cpp:29
> +#include "CachedFont.h"

I’m surprised that this header is needed.

[Kenichi Ishibashi]

Comment on attachment 119804 [details]
Patch

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

Thank you so much for review.

>> Source/WebCore/css/CSSFontFaceSrcValue.cpp:29
>> +#include "CachedFont.h"
> 
> I’m surprised that this header is needed.

"m_cachedFont = document->cachedResourceLoader()->requestFont(request);" requires the header.

[WebKit Review Bot]

Comment on attachment 119804 [details]
Patch

Clearing flags on attachment: 119804

Committed r104542: <http://trac.webkit.org/changeset/104542>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.