696188 - Leak nsCSSValueList with multi-transform and transition

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase

This testcase leaks a total of 392 bytes:

6  nsCSSValue::Array  
4  nsCSSValueList     
2  nsCSSValueList_heap
3  nsStringBuffer

Comment 1

[Jesse Ruderman]

Attachment: leak stats

(output from running a debug build with XPCOM_MEM_LEAK_LOG=2)

Comment 2

[Boris Zbarsky [:bzbarsky]]

This is a bug in AddTransformLists; it doesn't advance resultTail all the way to the end of the list properly in the cases when it calls AddDifferentTransformLists.  In particular, it moves resultTail to the next to last slot, not the last one.

Looks like this was introduced back in bug 531344.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

I made sure the crashtest fails without this patch and passes with this patch

Comment 4

[Boris Zbarsky [:bzbarsky]]

We should take this on branches too, I think...

Comment 5

[Daniel Holbert [:dholbert]]

Tested this bug a little locally for rendering issues.  AFAICT, whenever we hit this bug, we end up rendering as if the -moz-transform property is unset, and its computed value is also the initial value ("none").

I'll be posting two functional regression-tests for this in a bit.  One is a reftest, to check the rendering visually, and the other is a mochitest, to be sure the computed style is set correctly.

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #5)
> AFAICT, whenever we
> hit this bug, we end up rendering as if the -moz-transform property is
> unset, and its computed value is also the initial value ("none").

er.. just kidding, that's expected -- because the attached testcase re-assigns the style attribute (blowing away the "-moz-transform" specification).  So, I haven't run across any behavioral effects of this bug yet (but I expect to find some).

Comment 7

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/aa3e0cb8b251

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 569138 [details] [diff] [review]
Fix

Requesting aurora approval.  This is a simple fix to not leak memory when appending linked lists to each other...

The only risk I can see here is that our old behavior dropped part of the transform list from the transition/animation interpolated values (incorrectly!) and that it's _possible_ that some site somewhere depends on that bug.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: interactive testcase for functionality

Here's a testcase that demonstrates the functionality aspect of this bug.

The part of the transform list that's dropped (per comment 8) is the translation component, so as shown here, when we transition from a matrix with a translation to "none", we get weird behavior (snapping back and forth).

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: followup: add assertions & functional test

Here's a followup patch, with 2 assertions about "resultTail" really being a tail, and a test for the functionality that was broken here.

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/aa3e0cb8b251

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 569548 [details] [diff] [review]
followup: add assertions & functional test

r=me

Comment 13

[Daniel Holbert [:dholbert]]

Landed followup: https://hg.mozilla.org/integration/mozilla-inbound/rev/6181f480fcea

Comment 14

[Ed Morley [:emorley]]

Followup: https://hg.mozilla.org/mozilla-central/rev/6181f480fcea

Comment 15

[LegNeato]

Comment on attachment 569138 [details] [diff] [review]
Fix

[triage comment]

The benefit doesn't really look to be here and we think it could gain from additional bake time (for finding potentially affected sites mentioned in comment 8, etc). We'll let this one ship through the normal process.