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

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[John Daggett (:jtd)]

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

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Taking per email discussion with jdaggett.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: work in progress

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

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 7: reftests (work in progress)

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

Comment 16

[Jeff Walden [:Waldo]]

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

[Jeff Walden [:Waldo]]

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

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 7: remove nsGfxFontLoaderContext

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

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 24

[Boris Zbarsky [:bzbarsky]]

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.

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

[Boris Zbarsky [:bzbarsky]]

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?

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

[Boris Zbarsky [:bzbarsky]]

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?

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 9: reftests

Added a few additional tests.

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 32

[John Daggett (:jtd)]

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

My bad, missed trimming this out.

Comment 33

[John Daggett (:jtd)]

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

[John Daggett (:jtd)]

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

Comment 35

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 36

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 37

[Karl Tomlinson (:karlt)]

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

Comment 38

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 40

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 41

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 42

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2-3-4 interdiff

Comment 43

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 44

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 8: make the font cache check for a user font set match and update the font list

revised as described in previous comment

Comment 45

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2-3-4: do the necessary rebuilding when needed

Comment 46

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 47

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2-3-4 combined interdiff

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

Comment 48

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: 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!)

Comment 49

[Karl Tomlinson (:karlt)]

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

[Boris Zbarsky [:bzbarsky]]

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

Looks good.

Comment 51

[Boris Zbarsky [:bzbarsky]]

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

Doh.  Of course.

Comment 52

[John Daggett (:jtd)]

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

Comment 53

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Could we push that part off to a separate bug?

Comment 54

[John Daggett (:jtd)]

     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.

Comment 55

[John Daggett (:jtd)]

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

Done. Bug 465648.

Comment 56

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 57

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 58

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 59

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 60

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 61

[Aakash Desai [:aakashd]]

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

Comment 62

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 63

[Aakash Desai [:aakashd]]

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.