Closed Bug 696188 Opened 8 years ago Closed 8 years ago

Leak nsCSSValueList with multi-transform and transition


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






(Reporter: jruderman, Assigned: bzbarsky)


(Blocks 2 open bugs)


(Keywords: memory-leak, testcase, Whiteboard: [MemShrink:P3])


(5 files)

Attached file testcase
This testcase leaks a total of 392 bytes:

6  nsCSSValue::Array  
4  nsCSSValueList     
2  nsCSSValueList_heap
3  nsStringBuffer
Attached file leak stats
(output from running a debug build with XPCOM_MEM_LEAK_LOG=2)
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.
Assignee: nobody → bzbarsky
Blocks: 531344
Attached patch FixSplinter Review
I made sure the crashtest fails without this patch and passes with this patch
Attachment #569138 - Flags: review?(dholbert)
We should take this on branches too, I think...
Priority: -- → P1
Whiteboard: [need review]
Attachment #569138 - Flags: review?(dholbert) → review+
OS: Mac OS X → All
Hardware: x86_64 → All
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.
(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).
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla10
Comment on attachment 569138 [details] [diff] [review]

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.
Attachment #569138 - Flags: approval-mozilla-aurora?
Whiteboard: [MemShrink]
Whiteboard: [MemShrink] → [MemShrink:P3]
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).
Here's a followup patch, with 2 assertions about "resultTail" really being a tail, and a test for the functionality that was broken here.
Attachment #569548 - Flags: review?(bzbarsky)
Closed: 8 years ago
Resolution: --- → FIXED
Comment on attachment 569548 [details] [diff] [review]
followup: add assertions & functional test

Attachment #569548 - Flags: review?(bzbarsky) → review+
Comment on attachment 569138 [details] [diff] [review]

[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.
Attachment #569138 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
You need to log in before you can comment on or make changes to this bug.