Last Comment Bug 696188 - Leak nsCSSValueList with multi-transform and transition
: Leak nsCSSValueList with multi-transform and transition
Status: RESOLVED FIXED
[MemShrink:P3]
: mlk, testcase
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla10
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on:
Blocks: randomstyles 369230 505115 531344
  Show dependency treegraph
 
Reported: 2011-10-20 13:06 PDT by Jesse Ruderman
Modified: 2011-11-01 14:41 PDT (History)
9 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (408 bytes, text/html)
2011-10-20 13:06 PDT, Jesse Ruderman
no flags Details
leak stats (1.01 KB, text/plain)
2011-10-20 13:07 PDT, Jesse Ruderman
no flags Details
Fix (2.08 KB, patch)
2011-10-24 12:37 PDT, Boris Zbarsky [:bz]
dholbert: review+
christian: approval‑mozilla‑aurora-
Details | Diff | Review
interactive testcase for functionality (352 bytes, text/html)
2011-10-25 16:00 PDT, Daniel Holbert [:dholbert]
no flags Details
followup: add assertions & functional test (3.13 KB, patch)
2011-10-25 16:56 PDT, Daniel Holbert [:dholbert]
bzbarsky: review+
Details | Diff | Review

Description Jesse Ruderman 2011-10-20 13:06:13 PDT
Created attachment 568489 [details]
testcase

This testcase leaks a total of 392 bytes:

6  nsCSSValue::Array  
4  nsCSSValueList     
2  nsCSSValueList_heap
3  nsStringBuffer
Comment 1 Jesse Ruderman 2011-10-20 13:07:03 PDT
Created attachment 568491 [details]
leak stats

(output from running a debug build with XPCOM_MEM_LEAK_LOG=2)
Comment 2 Boris Zbarsky [:bz] 2011-10-24 12:25:50 PDT
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 [:bz] 2011-10-24 12:37:13 PDT
Created attachment 569138 [details] [diff] [review]
Fix

I made sure the crashtest fails without this patch and passes with this patch
Comment 4 Boris Zbarsky [:bz] 2011-10-24 12:45:10 PDT
We should take this on branches too, I think...
Comment 5 Daniel Holbert [:dholbert] 2011-10-24 16:42:44 PDT
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] 2011-10-24 18:21:04 PDT
(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 8 Boris Zbarsky [:bz] 2011-10-25 10:02:51 PDT
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] 2011-10-25 16:00:39 PDT
Created attachment 569534 [details]
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] 2011-10-25 16:56:07 PDT
Created attachment 569548 [details] [diff] [review]
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] 2011-10-25 18:02:09 PDT
https://hg.mozilla.org/mozilla-central/rev/aa3e0cb8b251
Comment 12 Boris Zbarsky [:bz] 2011-10-25 20:35:53 PDT
Comment on attachment 569548 [details] [diff] [review]
followup: add assertions & functional test

r=me
Comment 13 Daniel Holbert [:dholbert] 2011-10-27 16:00:51 PDT
Landed followup: https://hg.mozilla.org/integration/mozilla-inbound/rev/6181f480fcea
Comment 14 Ed Morley [:emorley] 2011-10-28 04:29:37 PDT
Followup: https://hg.mozilla.org/mozilla-central/rev/6181f480fcea
Comment 15 christian 2011-11-01 14:41:10 PDT
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.

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