1278463 - "ASSERTION: reading uninitialized value" when animating "stroke-dasharray: context-value"

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: reading uninitialized value: 'mUnit != eStyleUnit_Null', file nsStyleCoord.h, line 136

Assertion failure: false (unexpected unit), at layout/style/StyleAnimationValue.cpp:3804


Security-sensitive for now, because the first assertion message suggests a memory safety violation.

Comment 1

[Jesse Ruderman]

Attachment: stack for the second assertion

Comment 2

[Daniel Holbert [:dholbert]]

I debugged this a bit, with rr. The problem seems to be that this special keyword value for "stroke-dasharray" doesn't round-trip correctly through ExtractComputedValue/UncomputeValue.

Initially we have an empty array, which makes us take this "else" path in ExtractComputedValue:
> 3777           if (!svg->mStrokeDasharray.IsEmpty()) {
[...]
> 3808           } else {
> 3809             result = new nsCSSValueList;
> 3810             result->mValue.SetNoneValue();
http://mxr.mozilla.org/mozilla-central/source/layout/style/StyleAnimationValue.cpp?rev=885f01dee6d2#3808

So this produces a 1-length None-valued nsCSSValueList (which is notably different from what the CSS parser produces to represent this keyword).  Later on, that 1-length list gets computed into a 1-length nsTArray, and we end up hitting the above-quoted code again, and we take the *other* branch (i.e. we enter the "if" case instead of the "else" case), and we get confused & trip some assertions because we've got a dummy value in our array.

Comment 3

[Daniel Holbert [:dholbert]]

The code quoted in comment 2 dates back to...
 https://hg.mozilla.org/mozilla-central/rev/1c1825d6960d
...which changed to make us represent "stroke-dasharray: none" with a list that contained a single None-valued nsCSSValueList.  We used a "dummy" list instead of a single-value because UncomputeValue was rather Unforgiving at the time, as described in bug 524539 comment 1 -- it had only a few hardcoded properties that it expected "None" to be associated with. However, nowadays, UncomputeValue doesn't have any property restrictions on None values.

So, we can go back to just using a single "None"-united value to represent "None" here, and similarly we can use a single enum-united value to represent "context-value".

Comment 4

[Daniel Holbert [:dholbert]]

[CC'ing birtles, as I intend to tag him for review]

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #3)
> [...] changed to make us represent "stroke-dasharray: none" with a list
> that contained a single None-valued nsCSSValueList.  We used a "dummy" list
> instead of a single-value because UncomputeValue was rather Unforgiving at
> the time

Ah, we actually had another (better) reason for using a "dummy" list to represent "stroke-dasharray:none" -- we did that because it matched what the CSS parser produced to represent the same thing.  Here's the nsCSSParser code for parsing "stroke-dasharray:none", from that point in time:
> PRBool
> CSSParserImpl::ParseDasharray()
> {
>   nsCSSValue value;
>   if (ParseVariant(value, VARIANT_HLPN | VARIANT_NONE, nsnull)) {
>    nsCSSValueList *listHead = new nsCSSValueList;
>    nsCSSValueList *list = listHead;
> [...]
>    list->mValue = value;
https://hg.mozilla.org/mozilla-central/annotate/1c1825d6960d/layout/style/nsCSSParser.cpp#l9007

That changed, though (quite a while ago!!) here, in bug 576044:
https://hg.mozilla.org/mozilla-central/rev/b88472b0af90#l5.1958

Ever since that change, nsCSSParser represents "none" using a single "none"-unit nsCSSValue (and that's also what nsRuleNode.cpp expects when producing a computed "none" value, as a result).

So, ever since that change, StyleAnimation representation of a "none" specified value has been out of sync from the CSS parser's representation, and its "dummy"-list-valued-nsCSSValue representation has potentially given broken/bad behavior when fed into nsRuleNode::ComputeSVGData to produce a computed value.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: fix v1

This patch does two things:
 (1) It reverts http://hg.mozilla.org/mozilla-central/rev/1c1825d6960d (the change linked in comment 3) -- changing our representation of "None" back to a single nsCSSValue with unit "none" -- which is good & desirable, because it's bringing StyleAnimation's specified-style representation for "none" here back into alignment with what nsCSSParser produces. (after nsCSSParser changed in bug 576044 as noted above)

 (2) It makes this code recognize the nsStyleSVG representation of "stroke-dasharray:context-value".  The new code I'm adding for that is basically just reversing the nsCSSValue-to-nsStyleSVG conversion code here, in nsRuleNode: http://mxr.mozilla.org/mozilla-central/source/layout/style/nsRuleNode.cpp?rev=9adc60621a4a?mark=9421-9426#9421

Comment 7

[Brian Birtles (:birtles)]

Thanks for this. I'll have a look at this properly later. I have a similar patch in bug 1276688 (which I didn't want to land until the rest of that bug lands on beta/aurora) but this patch is probably better.

Comment 8

[Brian Birtles (:birtles)]

Comment on attachment 8762217 [details] [diff] [review]
fix v1

Review of attachment 8762217 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay on this. This looks great. Might be nice to add a regression test once this has landed (and be uplifted if necessary).

Comment 9

[Daniel Holbert [:dholbert]]

Thanks! Yeah, we should definitely add a crashtest once this bug can be opened.

Comment 10

[Daniel Holbert [:dholbert]]

This affects Aurora and Beta (same assertion/abort as comment 0):
http://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-beta-linux64-asan-debug/1465911146/
http://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-aurora-linux64-asan-debug/1465491331/

...but not release -- no assertion/abort there:
http://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-release-linux64-asan-debug/1465640124/

Comment 11

[Daniel Holbert [:dholbert]]

For the record -- I was discussing this bug with birtles at breakfast (and how much to worry about being prepared to backport before landing, in light of comment 10).  He said he suspected this bug might have been worked around by bug 1276688 (in which case we wouldn't have to care much about backporting here, since that bug's likely being backported).

Unfortunately, that's not *quite* the case -- I still get both assertions from comment 0 in a recent mozilla-central debug build: http://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-central-linux64-asan-debug/1465968326/  So bug 1276688 didn't directly fix this.

However, I think we still don't have to worry about backporting, because:
 (1) Nothing goes directly wrong as a result of this bug:
  (1a) The first (nonfatal) assertion, about "reading uninitialized value", isn't actually a memory-safety violation. The language in that assertion is misleading -- it really means "Reading the unit from a nsStyleCoord which hasn't been given a useful value yet".  The data that we're reading there -- mUnit -- *is* memory that's been "initialized" -- it's just been initialized to eStyleUnit_Null.  So, there's no memory-safety violation there.
  (1b) The second (fatal) assertion has a "return false" failure case immediately after it, which should propagate up to the caller (the client of StyleAnimationValue::ComputeValue).  So as long as the caller checks for success/failure, we should bail nicely/safely.

 (2) It was possible for things to go wrong *indirectly* as a result of this bug, if clients assume that StyleAnimationValue::ComputeValue succeeds.  That was the root cause of bug 1276688 (via an indirect dependence on the result of a ComputeValue call, I think).  It seems birtles has relaxed that assumption there, though, and I don't see any other cases where we're obviously making that assumption, from a quick skim of calls to ComputeValues, so I think we're OK.

SO. I think we're OK to not worry about backporting this bug. (Might be worth backporting to Aurora since the patch is simple/safe enough and fixes an abort, but I'd punt on backporting to beta.)

However, it's still perhaps worth avoiding landing/opening this bug, since it's related to bug 1276688 which is security-sensitive & not yet fixed on all branches, so maybe we should avoid leaking information about anything related to that bug.

Comment 12

[Brian Birtles (:birtles)]

That matches my understanding of the situation.

Comment 13

[Daniel Holbert [:dholbert]]

I think we can un-hide this bug, since bug 1276688 is now fixed on all affected branches, and we don't know of a way for anything bad to happen here in a build that has bug 1276688 fixed.

I'll un-hide this later today, and I'll go ahead and land the patch here (with a crashtest) at the same time.

Comment 14

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f40699e76a9

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4f40699e76a9