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.

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

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.

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

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

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

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.

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.

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

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. :)

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!

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

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

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

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

Thanks. Let's try again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3ee17f4440446f53e42d6e3ade47db32c150d33

Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b4c058f045a9 Backed out changeset 38bc7707e46f https://hg.mozilla.org/integration/autoland/rev/9e925c918d53 Backed out changeset b7424dfa039c https://hg.mozilla.org/integration/autoland/rev/587efeb89068 Backed out changeset 9a0e26964591 https://hg.mozilla.org/integration/autoland/rev/21644b144b34 Backed out changeset a60731cb1a7f https://hg.mozilla.org/integration/autoland/rev/3cc011c3eab2 Backed out changeset 292d20d46d1f https://hg.mozilla.org/integration/autoland/rev/18ce652bd03b Backed out changeset 0031a20388ac https://hg.mozilla.org/integration/autoland/rev/998d5c3d83b5 Backed out changeset 20866468f52d https://hg.mozilla.org/integration/autoland/rev/3b053931742a Backed out changeset 381e0b72dd55 https://hg.mozilla.org/integration/autoland/rev/95b3b94ce27e Backed out changeset 04e9846700ae https://hg.mozilla.org/integration/autoland/rev/34fe793e4c4d Backed out changeset 99c0d338a5f3 for bustage

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

Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/46f45eff2bfc Backed out changeset c40e5fbbdccf https://hg.mozilla.org/integration/autoland/rev/a3d1b2c6c59f Backed out changeset 041b34959d76 https://hg.mozilla.org/integration/autoland/rev/a3a6fc9b38b8 Backed out changeset 610204957f40 https://hg.mozilla.org/integration/autoland/rev/50d1812c6f87 Backed out changeset aafb719b4379 https://hg.mozilla.org/integration/autoland/rev/b82e88525bd6 Backed out changeset d9a88d8324ff https://hg.mozilla.org/integration/autoland/rev/a51a4b6e45d0 Backed out changeset 65e34c5fa167 https://hg.mozilla.org/integration/autoland/rev/ea314f31ac51 Backed out changeset 6bfb66a2c03e https://hg.mozilla.org/integration/autoland/rev/e4a33d2cfd5b Backed out changeset 0cd32b60dabe https://hg.mozilla.org/integration/autoland/rev/349f2dfda8a3 Backed out changeset c0e5f138b42a https://hg.mozilla.org/integration/autoland/rev/cb6056e7c490 Backed out changeset aaacb772bada for bustage - error C2143: syntax error

i'm very sorry cameron but that caused bustage again and so backout - https://treeherder.mozilla.org/logviewer.html#?job_id=113602644&repo=autoland

That was unified build bustage, I guess. Trying a build on all platforms: https://treeherder.mozilla.org/#/jobs?repo=try&revision=99aad6003e458e83404cb756f9192529e9a50c06

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

https://hg.mozilla.org/mozilla-central/rev/c4b8736c4c08 https://hg.mozilla.org/mozilla-central/rev/c2229c8eef89 https://hg.mozilla.org/mozilla-central/rev/5dc0af8dc8ff https://hg.mozilla.org/mozilla-central/rev/d5c07368105e https://hg.mozilla.org/mozilla-central/rev/c648fe419177 https://hg.mozilla.org/mozilla-central/rev/f2ecba994335 https://hg.mozilla.org/mozilla-central/rev/7c316771b894 https://hg.mozilla.org/mozilla-central/rev/93d998d3eacd https://hg.mozilla.org/mozilla-central/rev/e1489c41dd18 https://hg.mozilla.org/mozilla-central/rev/bb098650d199