Closed Bug 1376964 Opened 7 years ago Closed 7 years ago

stylo: Avoid the additional restyle for user font if the font can be loaded synchronously (e.g. from cache)

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: xidorn, Assigned: heycam)

References

Details

Attachments

(10 files)

Bug 1376964 - Part 1: Record the docshell's "bypass cache" flag on FontFaceSet.
59 bytes, text/x-review-board-request
jfkthame
: review+
Details
Bug 1376964 - Part 2: Record the docshell's "private browsing" flag on FontFaceSet.
59 bytes, text/x-review-board-request
jfkthame
: review+
Details
Bug 1376964 - Part 3: Add a generation counter to the user font cache.
59 bytes, text/x-review-board-request
jfkthame
: review+
Details
Bug 1376964 - Part 4: Call FontLoadAllowed ahead of time and cache the results for style worker threads.
59 bytes, text/x-review-board-request
jfkthame
: review+
Details
Bug 1376964 - Part 5: Add OMT wrapper for nsIURIs useful for font stuff.
59 bytes, text/x-review-board-request
jfkthame
: review+
Details
Bug 1376964 - Part 6: Use gfxFontSrcURI in the user font set and cache.
59 bytes, text/x-review-board-request
jfkthame
: review+
Details
Bug 1376964 - Part 7: Remove unused nsIURI argument from gfxPlatform::IsFontFormatSupported.
59 bytes, text/x-review-board-request
jfkthame
: review+
Details
Bug 1376964 - Part 8: Add OMT wrapper for nsIPrincipals useful for font stuff.
59 bytes, text/x-review-board-request
jfkthame
: review+
Details
Bug 1376964 - Part 9: Use gfxFontSrcPrincipal in the user font set and cache.
59 bytes, text/x-review-board-request
jfkthame
: review+
Details
Bug 1376964 - Part 10: Allow style worker threads to pick fonts out of the user font cache.
59 bytes, text/x-review-board-request
jfkthame
: review+
Details 
In Gecko, we have a global cache of loaded user font faces, from which font data can be loaded synchronously when needed.

However, in Stylo, we currently enqueue a post-traversal task for loading user font faces even if they can be loaded from cache, which can trigger an unnecessary restyle. It is unfortunate.

We should allow Stylo to try doing the synchronous load if possible, and only use the post-traversal task if we really need async load.

The current blocker is that, in FontFaceSet::CheckFontLoad which is called in gfxUserFontEntry::DoLoadNextSrc before we can load font from cache, we need to get the DocShell from its owner document to know whether we need to bypass cache in the current loading (for Ctrl-Shift-R presumably). And DocShell is referenced via a weak pointer in document. WeakPtr is not thread-safe, thus it is not allowed to be dereferenced in other threads.

To fix this, we probably want to either store the bypass cache flag somewhere else that we can get without querying the DocShell, or make WeakPtr thread-safe...
We can probably just store the bit in FontFaceSet... which seems to have some free slots.
OK, so there are several other things apart from bypass cache check:
* there is another (same) weak pointer involves in private browsing checking;
* FontFaceSet::IsFontLoadAllowed needs to check principal with nsIScriptSecurityManager, but the later is not thread-safe.

Even if we can workaround nsIScriptSecurityManager (which is only used for IsSystemPrincipal check so probably not a big deal), I suppose we would have similar problem with nsIContentPolicy at just several lines below that.

And all these things not being thread-safe probably means code below is written with single thread in mind, so we may have more trouble when we want to use them in parallel traversal.

So this bug may be non-trivial to fix actually.
I think I had the same train of thought when originally adding the font metrics/loading support, finding that for fonts coming out of the cache, there was too much to change with principal checking and CORS checking than I was comfortable with.
I'm concerned that this could be a performance regression if we restyle it twice.

I wonder if it is possible that we do some optimistic guess, that is, we unconditionally accept a font from cache in parallel traversal, but verify it in post-traversal, and if any verification failed, we force another restyle. This is complicated, and could still regress performance in some cases, but should be a reasonable compromise if that isn't detectable...
One thing we might be able to do is when we create the sequential task for restyling when we can't grab the font from the user font cache, that we just post a restyle for that element, instead of the entire document.
It's probably worth noting that, this kind of regression may not be caught by tp5o tests at all, because as far as I can see, there are very few sites in that test set use web fonts. A grep of '@font-face' in the set only shows three results. This may be because those tests are generated from early days when web fonts haven't been widely adopted.
Turns out we're not correctly restyling when we encounter this case of the metrics not being in the font metrics cache, but the font is in the user font cache, and we use the PostTraversalTask to pick it out of the cache and validate it once we're back on the main thread.

When the font isn't in the user font cache, and the first request for the font is due to needing its metrics for ch/ex units, then we will kick off the font load on the main thread, and eventually we'll be in nsFontFaceLoader::OnStreamComplete where we call nsPresContext::UserFontSetUpdated, which will do the appropriate restyling.

When the font is in the user font cache, we don't use the nsFontFaceLoader.  The generation counter increment we do at the end of ContinueLoad will cause us to get correct metrics if we did later restyle, but nothing causes us to restyle.


nsPresContext::UserFontSetUpdated's use of nsFontFaceUtils::MarkDirtyForFontChange was the optimization I was thinking of and mentioned during the meeting this week, although it looks as if it's just used for the reflow, not the restyling when ch/ex units were used.  Since stylo currently doesn't cache data on rule nodes, we could get rid of that PostRebuildAllStyleDataEvent call and update MarkDirtyForFontChange to post eRestyle_ForceDescendants restyles on the elements using the font.
For the call to IsFontLoadAllowed in gfxUserFontSet::UserFontCache::GetFont, which is the one that calls into security/principal stuff, I wonder if we could just go through all of the entries in the cache that match the principal that FontFaceSet::CheckFontLoad returns, and call IsFontLoadAllowed on them ahead of time and record the results somewhere?  Or do this every time an entry is added to the UserFontCache.
Jonathan, out of interest, do we use persistent user font cache entries and their CRC32-based lookups (added in bug 1030829) any more now that B2G is gone?  If not, would it be OK to remove that?  It might simplify this bug if I can remove the second half of gfxUserFontSet::UserFontCache::GetFont.
Flags: needinfo?(jfkthame)
(In reply to Cameron McCormack (:heycam) from comment #9)
> Jonathan, out of interest, do we use persistent user font cache entries and
> their CRC32-based lookups (added in bug 1030829) any more now that B2G is
> gone?  If not, would it be OK to remove that?

I don't believe we do. So yes, it looks like we could rip some more code out of gfxUserFontSet (and gfxFT2FontList).
Flags: needinfo?(jfkthame)
Great, I'll look at doing that, thanks.

(In reply to Cameron McCormack (:heycam) from comment #8)
> For the call to IsFontLoadAllowed in gfxUserFontSet::UserFontCache::GetFont,
> which is the one that calls into security/principal stuff, I wonder if we
> could just go through all of the entries in the cache that match the
> principal that FontFaceSet::CheckFontLoad returns, and call
> IsFontLoadAllowed on them ahead of time and record the results somewhere? 

So I think doing this will work.  We can store a table on each UserFontCache::Entry with the specific gfxUserFontSets that are allowed to use that entry.  In ServoStyleSet::PreTraverseSync, we can go over each entry in the cache, use the entry's mPrincipal in the call to IsFontLoadAllowed, then store the result in the table.

One wrinkle is that for font face URLs that came from UA or user sheets, we use the sheet's principal rather than the loading document's principal, and for data: URLs, we just store null in the entry's mPrincipal.  We can't easily know ahead of time what that sheet's principal will be.  Maybe it's OK to continue the delayed font load for data: URLs that are specified in UA or user sheets.  (It should be quite rare.)
(In reply to Cameron McCormack (:heycam) from comment #11)
> Great, I'll look at doing that, thanks.

I have a couple of patches that I tried locally to clean this up; if you like I can attach them to a dependent bug.
Depends on: 1378718
(In reply to Cameron McCormack (:heycam) from comment #11)
> One wrinkle is that for font face URLs that came from UA or user sheets, we
> use the sheet's principal rather than the loading document's principal, and
> for data: URLs, we just store null in the entry's mPrincipal.  We can't
> easily know ahead of time what that sheet's principal will be.

Well, we could iterate over all of the gfxFontFaceSrc objects in the gfxUserFontSet for those with mUseOriginPrincipal, and call IsFontLoadAllowed with their mOriginPrincipals, and store them along side the "normal" IsFontLoadAllowed results.  But we would need to make sure we update whenever the available @font-faces (or DOM FontFace objects) changes.  So I'm not sure it's worth it.
I have some patches which might need a bit more testing before review.  We still can't support synchronous loading of data: URLs (since the network stuff that FontFaceSet::SyncLoadFontData doesn't work OMT), though I could probably do something to use nsDataHandler::ParseURI directly or something.
Comment on attachment 8884516 [details]
Bug 1376964 - Part 1: Record the docshell's "bypass cache" flag on FontFaceSet.

https://reviewboard.mozilla.org/r/155404/#review160696

::: layout/style/FontFaceSet.cpp:131
(Diff revision 2)
> +    if (NS_SUCCEEDED(docShell->GetLoadType(&loadType))) {
> +      if ((loadType >> 16) & nsIWebNavigation::LOAD_FLAGS_BYPASS_CACHE) {
> +        mBypassCache = true;
> +      }
> +    }
> +    uint32_t flags;
> +    if (NS_SUCCEEDED(docShell->GetDefaultLoadFlags(&flags))) {
> +      if (flags & nsIRequest::LOAD_BYPASS_CACHE) {
> +        mBypassCache = true;
> +      }
> +    }

I realize you've just moved the existing logic here, but while we're touching these lines, how about combining the conditions into a single

    if ((NS_SUCCEEDED(...) && (...)) ||
        (NS_SUCCEEDED(...) && (...)))

so that if the first condition would set the flag, we don't bother testing the second?
Attachment #8884516 - Flags: review?(jfkthame) → review+
Comment on attachment 8884517 [details]
Bug 1376964 - Part 2: Record the docshell's "private browsing" flag on FontFaceSet.

https://reviewboard.mozilla.org/r/155406/#review160698
Attachment #8884517 - Flags: review?(jfkthame) → review+
Comment on attachment 8884518 [details]
Bug 1376964 - Part 3: Add a generation counter to the user font cache.

https://reviewboard.mozilla.org/r/155408/#review160704
Attachment #8884518 - Flags: review?(jfkthame) → review+
Comment on attachment 8884519 [details]
Bug 1376964 - Part 4: Call FontLoadAllowed ahead of time and cache the results for style worker threads.

https://reviewboard.mozilla.org/r/155410/#review160706

I'm not thrilled about the extra hashtables and lookups being added here, as hash lookups seem to have a way of showing up in profiles, but I guess this is probably not such hot code that it'll matter too much. (And I don't have a better suggestion in mind at this point!)

::: gfx/thebes/gfxUserFontSet.h:312
(Diff revision 2)
>          // the cache.  (Removals don't increment it.)
>          static uint32_t Generation() { return sGeneration; }
>  
> +        // For each entry in the user font cache where we haven't recorded
> +        // whether the given user font set is allowed to use the entry,
> +        // call IsFontLoadAllowed and recorded it.

s/recorded/record/

::: gfx/thebes/gfxUserFontSet.h:320
(Diff revision 2)
> +        // that we can determine whether a given font load (using a cached
> +        // font) would be allowed without having to call the non-OMT-safe
> +        // IsFontLoadAllowed from the style worker threads.
> +        static void UpdateAllowedFontSets(gfxUserFontSet* aUserFontSet);
> +
> +        // Clears all recored IsFontLoadAllowed results for the given

s/recored/recorded/

::: gfx/thebes/gfxUserFontSet.cpp:1298
(Diff revision 2)
> +    // We have to perform another content policy check here to prevent
> +    // cache poisoning. E.g. a.com loads a font into the cache but
> +    // b.com has a CSP not allowing any fonts to be loaded.

We used to do this check before doing the cache lookup, whereas now we'll always do the lookup and only then check whether it's allowed, which means for the not-allowed case we're doing more work than needed. But I guess that's uncommon enough that we can accept it; it's not worth complicating the code to try and maintain the current approach in the main-thread case.
Attachment #8884519 - Flags: review?(jfkthame) → review+
Comment on attachment 8884520 [details]
Bug 1376964 - Part 5: Add OMT wrapper for nsIURIs useful for font stuff.

https://reviewboard.mozilla.org/r/155412/#review160738

I imagine we might want to generalize this sometime, for use in other situations besides user-fonts, but for now this seems fine.

::: gfx/thebes/gfxFontSrcURI.h:10
(Diff revision 2)
> +
> +#ifndef MOZILLA_GFX_FONTSRCURI_H
> +#define MOZILLA_GFX_FONTSRCURI_H
> +
> +#include "nsIURI.h"
> +#include "nsProxyRelease.h"

Is this needed in the .h file? AFAICS it should be sufficient to include it in the .cpp.
Attachment #8884520 - Flags: review?(jfkthame) → review+
Comment on attachment 8884521 [details]
Bug 1376964 - Part 6: Use gfxFontSrcURI in the user font set and cache.

https://reviewboard.mozilla.org/r/155414/#review161286
Attachment #8884521 - Flags: review?(jfkthame) → review+
Comment on attachment 8884522 [details]
Bug 1376964 - Part 7: Remove unused nsIURI argument from gfxPlatform::IsFontFormatSupported.

https://reviewboard.mozilla.org/r/155416/#review161290

It's not clear to me why we ever had that parameter... good riddance. :)
Attachment #8884522 - Flags: review?(jfkthame) → review+
Comment on attachment 8884524 [details]
Bug 1376964 - Part 9: Use gfxFontSrcPrincipal in the user font set and cache.

https://reviewboard.mozilla.org/r/155420/#review161304

::: gfx/thebes/gfxUserFontSet.h:416
(Diff revision 2)
>              static KeyTypePointer KeyToPointer(KeyType aKey) { return &aKey; }
>  
>              static PLDHashNumber HashKey(const KeyTypePointer aKey) {
> -                uint32_t principalHash = 0;
> +                PLDHashNumber principalHash = 0;
>                  if (aKey->mPrincipal) {
> -                    aKey->mPrincipal->GetHashValue(&principalHash);
> +                    aKey->mPrincipal->Hash();

This looks broken -- it doesn't actually do anything with the Hash() value!
Attachment #8884524 - Flags: review?(jfkthame) → review-
Comment on attachment 8884525 [details]
Bug 1376964 - Part 10: Allow style worker threads to pick fonts out of the user font cache.

https://reviewboard.mozilla.org/r/155422/#review161322
Attachment #8884525 - Flags: review?(jfkthame) → review+
Comment on attachment 8884523 [details]
Bug 1376964 - Part 8: Add OMT wrapper for nsIPrincipals useful for font stuff.

https://reviewboard.mozilla.org/r/155418/#review161400
Attachment #8884523 - Flags: review?(jfkthame) → review+
Comment on attachment 8884524 [details]
Bug 1376964 - Part 9: Use gfxFontSrcPrincipal in the user font set and cache.

https://reviewboard.mozilla.org/r/155420/#review161562
Attachment #8884524 - Flags: review?(jfkthame) → review+
Thanks for the reviews!
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/99c0d338a5f3
Part 1: Record the docshell's "bypass cache" flag on FontFaceSet. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/04e9846700ae
Part 2: Record the docshell's "private browsing" flag on FontFaceSet. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/381e0b72dd55
Part 3: Add a generation counter to the user font cache. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/20866468f52d
Part 4: Call FontLoadAllowed ahead of time and cache the results for style worker threads. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/0031a20388ac
Part 5: Add OMT wrapper for nsIURIs useful for font stuff. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/292d20d46d1f
Part 6: Use gfxFontSrcURI in the user font set and cache. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/a60731cb1a7f
Part 7: Remove unused nsIURI argument from gfxPlatform::IsFontFormatSupported. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/9a0e26964591
Part 8: Add OMT wrapper for nsIPrincipals useful for font stuff. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/b7424dfa039c
Part 9: Use gfxFontSrcPrincipal in the user font set and cache. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/38bc7707e46f
Part 10: Allow style worker threads to pick fonts out of the user font cache. r=jfkthame
sorry had to backout for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=113570318&repo=autoland
Flags: needinfo?(cam)
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aaacb772bada
Part 1: Record the docshell's "bypass cache" flag on FontFaceSet. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/c0e5f138b42a
Part 2: Record the docshell's "private browsing" flag on FontFaceSet. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/0cd32b60dabe
Part 3: Add a generation counter to the user font cache. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/6bfb66a2c03e
Part 4: Call FontLoadAllowed ahead of time and cache the results for style worker threads. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/65e34c5fa167
Part 5: Add OMT wrapper for nsIURIs useful for font stuff. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/d9a88d8324ff
Part 6: Use gfxFontSrcURI in the user font set and cache. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/aafb719b4379
Part 7: Remove unused nsIURI argument from gfxPlatform::IsFontFormatSupported. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/610204957f40
Part 8: Add OMT wrapper for nsIPrincipals useful for font stuff. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/041b34959d76
Part 9: Use gfxFontSrcPrincipal in the user font set and cache. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/c40e5fbbdccf
Part 10: Allow style worker threads to pick fonts out of the user font cache. r=jfkthame
i'm very sorry cameron but that caused bustage again and so backout - https://treeherder.mozilla.org/logviewer.html#?job_id=113602644&repo=autoland
Flags: needinfo?(cam)
That was unified build bustage, I guess.  Trying a build on all platforms:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=99aad6003e458e83404cb756f9192529e9a50c06
Flags: needinfo?(cam)
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c4b8736c4c08
Part 1: Record the docshell's "bypass cache" flag on FontFaceSet. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/c2229c8eef89
Part 2: Record the docshell's "private browsing" flag on FontFaceSet. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/5dc0af8dc8ff
Part 3: Add a generation counter to the user font cache. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/d5c07368105e
Part 4: Call FontLoadAllowed ahead of time and cache the results for style worker threads. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/c648fe419177
Part 5: Add OMT wrapper for nsIURIs useful for font stuff. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/f2ecba994335
Part 6: Use gfxFontSrcURI in the user font set and cache. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/7c316771b894
Part 7: Remove unused nsIURI argument from gfxPlatform::IsFontFormatSupported. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/93d998d3eacd
Part 8: Add OMT wrapper for nsIPrincipals useful for font stuff. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/e1489c41dd18
Part 9: Use gfxFontSrcPrincipal in the user font set and cache. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/bb098650d199
Part 10: Allow style worker threads to pick fonts out of the user font cache. r=jfkthame
Depends on: 1384741
Depends on: 1420680
Assignee: nobody → cam
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: