To support downloadable fonts, a user font set object is created when @font-face rules are found in style content (see bug 441473). For each @font-face rule that is processed, a font entry is added to the user font set object. However, when a stylesheet is disabled, the @font-face rules for the disabled stylesheet need to also be disabled. Discussing this with dbaron, we decided that it made sense to simply rebuild the user font set object when stylesheets containing @font-face rules are enabled or disabled (see bug 441473 comment 77).

Updating summary to reflect that we also need to do this when rules in sheets are modified, added, or removed. (That requires hooking into a different code path than enabling/disabling entire sheets, so it's worth noting.)

So hooking in to nsCSSRuleProcessor::ClearRuleCascades or perhaps just nsCSSStyleSheet::ClearRuleCascades will catch the enabled/disabled cases and the rule insertion/removal cases -- that's probably the best place to clear the old user font set. You also need to hook into nsCSSRuleProcessor::RefreshRuleCascade to catch media-dependent style sheets.

Taking per email discussion with jdaggett.

Here's some work in progress; the marking-dirty stuff actually looks harder than I thought it would be; I think I might need to ensure that all of these things happen inside BeginUpdate/EndUpdate (UPDATE_STYLE) and then query all the rule processors for dirtyness inside the style set's EndUpdate.

How about a version that compiles and runs, at least?

I'm going to attach the current state of my thinking. I have a set of four patches that I think ought to work (who knows, maybe they're close), but I haven't done any testing. (For now, I'll need to switch to a different platform to do that testing.) This is roughly the same as attachment 346347 [details] [diff] [review]. It builds the array of @font-face rules up from the levels of the cascade, since some of the time we can reprocess one level of the cascade but not others (due to various types of dynamic changes). Since yesterday, I fixed things so we again don't have a user font set when there are no @font-face rules. This moves InsertFontFaceRule from one file to another, which makes the patch look big. (I took out two unused variables while I was moving it.)

I finally came up with a simple way to do this, by observing that everything that goes through one of the two codepaths in comment 2 also goes through one of these two codepaths. The patch in comment 1 means we don't actually need any clearing at the points I mentioned in comment 2 as long as we do the rebuilding, since the patch in comment 1 means we're storing the data in the RuleCascadeData, which is already cleared or swapped out in those codepaths. I need to verify my assertion that this should work more closely and probably add a bunch of comments pointing out that this is how things work.

This optimizes the rebuild with an array comparison. I think the main thing this saves is really not rebuilding the font set, but the ensuing style change reflow (see patch 4).

Patch 2 introduces the possibility of removing @font-face rules from the set of rules we're considering. This patch causes us to do a style change reflow in that case just like we do when font loads complete, and adds comments both similar cases pointing at the other.

In comment 7, when I wrote "the patch in comment 1", I meant "patch 1".

I've made a bit of progress on tests, although I still need to do a lot more. I'd like to test interaction of the dynamic cases with the ordering of rules since I think the changes I've made should have improved that, except that the static tests I've written for that don't work on the trunk. I also need to test media changes as well. I also need to figure out how to actually meet the security restrictions; right now running them from file: URLs hits the can't-cross-directory security restriction, I think. I also need to make sure that the tests that should be failing on the trunk are, and that the ones that should be passing with my patch are, etc.

(In reply to comment #11) > I'd like to test interaction of the dynamic cases with the ordering of rules > since I think the changes I've made should have improved that, except that the > static tests I've written for that don't work on the trunk. Note that I might be confused about what we ought to be implementing; I'll send email about that tomorrow.

This uses relative URL resolution so that one of the URLs can be served over HTTP and the other can be about:blank or a data: URL.

This allows "HTTP(..)", "HTTP(../..)", "HTTP(../../../)" for reftests so that we can relax the restriction that all the resources need to be in the same directory. This lets us use downloadable fonts in reftests from a resource directory containing fonts. (We can't use file: because of the cross-directory security restrictions on file:.)

More later, but here's a set that actually use downloadable fonts. They don't actually pass yet...

Comment on attachment 346740 [details] [diff] [review] patch 5: allow about:blank as a reference with HTTP reftests You might as well add a data: URL test with HTTP specified while you're at it, just for completeness.

Comment on attachment 346741 [details] [diff] [review] patch 6: allow variants of HTTP so that resources can be served from parent directory I wish reftest weren't line-based but rather whole-manifest-based, so we could just have one server rooted in one location and we wouldn't have this weird directory restriction. Probably too much effort to make the change worthwhile, alas...

I had to merge 2, 3, and 4. I don't think it's much of a loss. I'm now forcing a reflow when the user font set changes (not just non-null to null), since otherwise we'll never hit the loader bits.

Remove an unused class that made things a little more confusing.

This prevents the font cache from returning a font metrics object that has the wrong user font set. I had to add to nsIFontMetrics (though I could have added to nsIThebesFontMetrics, but I don't see why it matters and this is a little less ugly) because the font cache is in gkgfx, which doesn't link against thebes, so I can't call methods on a font group.

There are still a whole bunch more tests I need to write, but first I need to figure out why some of these are failing for vertical positioning differences and then mark and work around the substantive failures.

This fixes the height differences I was seeing in reftests (resulting from bad metrics).

Added an up-front null-check of mShell in case the pres context has been destroyed by the time we get the event.

Comment on attachment 346553 [details] [diff] [review] patch 1: build the list of @font-face rules from the correct pieces, guaranteeing the correct order >+++ b/layout/base/nsPresContext.cpp >+InsertFontFaceRule(nsCSSFontFaceRule *aRule, gfxUserFontSet* aFontSet) NS_ABORT_IF_FALSE(NS_SUCCEEDED(aRule->GetType(type)) >+ && type == nsICSSRule::FONT_FACE_RULE, >+ "InsertFontFaceRule passed a non-fontface CSS rule"); I'm not sure how this is expected to work.... GetType is a virtual function, and if we use the vtable for nsCSSFontFaceRule we'll always get back nsICSSRule::FONT_FACE_RULE, no? So what's this checking? >+ if (unit == eCSSUnit_String) { ... >+ } else { >+ NS_ASSERTION(unit == eCSSUnit_String, >+ "@font-face family name has non-string unit type"); I know you just moved this code, but this looks odd. Just make it an NS_ERROR if that's what we mean? >+++ b/layout/style/nsCSSRuleProcessor.cpp >+ * (2) Add any @font-face rules, in order, into XXX WRITE ME Please do. sr=bzbarsky with those addressed.

(In reply to comment #24) > (From update of attachment 346553 [details] [diff] [review]) > >+++ b/layout/base/nsPresContext.cpp > >+InsertFontFaceRule(nsCSSFontFaceRule *aRule, gfxUserFontSet* aFontSet) > NS_ABORT_IF_FALSE(NS_SUCCEEDED(aRule->GetType(type)) > >+ && type == nsICSSRule::FONT_FACE_RULE, > >+ "InsertFontFaceRule passed a non-fontface CSS rule"); > > I'm not sure how this is expected to work.... GetType is a virtual function, > and if we use the vtable for nsCSSFontFaceRule we'll always get back > nsICSSRule::FONT_FACE_RULE, no? So what's this checking? No. It'll still call the appropriate virtual function despite the cast. > >+ if (unit == eCSSUnit_String) { > ... > >+ } else { > >+ NS_ASSERTION(unit == eCSSUnit_String, > >+ "@font-face family name has non-string unit type"); > > I know you just moved this code, but this looks odd. Just make it an NS_ERROR > if that's what we mean? I think this is already fixed in bug 460217, so I'd rather just let both patches land and whoever lands second gets to merge. > >+++ b/layout/style/nsCSSRuleProcessor.cpp > >+ * (2) Add any @font-face rules, in order, into XXX WRITE ME > > Please do. Er, right.

Comment on attachment 346929 [details] [diff] [review] patch 2-3-4: do the necessary rebuilding when needed Shouldn't GetUserFontSet still do a (lazy) rebuild?

(In reply to comment #26) > Shouldn't GetUserFontSet still do a (lazy) rebuild? I suppose it could (that was my original approach), but any case where it matters, we're not even going to call GetUserFontSet unless we force a style change reflow. So if we're not forcing that style change reflow, gfx will never decide that it wants to load the fonts (and thus force another style change reflow) and layout will never rebuild text runs to get to a situation where it has different font groups. So I'm not sure I see the point given the rest of the changes.

Comment on attachment 346929 [details] [diff] [review] patch 2-3-4: do the necessary rebuilding when needed OK, then nix the comments that talk about that lazy rebuild?

I'm probably going to land these, with the ones for this bug commented out temporarily, sometime this weekend. Feel free to have a look through them, but I'm not going to ask that of anyone...

Added a few additional tests.

I landed: http://hg.mozilla.org/mozilla-central/rev/25ea3e7408ba (patch 5) http://hg.mozilla.org/mozilla-central/rev/04a589e040b9 (patch 6) http://hg.mozilla.org/mozilla-central/rev/70826688e822 (patch 9, part a) http://hg.mozilla.org/mozilla-central/rev/229683caa4c3 (patch 9, part b) This patch contains the reftests that I haven't landed yet, which are those that deal with dynamic cases (i.e., are specifically for this bug, rather than testing base cases that I wanted to test to ensure that these tests are sane).

Comment on attachment 346807 [details] [diff] [review] patch 7: remove nsGfxFontLoaderContext My bad, missed trimming this out.

(In reply to comment #20) > Created an attachment (id=346808) [details] > patch 8: make the font cache check for a user font set match > > This prevents the font cache from returning a font metrics object that has the > wrong user font set. > nsIFontMetrics* fm; > PRInt32 n = mFontMetrics.Count() - 1; > for (PRInt32 i = n; i >= 0; --i) { > fm = static_cast<nsIFontMetrics*>(mFontMetrics[i]); > - if (fm->Font().Equals(aFont)) { > + nsIThebesFontMetrics* tfm = static_cast<nsIThebesFontMetrics*>(fm); > + if (fm->Font().Equals(aFont) && tfm->GetUserFontSet() == aUserFontSet) { > nsCOMPtr<nsIAtom> langGroup; > fm->GetLangGroup(getter_AddRefs(langGroup)); > if (aLangGroup == langGroup.get()) { > if (i != n) { > // promote it to the end of the cache > mFontMetrics.MoveElement(i, n); > } > + tfm->GetThebesFontGroup()->UpdateFontList(); > NS_ADDREF(aMetrics = fm); > return NS_OK; > } > } > } I'm not sure I quite understand this. Why the need to call UpdateFontList? This will happen automatically whenever a text run is recreated, the point at which font matching is performed. Also, isn't there a chance that after a user font set has been torn down and recreated that you could end up with an identical pointer?

Comment on attachment 346553 [details] [diff] [review] patch 1: build the list of @font-face rules from the correct pieces, guaranteeing the correct order Just one small nit: + * (2) Add any @font-face rules, in order, into XXX WRITE ME Into the ???

(In reply to comment #34) > (From update of attachment 346553 [details] [diff] [review]) > Just one small nit: > > + * (2) Add any @font-face rules, in order, into XXX WRITE ME > > Into the ??? (2) add any @font-face rules, in order, into data->mFontFaceRules.

(In reply to comment #33) > I'm not sure I quite understand this. Why the need to call UpdateFontList? > This will happen automatically whenever a text run is recreated, the point at > which font matching is performed. Failing to call UpdateFontList was resulting in incorrect metrics being returned in the function GetNormalLineHeight in nsHTMLReflowState.cpp. This function calls aFontMetrics->GetExternalLeading(), which in turn uses the metrics on the first font in the font group. If we don't call UpdateFontList, the first font in the font group isn't updated until later, and we fail a bunch of the reftests because the line spacing is incorrect (since the test ends up still using the line spacing for the fallback font, while the reference uses the line spacing from the downloaded font). http://hg.mozilla.org/mozilla-central/annotate/d11e76e6fab9/layout/generic/nsHTMLReflowState.cpp#l1992 > Also, isn't there a chance that after a user font set has been torn down and > recreated that you could end up with an identical pointer? No. Since gfxFontGroup::mUserFontSet owns a reference to mUserFontSet, the old one won't actually be destroyed until after all the font groups pointing to it have gone away.

(In reply to comment #36) > (In reply to comment #33) > > I'm not sure I quite understand this. Why the need to call UpdateFontList? > > This will happen automatically whenever a text run is recreated, the point at > > which font matching is performed. > > Failing to call UpdateFontList was resulting in incorrect metrics being > returned in the function GetNormalLineHeight in nsHTMLReflowState.cpp. This > function calls aFontMetrics->GetExternalLeading(), which in turn uses the > metrics on the first font in the font group. If we don't call UpdateFontList, > the first font in the font group isn't updated until later, and we fail a bunch > of the reftests because the line spacing is incorrect. Is this going to be the only situation where we hit this? I fear that if we have one fontgroup user needing to do this (from a distance), there may be others. Or should the gfxFontGroup::GetFontAt(PRInt32) implementations do the UpdateFontList?

(In reply to comment #37) > Is this going to be the only situation where we hit this? I fear that if we > have one fontgroup user needing to do this (from a distance), there may be > others. > > Or should the gfxFontGroup::GetFontAt(PRInt32) implementations do the > UpdateFontList? Yeah, I was wondering at some point if this should be elsewhere. I'm curious what John thinks.

Comment on attachment 346929 [details] [diff] [review] patch 2-3-4: do the necessary rebuilding when needed So for some reason this patch seems to make layout/reftests/css-import/290018-1-ref.html fail because the quirks style sheet starts applying to the reference (but not the test). (I need to double-check that I've diagnosed things correctly...)

The problem seems to be the AppendFontFaceRules call in nsPresContext::FlushUserFontSet, which I guess causes us to build the rule cascade earlier. I suppose I should just finish bug 450191, though I should probably look into this a little more; perhaps there's something I could be testing to avoid an early call. (I thought that reftest failure was just a random failure like a bunch of others, but today I bisected all the patches in my tree to figure out what was causing it.)

Here's a revised version of patch 2-3-4 that goes back to lazily rebuilding in the initial case and thus fixes the problem in comment 39 and comment 40. I'll attach an interdiff for review shortly, since the previous patch was already reviewed.

(In reply to comment #37) > Is this going to be the only situation where we hit this? I fear that if we > have one fontgroup user needing to do this (from a distance), there may be > others. > > Or should the gfxFontGroup::GetFontAt(PRInt32) implementations do the > UpdateFontList? My inclination is to stick an assertion and comment in the GetFontAt implementations so we don't pay the performance penalty of checking every time in release builds, but if somebody else runs into the same problem they'll hit the assertion and quickly know what the problem is (and see the comment suggesting the possible alternative).

revised as described in previous comment

Here's a second interdiff for patch 2-3-4. It turns out that my revisions yesterday caused a bug where StyleChangedReflow would get called during the lazy build that we do during the first GetUserFontSet call. Since that code was getting confusing (it got me, clearly), I switched from two booleans to three that better describe what I care about, and as a bonus got to deal with out-of-memory while posting events a little bit better. At this point I'm not sure if it's easier to re-review the whole patch; I'm not sure of a good way to combine the interdiffs, which overlap a lot. If I could combine them, the interdiffs would probably be easier...

Actually, there's this nice command called 'combinediff'.

And this fixes the obvious mistake in my second interdiff, which, amazingly, wasn't caught by my tests. I should go add something that will catch it to my tests. (I guess none of them require rebuilding twice, at least not in a way that won't do anything at all unless we force the reflow!)

(In reply to comment #43) > My inclination is to stick an assertion and comment in the GetFontAt > implementations so we don't pay the performance penalty of checking every time > in release builds, but if somebody else runs into the same problem they'll hit > the assertion and quickly know what the problem is (and see the comment > suggesting the possible alternative). That sounds sensible in principle. I don't know, however, whether that assertion is strictly required. I wonder whether GetFontAt may be called at a time after another reflow has already been scheduled (but not started)? If so, it's not strictly necessary to get exactly the right font (though getting the right font would still be nice). (I don't mean this must hold up this patch. I'm asking the question because I don't yet know the answers.)

Comment on attachment 348614 [details] [diff] [review] patch 2-3-4 combined interdiff Looks good.

Comment on attachment 348618 [details] [diff] [review] patch 2-3-4 interdiff #3 Doh. Of course.

(In reply to comment #38) > (In reply to comment #37) > > Is this going to be the only situation where we hit this? I fear that if we > > have one fontgroup user needing to do this (from a distance), there may be > > others. > > > > Or should the gfxFontGroup::GetFontAt(PRInt32) implementations do the > > UpdateFontList? > > Yeah, I was wondering at some point if this should be elsewhere. I'm curious > what John thinks. I think we probably want to have UpdateFontList inside GetFontAt(). But we probably also need to verify that the code that's calling GetFontAt isn't assuming this won't change, especially in the case of upstream consumers of metrics info.

Could we push that part off to a separate bug?

virtual gfxFont *GetFontAt(PRInt32 i) { + // If it turns out to be hard for all clients that cache font + // groups to call UpdateFontList at appropriate times, we could + // instead consider just calling UpdateFontList from someplace + // more central (such as here). + NS_ASSERTION(!mUserFontSet || mCurrGeneration == GetGeneration(), + "Whoever was caching this font group should have " + "called UpdateFontList on it"); + I don't think this code will ever execute, since each of the per-platform font group classes override this method: http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/src/gfxWindowsFonts.cpp#889 http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/public/gfxAtsuiFonts.h#135 http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/src/gfxPangoFonts.cpp#1402 So I think you need to dupe the assertion into each of those methods.

(In reply to comment #53) > Could we push that part off to a separate bug? Done. Bug 465648.

Good point. This copies the assertion to all GetFontAt implementations. I reran the font-face reftests (including patch 9c) with this patch, and still didn't hit the assertion, which is good (on Linux, Windows, and Mac).

Fixed in mozilla-central (and thus also on mozilla-1.9.1): http://hg.mozilla.org/mozilla-central/rev/91690545debc http://hg.mozilla.org/mozilla-central/rev/71032d21e8b1 http://hg.mozilla.org/mozilla-central/rev/5bc893b6bdf2 http://hg.mozilla.org/mozilla-central/rev/8d2453e98c3f http://hg.mozilla.org/mozilla-central/rev/6b99f95a3ee3 Then backed out due to a Mac startup crash, whose underlying cause was actually bug 466399: http://hg.mozilla.org/mozilla-central/rev/85a49cd83f46 http://hg.mozilla.org/mozilla-central/rev/6ba942a7efa9 http://hg.mozilla.org/mozilla-central/rev/5bf014d8ab20 http://hg.mozilla.org/mozilla-central/rev/8bd6113ce27c http://hg.mozilla.org/mozilla-central/rev/db0e5d413618 http://hg.mozilla.org/mozilla-central/rev/05ed7ddf001b http://hg.mozilla.org/mozilla-central/rev/c965c099599b http://hg.mozilla.org/mozilla-central/rev/3957288748e1 http://hg.mozilla.org/mozilla-central/rev/2c326abe584a http://hg.mozilla.org/mozilla-central/rev/8b943e17629a Then relanded, and thus again fixed in mozilla-central (and thus also on mozilla-1.9.1): http://hg.mozilla.org/mozilla-central/rev/3946e0e1fa54 http://hg.mozilla.org/mozilla-central/rev/948149fa2c65 http://hg.mozilla.org/mozilla-central/rev/13709ba9ea21 http://hg.mozilla.org/mozilla-central/rev/cf88eac1c192 http://hg.mozilla.org/mozilla-central/rev/97214dc4dc0a

Running mochitest content/html/content/tests on 64bit linux asserts ###!!! ASSERTION: FlushUserFontSet should have been called first: '!mGetUserFontSetCalled', file /mozilla/layout/base/nsPresContext.cpp, line 1715

Running the whole mochitest asserts also ###!!! ASSERTION: @font-face family name has non-string unit type: 'unit == eCSSUnit_String', file /mozilla/layout/base/nsPresContext.cpp, line 1602

(In reply to comment #58) > Running mochitest content/html/content/tests on 64bit linux asserts > ###!!! ASSERTION: FlushUserFontSet should have been called first: > '!mGetUserFontSetCalled', file /mozilla/layout/base/nsPresContext.cpp, line > 1715 That's bug 466756, which is a regression from this patch. (In reply to comment #59) > Running the whole mochitest asserts also > ###!!! ASSERTION: @font-face family name has non-string unit type: 'unit == > eCSSUnit_String', file /mozilla/layout/base/nsPresContext.cpp, line 1602 That's bug 460217, which has been waiting for checkin / approval for weeks now.

(In reply to comment #60) > (In reply to comment #58) > > Running mochitest content/html/content/tests on 64bit linux asserts > > ###!!! ASSERTION: FlushUserFontSet should have been called first: > > '!mGetUserFontSetCalled', file /mozilla/layout/base/nsPresContext.cpp, line > > 1715 > > That's bug 466756, which is a regression from this patch. > > (In reply to comment #59) > > Running the whole mochitest asserts also > > ###!!! ASSERTION: @font-face family name has non-string unit type: 'unit == > > eCSSUnit_String', file /mozilla/layout/base/nsPresContext.cpp, line 1602 > > That's bug 460217, which has been waiting for checkin / approval for weeks now. Do these mochitest testcases work for this bug? If not, do we need testcases?

No, they don't, but patch 9 contained plenty of tests.

If that's the case, there hasn't been any discussions about this bug for 4 months, I'm guessing there aren't any residual issues. I'm moving this to verified as a result. If anyone has any qualms, feel free to bring them up.