1030067 - improve userfont caching behavior for data-URI fonts embedded in CSS

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Jonathan Kew [:jfkthame]]

For user fonts that are embedded "inline" in CSS using data: URIs, there's no reason to record the principal as part of the cache key, or to check it when doing a lookup. Any caller who has the URI has already successfully loaded the relevant data to use the font.

This should improve subsequent page-loads for apps or sites where a number of pages share the same CSS that uses "inline" data-URI fonts.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: ignore the principal when caching data-URI fonts, to allow sharing across pages with the same CSS.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8445804 [details] [diff] [review]
ignore the principal when caching data-URI fonts, to allow sharing across pages with the same CSS.

Review of attachment 8445804 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxUserFontSet.cpp
@@ +884,5 @@
>          return false;
>      }
>  
> +    // for fonts loaded from data: URIs, we don't care about the principal
> +    if (NS_FAILED(mURI->SchemeIs("data", &equal)) || !equal) {

We should not do explicit scheme checks here.

I think here we should call nsIProtocolHandler::GetProtocolFlags and check for URI_IS_LOCAL_RESOURCE. I think that's safe since lookups in the cache are guarded by gfxUserFontSet::CheckFontLoad.

Rest of the patch looks good to me but I think you should get review from bz since Necko protocol flag checks are tricky.

Comment 3

[Jonathan Kew [:jfkthame]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)

> I think here we should call nsIProtocolHandler::GetProtocolFlags and check
> for URI_IS_LOCAL_RESOURCE. I think that's safe since lookups in the cache
> are guarded by gfxUserFontSet::CheckFontLoad.

That will apply to fonts loaded from local files as well as data: URIs; are we OK with that? Where do cross-origin checks happen?

Comment 4

[Jonathan Kew [:jfkthame]]

Hmm, yes, as I suspected: this allows a cached font to be used for a local file: URL, even when cross-origin restrictions (target file is not in the document's directory or a descendant) ought to prevent it being loaded.

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: ignore the principal when caching data-URI fonts, to allow sharing across pages with the same CSS.

For clarity, factored out the scheme check into a single helper function. In view of comment 4, I did -not- replace this with checking protocol flags as suggested in comment 2. Is there a better way to do this, or do we need to stay with the explicit scheme check here?

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Jonathan Kew (:jfkthame) from comment #4)
> Hmm, yes, as I suspected: this allows a cached font to be used for a local
> file: URL, even when cross-origin restrictions (target file is not in the
> document's directory or a descendant) ought to prevent it being loaded.

I thought nsUserFontSet::CheckFontLoad would deny the local file load before we do the cache lookup.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Hmm.  CheckFontLoad() won't prevent file:// loads, since CheckLoadURI allows any file:// to load any other file://.  That says nothing about their origin.

Why is URI_INHERITS_SECURITY_CONTEXT the wrong thing to use here, though?

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: ignore the principal when caching data-URI fonts, to allow sharing across pages with the same CSS.

It's not the wrong thing, I'm just ignorant of it. :) Updated the patch to use this, thanks.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8447002 [details] [diff] [review]
ignore the principal when caching data-URI fonts, to allow sharing across pages with the same CSS.

s/NULL/nullptr/ in the comments, and looks great.

Comment 10

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aeb88c96da92

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/aeb88c96da92