Closed Bug 873222 Opened 6 years ago Closed 6 years ago

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

Categories

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

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla25
Tracking Status
firefox24 --- verified

People

(Reporter: jruderman, Assigned: jtd)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, testcase)

Attachments

(3 files)

Attached file 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.
Attached file stack+ (gdb)
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.
Attachment #751591 - Flags: review?(dbaron) → review+
Assignee: nobody → jdaggett
(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.
Pushed to inbound (including testcase as crashtest):
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d831fcaf4f9
https://hg.mozilla.org/mozilla-central/rev/7d831fcaf4f9
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
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
Attachment #751591 - Flags: approval-mozilla-aurora?
Attachment #751591 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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.
Reproduced in 2013-05-16-mozilla-central-debug.
No assertion in 2013-08-23-mozilla-beta-debug.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.