Bug 43704 - Web font is printed as blank if it is not cached
: Web font is printed as blank if it is not cached
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: CSS
: 528+ (Nightly build)
: PC All
: P2 Normal
Assigned To: Yuzo Fujishima
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-08-08 23:41 PDT by Yuzo Fujishima
Modified: 2011-05-16 15:28 PDT (History)
11 users (show)

See Also:


Attachments
Patch (11.13 KB, patch)
2010-08-08 23:54 PDT, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Patch 2 (6.41 KB, patch)
2010-09-10 01:06 PDT, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Use RAII (6.44 KB, patch)
2010-12-06 03:06 PST, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Renamed the cache validation suppression bit (6.62 KB, patch)
2010-12-06 18:15 PST, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Use only m_allowStaleResources (8.49 KB, patch)
2011-01-05 20:11 PST, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Patch (8.78 KB, patch)
2011-01-12 22:57 PST, Yuzo Fujishima
abarth: review+
abarth: commit‑queue-
Details | Formatted Diff | Diff
Update and clean up (9.08 KB, patch)
2011-05-08 19:39 PDT, Yuzo Fujishima
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yuzo Fujishima 2010-08-08 23:41:14 PDT
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
Comment 1 Yuzo Fujishima 2010-08-08 23:54:06 PDT
Created attachment 63863 [details]
Patch
Comment 2 Yuzo Fujishima 2010-08-22 18:13:04 PDT
Ping?
Comment 3 Alexey Proskuryakov 2010-08-23 10:39:53 PDT
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.
Comment 4 Adam Barth 2010-08-31 20:06:35 PDT
Comment on attachment 63863 [details]
Patch

Yeah, I don't think the loader should know whether we're in the middle of printing.  That seems wrong.
Comment 5 Yuzo Fujishima 2010-09-10 01:06:23 PDT
Created attachment 67163 [details]
Patch 2
Comment 6 Yuzo Fujishima 2010-09-10 01:08:06 PDT
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])
> Yeah, I don't think the loader should know whether we're in the middle of printing.  That seems wrong.
Comment 7 Yuzo Fujishima 2010-10-29 00:16:33 PDT
Ping?
Comment 8 Yuzo Fujishima 2010-11-21 22:31:05 PST
Ping again?
Comment 9 Alexey Proskuryakov 2010-12-02 13:46:17 PST
What is the difference between the new bit and m_allowStaleResources?
Comment 10 Eric Seidel 2010-12-03 13:07:18 PST
Comment on attachment 67163 [details]
Patch 2

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.
Comment 11 Yuzo Fujishima 2010-12-06 03:06:39 PST
Created attachment 75661 [details]
Use RAII
Comment 12 Yuzo Fujishima 2010-12-06 03:07:18 PST
Thank you for the review. Introduced an RAII class.
Comment 13 Alexey Proskuryakov 2010-12-06 08:54:29 PST
The question from comment 9 still stands.
Comment 14 Yuzo Fujishima 2010-12-06 18:15:02 PST
Created attachment 75770 [details]
Renamed the cache validation suppression bit
Comment 15 WebKit Review Bot 2010-12-06 18:21:38 PST
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.
Comment 16 Yuzo Fujishima 2010-12-06 18:27:22 PST
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.
Comment 17 Alexey Proskuryakov 2010-12-06 20:40:50 PST
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.
Comment 18 Yuzo Fujishima 2011-01-04 19:09:55 PST
Ping?
Comment 19 Antti Koivisto 2011-01-05 06:47:00 PST
Comment on attachment 75770 [details]
Renamed the cache validation suppression bit

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.
Comment 20 Yuzo Fujishima 2011-01-05 20:11:52 PST
Created attachment 78091 [details]
Use only m_allowStaleResources
Comment 21 Yuzo Fujishima 2011-01-05 20:17:56 PST
Antti,

Thank you for the review.

(In reply to comment #19)
> (From update of attachment 75770 [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?
Comment 22 Alexey Proskuryakov 2011-01-05 20:56:34 PST
> 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.
Comment 23 Antti Koivisto 2011-01-06 10:27:46 PST
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;
Comment 24 Antti Koivisto 2011-01-06 10:28:23 PST
If it doesn't it would be interesting to see why.
Comment 25 Yuzo Fujishima 2011-01-06 18:34:42 PST
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.
Comment 26 Antti Koivisto 2011-01-07 02:57:35 PST
(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.
Comment 27 Yuzo Fujishima 2011-01-12 22:57:42 PST
Created attachment 78779 [details]
Patch
Comment 28 Yuzo Fujishima 2011-01-12 22:58:39 PST
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.
Comment 29 Adam Barth 2011-04-26 16:31:54 PDT
Comment on attachment 78779 [details]
Patch

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.
Comment 30 Yuzo Fujishima 2011-05-08 19:39:34 PDT
Created attachment 92756 [details]
Update and clean up
Comment 31 WebKit Commit Bot 2011-05-08 23:33:50 PDT
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: alice.liu@apple.com)
fast/dom/onerror-img.html bug 51019
The commit-queue is continuing to process your patch.
Comment 32 WebKit Commit Bot 2011-05-08 23:35:39 PDT
Comment on attachment 92756 [details]
Update and clean up

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
Comment 33 Yuzo Fujishima 2011-05-08 23:49:45 PDT
Adam, can you take another look?
Comment 34 Antti Koivisto 2011-05-16 11:03:33 PDT
Comment on attachment 92756 [details]
Update and clean up

r=me
Comment 35 WebKit Commit Bot 2011-05-16 13:25:00 PDT
Comment on attachment 92756 [details]
Update and clean up

Clearing flags on attachment: 92756

Committed r86601: <http://trac.webkit.org/changeset/86601>
Comment 36 WebKit Commit Bot 2011-05-16 13:25:07 PDT
All reviewed patches have been landed.  Closing bug.
Comment 37 WebKit Commit Bot 2011-05-16 15:28:31 PDT
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.