###!!! ASSERTION: @font-face family name has non-string unit type: 'unit == eCSSUnit_String', file layout/style/nsCSSRuleProcessor.cpp, line 2325

Well, this is an easy fix. If there's no font-family: at all, the rule is useless and should just be ignored. (I wish the spec had been @font-face "Ahem" { ... } instead of @font-face { font-family: "Ahem"; ... } to make it more intuitively clear that that one is essential.) There's a slight problem with testing. I put your test case in layout/style/crashtests, but it passes even with the bug present, although I do see the assertion go by (and there's another test in that directory that gets an assertion but still passes). How is one supposed to test for the absence of assertions?

Comment on attachment 343406 [details] [diff] [review] (rev 1) handle @font-face block without family name correctly in nsCSSRuleProcessor >- if (unit == eCSSUnit_String) { >+ if (unit == eCSSUnit_Null) { >+ // If we have no family name, just ignore the rule. >+ return; >+ } else if (unit == eCSSUnit_String) { > val.GetStringValue(fontfamily); > fontfamily.Trim("\""); > } else { > NS_ASSERTION(unit == eCSSUnit_String, > "@font-face family name has non-string unit type"); > return; > } Why not just: if (unit == eCSSUnit_String) { val.GetStringValue(fontfamily); fontfamily.Trim("\""); } else { NS_ASSERTION(unit == eCSSUnit_Null, "@font-face family name has non-string unit type"); // If we have no family name, just ignore the rule. return; } ? r+sr=dbaron with that Does the spec really support this behavior, though, or is the idea really that you ought to download the font to see what its family name is? (I still don't remember what was decided on whether descriptors override font metadata.)

(In reply to comment #1) > an assertion but still passes). How is one supposed to test for the absence of > assertions? The idea is that crashtests, reftests, and mochitests should eventually be run with assertions being fatal. For now, adding assertion tests to crashtests is the right thing to do.

> Does the spec really support [ignoring @font-face rules with no family], > though, or is the idea really that you ought to download the font to > see what its family name is? The text currently http://dev.w3.org/csswg/css3-fonts/ is ambiguous. About font-family, it says | This descriptor defines the font family name that will be used in all | CSS font family name matching. For the purposes of font name matching | it overrides font family names contained in the underlying font data. but it doesn't say what you do if it's missing. Later, it says that you use the data in the font itself if you haven't got a weight, stretch, or variant descriptor, so I suppose one could parse it as *not* licensing that for family, but I'm not sure I want to split hairs so fine. From the implementation end, I mainly did it this way because I didn't know if the lower layers could handle a user font with no family name. How about we land this patch (with the revisions, which unfortunately I won't be able to do until later this afternoon) to eliminate the assertion, file a query with the WG for disambiguation, and open a new bug if they say you're supposed to do something with a font with a src: but no family:? > The idea is that crashtests, reftests, and mochitests should eventually be run > with assertions being fatal. For now, adding assertion tests to crashtests is > the right thing to do. Ok. Thanks for clarifying.

Here's a revised patch. In addition to the change you requested, David, I decided it would be a good idea to enhance the defensiveness of the code here; so now everything that pulls values out of the rule has an assertion that the CSS unit of what it gets is either null or what it's expecting. I also fixed the code that checks the rule type so it returns early if it's got the wrong kind, in addition to asserting. Requesting re-review since I made changes beyond what was requested.

Comment on attachment 343665 [details] [diff] [review] (rev 2) dbaron's suggestions + extend sanity checks to all data from rule >diff --git a/layout/style/nsCSSRuleProcessor.cpp b/layout/style/nsCSSRuleProcessor.cpp > static void > InsertFontFaceRule(nsICSSRule* aRule, gfxUserFontSet* fs) > { >+ { >+ PRInt32 type; >+ nsresult rv = aRule->GetType(type); >+ if (NS_FAILED(rv) || type != nsICSSRule::FONT_FACE_RULE) { >+ NS_ERROR("InsertFontFaceRule passed a non-fontface CSS rule"); >+ return; >+ } >+ } > nsCSSFontFaceRule *fontFace = static_cast<nsCSSFontFaceRule*> (aRule); >- PRInt32 type; >- NS_ASSERTION(NS_SUCCEEDED(aRule->GetType(type)) >- && type == nsICSSRule::FONT_FACE_RULE, >- "InsertFontFaceRule passed a non-fontface CSS rule"); >- You should leave this as-is; it's a mistake for the caller to call this on a non-font-face rule, and it's pretty easily verifiable that they don't right now. However, putting the assertion before the declaration of fontFace is fine. > } else { >- NS_ASSERTION(unit == eCSSUnit_String, >- "@font-face family name has non-string unit type"); >+ NS_ASSERTION(unit == eCSSUnit_Null, >+ "@font-face family name has unexpected unit"); > return; > } It might be worth putting a big comment above this early return, since it looks somewhat easy to miss. (And explain why you want to return early in only the family case.) > } >+ } else { >+ NS_ASSERTION(unit == eCSSUnit_Null, "@font-face src has unexpected unit"); Feel free to fix the over-indentation (by one space) of the } just above this. r+sr=dbaron

(In reply to comment #6) > >+ { > >+ PRInt32 type; > >+ nsresult rv = aRule->GetType(type); > >+ if (NS_FAILED(rv) || type != nsICSSRule::FONT_FACE_RULE) { > >+ NS_ERROR("InsertFontFaceRule passed a non-fontface CSS rule"); > >+ return; > >+ } > >+ } > > nsCSSFontFaceRule *fontFace = static_cast<nsCSSFontFaceRule*> (aRule); > >- PRInt32 type; > >- NS_ASSERTION(NS_SUCCEEDED(aRule->GetType(type)) > >- && type == nsICSSRule::FONT_FACE_RULE, > >- "InsertFontFaceRule passed a non-fontface CSS rule"); > >- > > You should leave this as-is; it's a mistake for the caller to call this on a > non-font-face rule, and it's pretty easily verifiable that they don't right > now. However, putting the assertion before the declaration of fontFace is > fine. Yes, but since assertions aren't fatal, we need the early return in there to prevent InsertFontFaceRule from blithely going on and calling nsCSSFontFaceRule methods on something that isn't a nsCSSFontFaceRule. Thus factoring out the test, and as far as I can tell from nsDebug.h, the only difference between NS_ERROR and NS_ASSERTION is that the former is unconditional. I would really rather leave this as is. > It might be worth putting a big comment above this early return, since it > looks somewhat easy to miss. (And explain why you want to return early > in only the family case.) Ok. > Feel free to fix the over-indentation (by one space) of the } just above this. Will do.

er, for clarity, I would rather *not* take out the change to make InsertFontFaceRule return early when passed the wrong type of rule.

We don't need to check something at runtime that's never true. In general, something should either be an assertion or be checked at runtime; it shouldn't be both. Sometimes we let them be both as a transition from runtime checking to asserting, but that's a temporary state. Assertions should be conditions that it's not possible to hit.

(In reply to comment #9) > In general, > something should either be an assertion or be checked at runtime; it shouldn't > be both. I would agree with this if assertions were fatal. But they're not, and to my mind, that means *every assertion* needs to be backed up by a runtime check.

(In reply to comment #10) > ... that means *every assertion* needs to be backed up by a runtime check. Weakening this a little: I don't mind if those runtime checks go away in non-debug builds, but I want them there when the assertions are there, so that the control paths that should have been fatal don't instead cascade through and cause more problems.

Feel free to make the assertion in question an NS_ABORT_IF_FALSE.

(In reply to comment #12) > Feel free to make the assertion in question an NS_ABORT_IF_FALSE. It's a deal. Final revision attached.

Comment on attachment 346129 [details] [diff] [review] (rev 3) final >+ { >+ PRInt32 type; >+ nsresult rv = aRule->GetType(type); >+ NS_ABORT_IF_FALSE(NS_SUCCEEDED(rv) && type == nsICSSRule::FONT_FACE_RULE, >+ "InsertFontFaceRule passed a non-fontface CSS rule"); >+ } > nsCSSFontFaceRule *fontFace = static_cast<nsCSSFontFaceRule*> (aRule); >- PRInt32 type; >- NS_ASSERTION(NS_SUCCEEDED(aRule->GetType(type)) >- && type == nsICSSRule::FONT_FACE_RULE, >- "InsertFontFaceRule passed a non-fontface CSS rule"); The aRule->GetType(type) should probably go back inside the NS_SUCCEEDED(), and thus not get compiled in non-DEBUG builds. Also, this may require merging with bug 457821, depending on who lands first. That will move the code, although largely not change it.

Comment on attachment 346129 [details] [diff] [review] (rev 3) final Let's wait until after beta 2 for this.

Comment on attachment 346129 [details] [diff] [review] (rev 3) final a191=beltzner

This patch could use merging to current mozilla-central.

Here's a patch which applies to latest trunk. (The function that needed patching moved to nsPresContext.cpp.)

slight additional revision: incorporate the test case from bug 467490.

flagging for check-in to trunk. Can I assume the approval1.9.1+ carries over?

Yes, it does.

It looks like earlier versions of the patch fixed a missing: unit = val.GetUnit(); in the eCSSFontDesc_Style handling, but the merged one doesn't. I'll stick that fix back in and prepare it for landing next time I land stuff.

Landed on mozilla-central: http://hg.mozilla.org/mozilla-central/rev/01d03b8d40a4 (Still needs to land on 1.9.1.)

Landed on 1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d77d8dc86524

verified FIXED (no assertion with attached test case) on debug builds: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090608 Shiretoko/3.5pre ID:20090608122057 and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090608 Minefield/3.6a1pre ID:20090608122028