Closed
Bug 873222
Opened 11 years ago
Closed 11 years ago
"ABORT: not an int value" serializing font and font-synthesis properties
Categories
(Core :: CSS Parsing and Computation, defect)
Tracking
()
VERIFIED
FIXED
mozilla25
Tracking | Status | |
---|---|---|
firefox24 | --- | verified |
People
(Reporter: jruderman, Assigned: jtd)
References
Details
(Keywords: assertion, testcase)
Attachments
(3 files)
256 bytes,
text/html
|
Details | |
12.30 KB,
text/plain
|
Details | |
1.57 KB,
patch
|
dbaron
:
review+
bajaj
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
###!!! 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.
Reporter | ||
Comment 1•11 years ago
|
||
Assignee | ||
Comment 2•11 years ago
|
||
Attachment #751591 -
Flags: review?(dbaron)
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 | ||
Updated•11 years ago
|
Assignee: nobody → jdaggett
Assignee | ||
Comment 4•11 years ago
|
||
(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.
Assignee | ||
Comment 5•11 years ago
|
||
Pushed to inbound (including testcase as crashtest): https://hg.mozilla.org/integration/mozilla-inbound/rev/7d831fcaf4f9
Comment 6•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7d831fcaf4f9
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Assignee | ||
Comment 7•11 years ago
|
||
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?
Assignee | ||
Updated•11 years ago
|
status-firefox24:
--- → affected
Updated•11 years ago
|
Attachment #751591 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 8•11 years ago
|
||
Pushed to aurora https://hg.mozilla.org/releases/mozilla-aurora/rev/cd11b56ef7c9
(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•11 years ago
|
||
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.
Description
•