need to rebuild user font set objects when style sheets are disabled/enabled or rules are modified/added/removed

VERIFIED FIXED in mozilla1.9.1b3

Status

()

P3
major
VERIFIED FIXED
10 years ago
8 years ago

People

(Reporter: jtd, Assigned: dbaron)

Tracking

(Depends on: 1 bug, {verified1.9.1})

Trunk
mozilla1.9.1b3
verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(10 attachments, 17 obsolete attachments)

26.28 KB, patch
jtd
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
2.15 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
7.13 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
1.14 KB, patch
jtd
: review+
Details | Diff | Splinter Review
70.27 KB, patch
Details | Diff | Splinter Review
31.48 KB, patch
Details | Diff | Splinter Review
13.90 KB, patch
Details | Diff | Splinter Review
6.62 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
1.89 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
13.77 KB, patch
jtd
: review+
Details | Diff | Splinter Review
(Reporter)

Description

10 years ago
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).
Flags: blocking1.9.1?
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.)
Component: GFX: Thebes → Style System (CSS)
QA Contact: thebes → style-system
Summary: need to rebuilt user font set objects when stylesheets are disabled or enabled → need to rebuild user font set objects when style sheets are disabled/enabled or rules are modified/added/removed
(Reporter)

Updated

10 years ago
Priority: -- → P1
Flags: blocking1.9.1? → blocking1.9.1+
Priority: P1 → P3
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.
Assignee: jdaggett → dbaron
Created attachment 346342 [details] [diff] [review]
work in progress

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.
Created attachment 346347 [details] [diff] [review]
work in progress

How about a version that compiles and runs, at least?
Attachment #346342 - Attachment is obsolete: true
Created attachment 346553 [details] [diff] [review]
patch 1: build the list of @font-face rules from the correct pieces, guaranteeing the correct order

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.)
Attachment #346347 - Attachment is obsolete: true
Created attachment 346555 [details] [diff] [review]
patch 2: mark the font set dirty when needed

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.
Created attachment 346556 [details] [diff] [review]
patch 3: optimize font set rebuild with array comparison

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).
Created attachment 346558 [details] [diff] [review]
patch 4: do a style change reflow when we remove @font-face rules

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.
Created attachment 346620 [details] [diff] [review]
patch 5: reftests (work in progress)

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.
Created attachment 346740 [details] [diff] [review]
patch 5: allow about:blank as a reference with HTTP reftests

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.
Attachment #346740 - Flags: review?
Created attachment 346741 [details] [diff] [review]
patch 6: allow variants of HTTP so that resources can be served from parent directory

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:.)
Attachment #346741 - Flags: review?(jwalden+bmo)
Created attachment 346742 [details] [diff] [review]
patch 7: reftests (work in progress)

More later, but here's a set that actually use downloadable fonts.  They don't actually pass yet...
Attachment #346620 - Attachment is obsolete: true

Updated

10 years ago
Attachment #346740 - Flags: review?(jwalden+bmo) → review+

Comment 16

10 years ago
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 17

10 years ago
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...
Attachment #346741 - Flags: review?(jwalden+bmo) → review+
Created attachment 346806 [details] [diff] [review]
patch 2-3-4: do the necessary rebuilding when needed

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.
Attachment #346555 - Attachment is obsolete: true
Attachment #346556 - Attachment is obsolete: true
Attachment #346558 - Attachment is obsolete: true
Created attachment 346807 [details] [diff] [review]
patch 7: remove nsGfxFontLoaderContext

Remove an unused class that made things a little more confusing.
Attachment #346807 - Flags: review?(jdaggett)
Created attachment 346808 [details] [diff] [review]
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.

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.
Attachment #346808 - Flags: superreview?(roc)
Attachment #346808 - Flags: review?(jdaggett)
Created attachment 346831 [details] [diff] [review]
patch 9: reftests (work in progress)

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.
Attachment #346742 - Attachment is obsolete: true
Created attachment 346855 [details] [diff] [review]
patch 8: make the font cache check for a user font set match and update the font list

This fixes the height differences I was seeing in reftests (resulting from bad metrics).
Attachment #346808 - Attachment is obsolete: true
Attachment #346855 - Flags: superreview?(roc)
Attachment #346855 - Flags: review?(jdaggett)
Attachment #346808 - Flags: superreview?(roc)
Attachment #346808 - Flags: review?(jdaggett)
Created attachment 346929 [details] [diff] [review]
patch 2-3-4: do the necessary rebuilding when needed

Added an up-front null-check of mShell in case the pres context has been destroyed by the time we get the event.
Attachment #346806 - Attachment is obsolete: true
Attachment #346929 - Flags: superreview?(bzbarsky)
Attachment #346929 - Flags: review?(jdaggett)
Attachment #346806 - Flags: superreview?(bzbarsky)
Attachment #346806 - Flags: review?(jdaggett)
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.
Attachment #346553 - Flags: superreview?(bzbarsky) → superreview+
(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?
Attachment #346929 - Flags: superreview?(bzbarsky) → superreview+
Created attachment 347005 [details] [diff] [review]
patch 9: reftests

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...
Attachment #346831 - Attachment is obsolete: true
Created attachment 347068 [details] [diff] [review]
patch 9c: remaining reftests

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

Comment 32

10 years ago
Comment on attachment 346807 [details] [diff] [review]
patch 7: remove nsGfxFontLoaderContext

My bad, missed trimming this out.
Attachment #346807 - Flags: review?(jdaggett) → review+
(Reporter)

Updated

10 years ago
Blocks: 457825
(Reporter)

Comment 33

10 years ago
(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?
(Reporter)

Comment 34

10 years ago
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 ???
Attachment #346553 - Flags: review?(jdaggett) → review+
(Reporter)

Updated

10 years ago
Attachment #346929 - Flags: review?(jdaggett) → review+
(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.
(Reporter)

Updated

10 years ago
Attachment #346855 - Flags: review?(jdaggett) → review+
(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.)
Created attachment 348461 [details] [diff] [review]
patch 2-3-4: do the necessary rebuilding when needed

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.
Attachment #346929 - Attachment is obsolete: true
(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).
Created attachment 348597 [details] [diff] [review]
patch 8: make the font cache check for a user font set match and update the font list

revised as described in previous comment
Attachment #346855 - Attachment is obsolete: true
Attachment #348597 - Flags: superreview?(roc)
Attachment #348597 - Flags: review?(jdaggett)
Attachment #346855 - Flags: superreview?(roc)
Created attachment 348613 [details] [diff] [review]
patch 2-3-4 interdiff #2

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...
Attachment #348613 - Flags: superreview?(bzbarsky)
Attachment #348613 - Flags: review?(bzbarsky)
Created attachment 348614 [details] [diff] [review]
patch 2-3-4 combined interdiff

Actually, there's this nice command called 'combinediff'.
Attachment #348462 - Attachment is obsolete: true
Attachment #348613 - Attachment is obsolete: true
Attachment #348614 - Flags: superreview?(bzbarsky)
Attachment #348614 - Flags: review?(bzbarsky)
Attachment #348462 - Flags: superreview?(bzbarsky)
Attachment #348462 - Flags: review?(bzbarsky)
Attachment #348613 - Flags: superreview?(bzbarsky)
Attachment #348613 - Flags: review?(bzbarsky)
Created attachment 348618 [details] [diff] [review]
patch 2-3-4 interdiff #3

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!)
Attachment #348618 - Flags: superreview?(bzbarsky)
Attachment #348618 - Flags: review?(bzbarsky)
(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.)
Attachment #348597 - Flags: superreview?(roc) → superreview+
Comment on attachment 348614 [details] [diff] [review]
patch 2-3-4 combined interdiff

Looks good.
Attachment #348614 - Flags: superreview?(bzbarsky)
Attachment #348614 - Flags: superreview+
Attachment #348614 - Flags: review?(bzbarsky)
Attachment #348614 - Flags: review+
Comment on attachment 348618 [details] [diff] [review]
patch 2-3-4 interdiff #3

Doh.  Of course.
Attachment #348618 - Flags: superreview?(bzbarsky)
Attachment #348618 - Flags: superreview+
Attachment #348618 - Flags: review?(bzbarsky)
Attachment #348618 - Flags: review+
(Reporter)

Comment 52

10 years ago
(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.
(Reporter)

Comment 54

10 years ago
     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.
(Reporter)

Updated

10 years ago
Attachment #348597 - Flags: review?(jdaggett) → review-
(Reporter)

Comment 55

10 years ago
(In reply to comment #53)
> Could we push that part off to a separate bug?

Done. Bug 465648.
Created attachment 348925 [details] [diff] [review]
patch 8: make the font cache check for a user font set match and update the font list

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).
Attachment #348597 - Attachment is obsolete: true
Attachment #348925 - Flags: review?(jdaggett)
(Reporter)

Updated

10 years ago
Attachment #348925 - Flags: review?(jdaggett) → review+
Whiteboard: [needs re-review on one last piece] → [needs landing]

Updated

10 years ago
Depends on: 466756
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
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla1.9.1
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.
Depends on: 467084
Keywords: fixed1.9.1
(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?
Flags: in-testsuite?
No, they don't, but patch 9 contained plenty of tests.
Flags: in-testsuite? → in-testsuite+
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.
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
Depends on: 550963
Depends on: 622255
You need to log in before you can comment on or make changes to this bug.