Last Comment Bug 709907 - "ASSERTION: Calling InvertSign with an unsupported unit"
: "ASSERTION: Calling InvertSign with an unsupported unit"
Status: RESOLVED FIXED
: assertion, testcase
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Daniel Holbert [:dholbert]
:
:
Mentors:
Depends on: 523355
Blocks: 344905
  Show dependency treegraph
 
Reported: 2011-12-12 12:04 PST by Jesse Ruderman
Modified: 2011-12-16 05:56 PST (History)
4 users (show)
dholbert: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (105 bytes, image/svg+xml)
2011-12-12 12:04 PST, Jesse Ruderman
no flags Details
stack trace (2.02 KB, text/plain)
2011-12-12 12:05 PST, Jesse Ruderman
no flags Details
fix v1 (2.40 KB, patch)
2011-12-13 13:32 PST, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
fix v1 (diff -w version) (2.10 KB, patch)
2011-12-13 13:33 PST, Daniel Holbert [:dholbert]
dbaron: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2011-12-12 12:04:49 PST
Created attachment 580998 [details]
testcase

###!!! ASSERTION: Calling InvertSign with an unsupported unit: 'Not Reached', file content/smil/nsSMILCSSValueType.cpp, line 154
Comment 1 Jesse Ruderman 2011-12-12 12:05:09 PST
Created attachment 580999 [details]
stack trace
Comment 2 Daniel Holbert [:dholbert] 2011-12-13 12:59:34 PST
The unsupported unit in question is "eUnit_Dasharray", added in bug 523355, which nsSMILCSSValueType::InvertSign doesn't know about.

Rather than trying to teach nsSMILCSSValueType::InvertSign about the inner-workings of dash-array-valued nsStyleAnimation::Value objects, I think we should move InvertSign into nsStyleAnimation::Value (where it'll have access to private data, and where it'll be clearer that it needs updating if numeric/number-like types are added/modified in the future.
Comment 3 Daniel Holbert [:dholbert] 2011-12-13 13:18:35 PST
Actually, I take that back -- I think we need to explicitly opt out stroke-dasharray from this negative-checking / inverting hack right now.

Basically what we do now is:
  if (value looks like "-[digit][anything]") {
    Strip off the "-" and try to parse the string after that as our property-value.
    If that parse succeeded {
       try to negate the resulting parsed value
    } else, parse failed --> fail.

This isn't sensitive enough for stroke-dasharray, though.  Supposing someone wants to animate by "-1, 2, 3, 4", then the above logic would end up trying to parse the value "1, 2, 3, 4", and then we'd "negate" that and use the resulting value.  But there's no clear way to negate "1, 2, 3, 4" (which values should we negate?)

For stroke-dasharray, we really need to do this checking / negation per-array-value, which our current logic doesn't really support.  So I think we should just opt stroke-dasharray out of this chunk.
Comment 4 Daniel Holbert [:dholbert] 2011-12-13 13:32:22 PST
Created attachment 581400 [details] [diff] [review]
fix v1

As the comment at the top of the contextual code hints, bug 501188 would really the cleanest way to fix this -- given that, I don't think it's worth trying to add hackarounds to make stroke-dasharray work with negative values here.

So, this patch just disables the negative-value-screening logic for stroke-dasharray (so we pass the negative value along to the parser, and it'll fail to parse, which is probably fine).

Just for reference, in case it's not clear -- negative numbers would in theory be useful for something like:
  <rect stroke-dasharray="10, 10" ...>
    <animate attributeName="stroke-dasharray" by="-2, -2" ..>
  </rect>
The author's intention there would be to get a resulting animation towards "8, 8", but we'll fail to animate that right now, because "-2, -2" is an invalid value for stroke-dahsarray.
Comment 5 Daniel Holbert [:dholbert] 2011-12-13 13:33:21 PST
Created attachment 581401 [details] [diff] [review]
fix v1 (diff -w version)
Comment 6 David Baron :dbaron: ⌚️UTC-7 2011-12-13 14:44:57 PST
Comment on attachment 581401 [details] [diff] [review]
fix v1 (diff -w version)

r=dbaron, though I'm not sure if I'm the best reviewer for this
Comment 7 Daniel Holbert [:dholbert] 2011-12-13 15:55:40 PST
(In reply to David Baron [:dbaron] from comment #6)
> r=dbaron

Thanks! :)

> though I'm not sure if I'm the best reviewer for this

I'm not concerned -- I mostly wanted a sanity-check, and I picked you since you wrote the code to add the unit in question ("eUnit_Dasharray", bug 523355), in case there was something obvious about it that I was missing.
Comment 8 Daniel Holbert [:dholbert] 2011-12-15 16:30:50 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/b278df809c4b
Comment 9 :Ms2ger (⌚ UTC+1/+2) 2011-12-16 05:56:42 PST
https://hg.mozilla.org/mozilla-central/rev/b278df809c4b

Note You need to log in before you can comment on or make changes to this bug.