Bug 115131 – REGRESSION (r146544): Once a custom font is cached to disk, it starts failing to render until the page is refreshed.

• Component: WebKit2
• Status: RESOLVED FIXED

[Brady Eidson]

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>

[Brady Eidson]

Created attachment 199521 [details]
Patch v1 - Don't replace encoded data for a CachedFont that has ever created a FontCustomPlatformData object

[Alexey Proskuryakov]

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

[Brady Eidson]

(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!

[Brady Eidson]

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!

[Brady Eidson]

(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.

[Brady Eidson]

(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

[Brady Eidson]

But on that note, I think it should probably be pure virtual so you're forced to consider it.

[Alexey Proskuryakov]

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/ :-\

[Brady Eidson]

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.