Closed Bug 1030067 Opened 7 years ago Closed 7 years ago

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

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: jfkthame, Assigned: jfkthame)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
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.
Attachment #8445804 - Flags: review?(roc) → review-
(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?
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.
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?
Attachment #8446392 - Flags: review?(bzbarsky)
Attachment #8445804 - Attachment is obsolete: true
Blocks: 1030829
(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.
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?
Flags: needinfo?(jfkthame)
It's not the wrong thing, I'm just ignorant of it. :) Updated the patch to use this, thanks.
Attachment #8447002 - Flags: review?(bzbarsky)
Attachment #8446392 - Attachment is obsolete: true
Attachment #8446392 - Flags: review?(bzbarsky)
Flags: needinfo?(jfkthame)
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.
Attachment #8447002 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/aeb88c96da92
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.