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
: WebKit
CSS
: 528+ (Nightly build)
: PC All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-08-08 23:41 PST by
Modified: 2011-05-16 15:28 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-08-08 23:41:14 PST
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 From 2010-08-08 23:54:06 PST -------
Created an attachment (id=63863) [details]
Patch
------- Comment #2 From 2010-08-22 18:13:04 PST -------
Ping?
------- Comment #3 From 2010-08-23 10:39:53 PST -------
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 From 2010-08-31 20:06:35 PST -------
(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 #5 From 2010-09-10 01:06:23 PST -------
Created an attachment (id=67163) [details]
Patch 2
------- Comment #6 From 2010-09-10 01:08:06 PST -------
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.
------- Comment #7 From 2010-10-29 00:16:33 PST -------
Ping?
------- Comment #8 From 2010-11-21 22:31:05 PST -------
Ping again?
------- Comment #9 From 2010-12-02 13:46:17 PST -------
What is the difference between the new bit and m_allowStaleResources?
------- Comment #10 From 2010-12-03 13:07:18 PST -------
(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.
------- Comment #11 From 2010-12-06 03:06:39 PST -------
Created an attachment (id=75661) [details]
Use RAII
------- Comment #12 From 2010-12-06 03:07:18 PST -------
Thank you for the review. Introduced an RAII class.
------- Comment #13 From 2010-12-06 08:54:29 PST -------
The question from comment 9 still stands.
------- Comment #14 From 2010-12-06 18:15:02 PST -------
Created an attachment (id=75770) [details]
Renamed the cache validation suppression bit
------- Comment #15 From 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 From 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 From 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 From 2011-01-04 19:09:55 PST -------
Ping?
------- Comment #19 From 2011-01-05 06:47:00 PST -------
(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.
------- Comment #20 From 2011-01-05 20:11:52 PST -------
Created an attachment (id=78091) [details]
Use only m_allowStaleResources
------- Comment #21 From 2011-01-05 20:17:56 PST -------
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?
------- Comment #22 From 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 From 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 From 2011-01-06 10:28:23 PST -------
If it doesn't it would be interesting to see why.
------- Comment #25 From 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 From 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 From 2011-01-12 22:57:42 PST -------
Created an attachment (id=78779) [details]
Patch
------- Comment #28 From 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 From 2011-04-26 16:31:54 PST -------
(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.
------- Comment #30 From 2011-05-08 19:39:34 PST -------
Created an attachment (id=92756) [details]
Update and clean up
------- Comment #31 From 2011-05-08 23:33:50 PST -------
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 From 2011-05-08 23:35:39 PST -------
(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
------- Comment #33 From 2011-05-08 23:49:45 PST -------
Adam, can you take another look?
------- Comment #34 From 2011-05-16 11:03:33 PST -------
(From update of attachment 92756 [details])
r=me
------- Comment #35 From 2011-05-16 13:25:00 PST -------
(From update of attachment 92756 [details])
Clearing flags on attachment: 92756

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