REGRESSION (r146544): Once a custom font is cached to disk, it starts failing to render until the page is refreshed. One example of a WebFont user in the wild is http://www.okemo.com/activities/snowsports/ticketing/deals/onlinedeals.asp We can't do the CachedResource::tryReplaceEncodedData trick for CachedFonts on CG platforms because the CGFont object is directly reliant on the original encoded data from the original SharedBuffer. This patch will introduce the concept of a CachedResource saying it doesn't support being swapped out. In radar as <rdar://problem/13622998>
Created attachment 199521 [details] Patch v1 - Don't replace encoded data for a CachedFont that has ever created a FontCustomPlatformData object
Comment on attachment 199521 [details] Patch v1 - Don't replace encoded data for a CachedFont that has ever created a FontCustomPlatformData object View in context: https://bugs.webkit.org/attachment.cgi?id=199521&action=review > Source/WebCore/loader/cache/CachedFont.h:67 > + virtual bool shouldTryReplaceEncodedData() const; I think that it's a "may", not "should". > Source/WebCore/loader/cache/CachedResource.h:299 > virtual PurgePriority purgePriority() const { return PurgeDefault; } > + virtual bool shouldTryReplaceEncodedData() const { return true; } OVERRIDE
(In reply to comment #2) > (From update of attachment 199521 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=199521&action=review > > > Source/WebCore/loader/cache/CachedFont.h:67 > > + virtual bool shouldTryReplaceEncodedData() const; > > I think that it's a "may", not "should". Fair enough. > > > Source/WebCore/loader/cache/CachedResource.h:299 > > virtual PurgePriority purgePriority() const { return PurgeDefault; } > > + virtual bool shouldTryReplaceEncodedData() const { return true; } > > OVERRIDE Duh. Thanks!
http://trac.webkit.org/changeset/149070
Comment on attachment 199521 [details] Patch v1 - Don't replace encoded data for a CachedFont that has ever created a FontCustomPlatformData object View in context: https://bugs.webkit.org/attachment.cgi?id=199521&action=review >>> Source/WebCore/loader/cache/CachedResource.h:299 >>> + virtual bool shouldTryReplaceEncodedData() const { return true; } >> >> OVERRIDE > > Duh. > > Thanks! I’m surprised that the default is true!
(In reply to comment #5) > (From update of attachment 199521 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=199521&action=review > > >>> Source/WebCore/loader/cache/CachedResource.h:299 > >>> + virtual bool shouldTryReplaceEncodedData() const { return true; } > >> > >> OVERRIDE > > > > Duh. > > > > Thanks! > > I’m surprised that the default is true! Why? Only certain CachedFonts are known to misbehave with the replacement. All other CachedResources still get it.
(In reply to comment #6) > (In reply to comment #5) > > (From update of attachment 199521 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=199521&action=review > > > > >>> Source/WebCore/loader/cache/CachedResource.h:299 > > >>> + virtual bool shouldTryReplaceEncodedData() const { return true; } > > >> > > >> OVERRIDE > > > > > > Duh. > > > > > > Thanks! > > > > I’m surprised that the default is true! > > Why? Only certain CachedFonts are known to misbehave with the replacement. All other CachedResources still get it. Because if you’re writing a new CachedResource derived class, you should inherit safe behavior by default, and make a conscious choice to opt into unsafe behavior if you know that you can handle it.
(In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #5) > > > (From update of attachment 199521 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=199521&action=review > > > > > > >>> Source/WebCore/loader/cache/CachedResource.h:299 > > > >>> + virtual bool shouldTryReplaceEncodedData() const { return true; } > > > >> > > > > > > I’m surprised that the default is true! > > > > Why? Only certain CachedFonts are known to misbehave with the replacement. All other CachedResources still get it. > > Because if you’re writing a new CachedResource derived class, you should inherit safe behavior by default, and make a conscious choice to opt into unsafe behavior if you know that you can handle it. Oh, you and your good points. :P
But on that note, I think it should probably be pure virtual so you're forced to consider it.
I would note that "safe" behavior is the one that doesn't kill performance. From this point of view, the current default is actually safe.
(In reply to comment #10) > I would note that "safe" behavior is the one that doesn't kill performance. From this point of view, the current default is actually safe. When I used the word “safe”, I didn’t mean “doesn’t kill performance”. Maybe there’s another word for “doesn’t kill performance”, or maybe there should be, but the word “safe” already means something different, as in the sentence “when shouldTryReplaceEncodedData() returns false, it is not safe to hang on to the pointer to the encoded data beyond the end of the current iteration of the run loop”.
(In reply to comment #11) > (In reply to comment #10) > > I would note that "safe" behavior is the one that doesn't kill performance. From this point of view, the current default is actually safe. > > When I used the word “safe”, I didn’t mean “doesn’t kill performance”. Maybe there’s another word for “doesn’t kill performance”, or maybe there should be, but the word “safe” already means something different, as in the sentence “when shouldTryReplaceEncodedData() returns false, it is not safe to hang on to the pointer to the encoded data beyond the end of the current iteration of the run loop”. s/false/true/ :-\
This whole mechanism is a memory optimization for process-per-tab mode. Not doing it does not "kill performance", and *all* resources will still get the file backed memory on the next load after it was cached.