873222 - "ABORT: not an int value" serializing font and font-synthesis properties

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ABORT: not an int value: 'mUnit == eCSSUnit_Integer || mUnit == eCSSUnit_Enumerated || mUnit == eCSSUnit_EnumColor', file ../../dist/include/nsCSSValue.h, line 345

aProperty is eCSSProperty_font; mUnit is eCSSUnit_None.

The string returned from r.getAttribute("style") in a non-debug build is suspect: it includes the font value as it was given, but elides the font-synthesis value.

Comment 1

[Jesse Ruderman]

Attachment: stack+ (gdb)

Comment 2

[John Daggett (:jtd)]

Attachment: patch, get for the none unit type before fetching int value

Comment 3

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

Comment on attachment 751591 [details] [diff] [review]
patch, get for the none unit type before fetching int value

It's probably cleaner to do this the same way the |stretch| check is done, with:

fontSynthesis.GetUnit() != eCSSUnit_Enumerated ||
fontSynthesis.GetIntValue() != ...

which avoids needing the extra parentheses, and is also more resistant to future values.

You also need the same change for fontKerning.

r=dbaron with both of those changes.

Comment 4

[John Daggett (:jtd)]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #3)
> Comment on attachment 751591 [details] [diff] [review]

> You also need the same change for fontKerning.

The font-kerning property has values 'auto | normal | none' but uses an enumerated table, so there's no need for logic similar to font-stretch/font-synthesis.  It would probably be more consistent to switch from VARIANT_HK to (VARIANT_AUTO | VARIANT_NORMAL | VARIANT_NONE).  Let me know if you think it's important and I'll make a follow-up patch.

Comment 5

[John Daggett (:jtd)]

Pushed to inbound (including testcase as crashtest):
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d831fcaf4f9

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7d831fcaf4f9

Comment 7

[John Daggett (:jtd)]

Comment on attachment 751591 [details] [diff] [review]
patch, get for the none unit type before fetching int value

[Approval Request Comment]
Bug caused by (feature/regressing bug #): font-variant support, bug 549861
User impact if declined: crash is possible (although unlikely)
Testing completed (on m-c, etc.): yes
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 8

[John Daggett (:jtd)]

Pushed to aurora
https://hg.mozilla.org/releases/mozilla-aurora/rev/cd11b56ef7c9

Comment 9

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

(In reply to John Daggett (:jtd) from comment #4)
> (In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from
> comment #3)
> > Comment on attachment 751591 [details] [diff] [review]
> 
> > You also need the same change for fontKerning.
> 
> The font-kerning property has values 'auto | normal | none' but uses an
> enumerated table, so there's no need for logic similar to
> font-stretch/font-synthesis.  It would probably be more consistent to switch
> from VARIANT_HK to (VARIANT_AUTO | VARIANT_NORMAL | VARIANT_NONE).  Let me
> know if you think it's important and I'll make a follow-up patch.

OK, I guess if you want to assume that fontKerning is VARIANT_HK, you should add a MOZ_ASSERT(fontKerning.GetUnit() == eCSSUnit_Enumerated);
to document this (and make it clearer to anybody changing font-kerning code in the future).  But I think you should do something for font-kerning here.

Comment 10

[Paul Silaghi, QA [:pauly]]

Reproduced in 2013-05-16-mozilla-central-debug.
No assertion in 2013-08-23-mozilla-beta-debug.