Jesse Ruderman Reporter
	Description • 16 years ago

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

	Zack Weinberg (:zwol) Assignee
	Comment 1 • 16 years ago

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?

	David Baron :dbaron:
	Comment 2 • 16 years ago

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

	David Baron :dbaron:
	Comment 3 • 16 years ago

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

	Zack Weinberg (:zwol) Assignee
	Comment 4 • 16 years ago

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

	Zack Weinberg (:zwol) Assignee
	Comment 5 • 16 years ago

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.

	David Baron :dbaron:
	Comment 6 • 16 years ago

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

	Zack Weinberg (:zwol) Assignee
	Comment 7 • 16 years ago

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

	Zack Weinberg (:zwol) Assignee
	Comment 8 • 16 years ago

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

	David Baron :dbaron:
	Comment 9 • 16 years ago

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.

	Zack Weinberg (:zwol) Assignee
	Comment 10 • 16 years ago

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

	Zack Weinberg (:zwol) Assignee
	Comment 11 • 16 years ago

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

	David Baron :dbaron:
	Comment 12 • 16 years ago

Feel free to make the assertion in question an NS_ABORT_IF_FALSE.

	Zack Weinberg (:zwol) Assignee
	Comment 13 • 16 years ago

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

	David Baron :dbaron:
	Comment 14 • 16 years ago

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.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 15 • 16 years ago

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

Let's wait until after beta 2 for this.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 16 • 16 years ago

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

a191=beltzner

	David Baron :dbaron:
	Comment 17 • 16 years ago

This patch could use merging to current mozilla-central.

	Zack Weinberg (:zwol) Assignee
	Comment 19 • 16 years ago

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

	Zack Weinberg (:zwol) Assignee
	Comment 20 • 16 years ago

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

	Zack Weinberg (:zwol) Assignee
	Comment 21 • 16 years ago

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

	David Baron :dbaron:
	Comment 22 • 16 years ago

Yes, it does.

	David Baron :dbaron:
	Comment 23 • 16 years ago

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.

	David Baron :dbaron:
	Comment 24 • 16 years ago

Landed on mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/01d03b8d40a4

(Still needs to land on 1.9.1.)

	David Baron :dbaron:
	Comment 25 • 16 years ago

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

	Aakash Desai [:aakashd]
	Comment 26 • 15 years ago

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