Web font is printed as blank if it is not cached

If a web font is configured not to be cached (e.g., by not setting Expires header),
the part of the page to be rendered in that web font is printed as blank.

For example, the following page is printed as blank if cgi-bin/getfont.py is not cached.

<style> 
span { 
    font-size: 64px; 
} 
@font-face { 
    font-family:myfont; 
    src: url(cgi-bin/getfont.py); 
} 
</style> 
<span style="font-size:64px; font-family:myfont"> 
This will not be printed on paper in the first try to print. 
</span>

This is related to https://bugs.webkit.org/show_bug.cgi?id=43551
Currently, WebKit validates cache when a subresource is loaded for the first
time and the second. Cache is not validated for the third and later.
If we change WebKit such that cache validation is skipped also for the second load,
this printing bug will be gone.

Note:
This is the main cause of http://code.google.com/p/chromium/issues/detail?id=42499

Created an attachment (id=63863) [details]
Patch

Ping?

I think that solution to these problems is in CSS code, not in loader code. But since I don't know CSS code much, I may well be wrong.

(From update of attachment 63863 [details])
Yeah, I don't think the loader should know whether we're in the middle of printing.  That seems wrong.

Created an attachment (id=67163) [details]
Patch 2

Hi, Alexey, Adam,

Thank you for the reviews. Can you take another look?
I've written a new patch based on your comments.

(In reply to comment #4)
> (From update of attachment 63863 [details] [details])
> Yeah, I don't think the loader should know whether we're in the middle of printing.  That seems wrong.

Ping?

Ping again?

What is the difference between the new bit and m_allowStaleResources?

(From update of attachment 67163 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=67163&action=review

> WebKit/mac/WebView/WebHTMLView.mm:3886
> +            document->cachedResourceLoader()->validateCachedDocumentResources(false);

We may want to make a RAII class to do this false/true setting automatically.  Just makes it impossible to not pair them correctly and get into a bad state.

Created an attachment (id=75661) [details]
Use RAII

Thank you for the review. Introduced an RAII class.

The question from comment 9 still stands.

Created an attachment (id=75770) [details]
Renamed the cache validation suppression bit

Attachment 75770 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'HEAD']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'HEAD'.


If any of these errors are false positives, please file a bug against check-webkit-style.

Sorry for not responding sooner.

m_allowStaleResources suppresses cache validation entirely, i.e., across documents.

m_allowStaleResourcesWithinDocument (renamed from m_validateCachedDocumentResources) suppresses cache validation within the document.

If a web font has been downloaded for a document for continuous media (screen),
it should not be cache-validated and reloaded for paged media (print).

But if the web font has been downloaded for another document,
cache validation should happen.

Example 1:

- MyDocument uses FontW for both continuous and paged media.

In printing MyDocument, FontW cached for continuous media should be used without validating the cache.

Example 2:

- MyDocument uses FontC for continuous media and FontP, for paged.
- AnotherDocument uses FontP.

In printing MyDocument, FontP cached for AnotherDocument must not be used without first validating the cache. (Cache validation won't happen if we only use m_allowStaleResources.) 


(In reply to comment #13)
> The question from comment 9 still stands.

Thank you.

CC'ing Antti, who is likely the best reviewer for this patch. I can't say that I'm fully convinced that this is a loader issue, but you may be right in treating is as such.

Technically, it's not great to have two booleans with partially overlapping meaning - an enum with three values could be a better fit.

Ping?

(From update of attachment 75770 [details])
Why does it try to reload resources when switching to printing? 

Why can't you use the existing m_allowStaleResources? A separate m_allowStaleResourcesWithinDocument doesn't make that much sense as CachedResourceLoader is per-document anyway.

You need to make a new patch, the code you are modifying has changed substantially, r- for now.

Created an attachment (id=78091) [details]
Use only m_allowStaleResources

Antti,

Thank you for the review.

(In reply to comment #19)
> (From update of attachment 75770 [details] [details])
> Why does it try to reload resources when switching to printing?

It is because style sheets are evaluated again in switching to printing.
If web fonts are used in the sheets, they are reloaded.

> 
> Why can't you use the existing m_allowStaleResources? A separate m_allowStaleResourcesWithinDocument doesn't make that much sense as CachedResourceLoader is per-document anyway.

I misunderstood the behavior. Changed to use m_allowStaleResources.

> 
> You need to make a new patch, the code you are modifying has changed substantially, r- for now.

Updated the patch. Can you take another look?

> Why does it try to reload resources when switching to printing? 

That's the same question that I had; see also bug 27971. But I don't know if this can actually be fixed in CSS code.

Are you sure this bug actually occurs in current TOT?

In principle this test in CachedResourceLoader::determineRevalidationPolicy() should prevent reloading:

    // Avoid loading the same resource multiple times for a single document, even if the cache policies would tell us to.
    if (m_validatedURLs.contains(existingResource->url()))
        return Use;

If it doesn't it would be interesting to see why.

Hi, Antti,

You are right.
r74807 seems to have fixed the loading behavior and the printing issue is now gone.

I marked my patch as obsolete and close this bug.

(In reply to comment #24)
> If it doesn't it would be interesting to see why.

(In reply to comment #25)
> Hi, Antti,
> 
> You are right.
> r74807 seems to have fixed the loading behavior and the printing issue is now gone.
> 
> I marked my patch as obsolete and close this bug.
> 
> (In reply to comment #24)
> > If it doesn't it would be interesting to see why.

Actually we should be reloading the resource in case of Cache-control: no-store (but are not due to bug 52044) in which case your patch is still needed.

The patch looks good to me. It would be better if your RAII type would save and restore the existing state so it could be safely used in nested context.

Created an attachment (id=78779) [details]
Patch

Hi, Antti,

Changed to save and use previous state. Can you take another look?

(In reply to comment #26)
> (In reply to comment #25)
> > Hi, Antti,
> > 
> > You are right.
> > r74807 seems to have fixed the loading behavior and the printing issue is now gone.
> > 
> > I marked my patch as obsolete and close this bug.
> > 
> > (In reply to comment #24)
> > > If it doesn't it would be interesting to see why.
> 
> Actually we should be reloading the resource in case of Cache-control: no-store (but are not due to bug 52044) in which case your patch is still needed.
> 
> The patch looks good to me. It would be better if your RAII type would save and restore the existing state so it could be safely used in nested context.

(From update of attachment 78779 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=78779&action=review

I'm sad about this patch not having tests, but it seems like the consensus is that we should accept this patch.

> Source/WebCore/loader/cache/CachedResourceLoader.h:146
> +    ResourceCacheValidationSuppressor(CachedResourceLoader* loader) : m_loader(loader), m_previousState(false)

The initializers should be on their own lines.

> Source/WebCore/loader/cache/CachedResourceLoader.h:148
> +        if (m_loader) {

Shouldn't we ASSERT that m_loader is non-NULL?  What situation would we get into where we wanted to use a null loader?

> Source/WebCore/page/Frame.cpp:506
> +    // In setting printing, we should not validate resources already cached for the document.
> +    // See https://bugs.webkit.org/show_bug.cgi?id=43704

Having a test would be better than having this comment.

Created an attachment (id=92756) [details]
Update and clean up

The commit-queue encountered the following flaky tests while processing attachment 92756 [details]:

http/tests/misc/favicon-loads-with-icon-loading-override.html bug 58412 (author: [e-mail])
fast/dom/onerror-img.html bug 51019
The commit-queue is continuing to process your patch.

(From update of attachment 92756 [details])
Rejecting attachment 92756 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'la..." exit_code: 1

Last 500 characters of output:
=43704&ctype=xml
Processing 1 patch from 1 bug.
Cleaning working directory
Updating working directory
Processing patch 92756 from bug 43704.
NOBODY (OOPS!) found in /Projects/CommitQueue/Source/WebCore/ChangeLog does not appear to be a valid reviewer according to committers.py.
ERROR: /Projects/CommitQueue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
Updating OpenSource
Current branch new_master is up to date.

Full output: http://queues.webkit.org/results/8640426

Adam, can you take another look?

(From update of attachment 92756 [details])
r=me

(From update of attachment 92756 [details])
Clearing flags on attachment: 92756

Committed r86601: <http://trac.webkit.org/changeset/86601>

All reviewed patches have been landed.  Closing bug.

The commit-queue encountered the following flaky tests while processing attachment 92756 [details]:

http/tests/appcache/origin-usage.html bug 60928
The commit-queue is continuing to process your patch.