1266868 - The test_transitions_per_property.html mochitests that landed for clip-path basic shapes leaks

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

The tests for "clip-path" basic shapes (added in bug 1110460) seem to fail & leak when they're enabled, as shown in the Linux x64 debug mochitest-9 failures in this TreeHerder run:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=78c0ed233443

To reproduce this locally, you can import the patch from bug 1247229, or you can add...
>  user_pref("layout.css.clip-path-shapes.enabled", true);
...to testing/profiles/prefs_general.js, and then run:
./mach mochitest layout/style/test/test_transitions_per_property.html


Sample failure log:
745 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | unexpected reference box found
746 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | Reference boxes should match - got "border-box", expected 200px,at,200px,200px
748 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | uncaught exception - TypeError: expectedList[1] is undefined at test_clip_path_equals@http://mochi.test:8888/tests/layout/style/test/test_transitions_per_property.html:1373:1
TEST-UNEXPECTED-FAIL | leakcheck | tab process: 14728 bytes leaked (nsCSSValue::Array, nsCSSValuePair, nsCSSValuePairList, nsCSSValuePairList_heap, nsCSSValuePair_heap)

https://treeherder.mozilla.org/logviewer.html#?job_id=26268831&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=26270178&repo=mozilla-inbound

Comment 1

[Daniel Holbert [:dholbert]]

(Hoping jwatt can investigate. I suspect this might be the only thing that should block us from enabling the pref by default in non-release builds, though I'm not sure.)

Comment 2

[Jonathan Watt [:jwatt]]

Yeah, this is what I filed bug 1266316 for. I'd anticipate fixing and properly enabling that test over in that bug at the time we change the code to conform to whatever decision is made regarding the spec.

Comment 3

[Jonathan Watt [:jwatt]]

Let's address the leaks in this bug.

There are two issues.

nsCSSValue::SetPairValue does not take ownership of the passed in value, it copies it. Therefore the two points in StyleAnimationValue that hold a UniquePtr<nsCSSValuePair> and then .release() it to pass the pointer to nsCSSValue::SetPairValue are wrong. We should use .get() instead. (Note that code in nsCSSParser passes stack allocated objects to nsCSSValue::SetPairValue, so we can't change its semantics to make it take ownership of the passed object.)

The second issue is that our handling for StyleAnimationValue.mValue.mCSSValueArray is wrong. nsCSSValue::Array objects are reference counted so StyleAnimationValue::SetAndAdoptCSSValueArrayValue can't cake ownership as the comment there claims. We're also completely missing handling of this member in StyleAnimationValue::FreeValue.

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Jonathan Watt [:jwatt] from comment #3)
> nsCSSValue::SetPairValue does not take ownership of the passed in value, it
> copies it. Therefore the two points in StyleAnimationValue that hold a
> UniquePtr<nsCSSValuePair> and then .release() it to pass the pointer to
> nsCSSValue::SetPairValue are wrong. We should use .get() instead.

I think this means we shouldn't use UniquePtr here at all.  Since SetPairValue doesn't take ownership, that means we're doing heap allocation for an always-temporary variable -- and that's a bit silly.

So: I think we should change AddCSSValuePair() to return type "Maybe<nsCSSValuePair>" instead of "UniquePtr<nsCSSValuePair>".  I believe it should pretty much Just Work -- the MakeUnique call just needs to be converted to an "emplace" call, and the comments about "return result; // nullptr" need a s/nullptr/Nothing()/ tweak.


> (Note that
> code in nsCSSParser passes stack allocated objects to
> nsCSSValue::SetPairValue, so we can't change its semantics to make it take
> ownership of the passed object.)

(Gotcha. This makes some sense, given that internally nsCSSValue uses a refcounted flavor of nsCSSValuePair for its internal copy.)

> The second issue is that our handling for
> StyleAnimationValue.mValue.mCSSValueArray is wrong.

Yikes! Thanks for catching & fixing that.

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8745012 [details] [diff] [review]
patch

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

::: layout/style/StyleAnimationValue.cpp
@@ +2010,3 @@
>          const nsCSSValuePair& pair2 = radii2->Item(i).GetPairValue();
>          UniquePtr<nsCSSValuePair>
>            pairResult(AddCSSValuePair(aProperty, restrictions,

As noted above, pairResult should be Maybe<nsCSSValuePair> here.

@@ +2014,5 @@
>                                       aCoeff2, &pair2));
>          if (!pairResult) {
>            return nullptr;
>          }
> +        resultRadii->Item(i).SetPairValue(pairResult.get());

Once you're using Maybe<>, I think you can just directly pass "pairResult" here and it'll cast to a pointer automatically. 

(".ptr()" works as the explicit pointer-value getter, but only use that if you really need it -- and I don't think you need to here.)

@@ +3477,4 @@
>        RefPtr<nsCSSValue::Array> radiusArray = nsCSSValue::Array::Create(4);
>        const nsStyleCorners& radii = shape->GetRadius();
>        NS_FOR_CSS_FULL_CORNERS(corner) {
>          auto pair = MakeUnique<nsCSSValuePair>();

This should be a stack-allocated nsCSSValuePair. No need for Maybe<> or UniquePtr<> here.

@@ +3483,5 @@
>              !StyleCoordToCSSValue(radii.Get(NS_FULL_TO_HALF_CORNER(corner, true)),
>                                    pair->mYValue)) {
> +          return false;
> +        }
> +        radiusArray->Item(corner).SetPairValue(pair.get());

...and so this should just be "&pair".

@@ +4408,1 @@
>  StyleAnimationValue::SetAndAdoptCSSValueArrayValue(nsCSSValue::Array* aValue,

For consistency, this needs to be renamed to just "SetCSSValueArrayValue" -- i.e. drop the "AndAdopt". All of the "SetAndAdopt*" methods take ownership of a uniquely-owned value.

(And the one other setter that we have which uses AddRef() -- StyleAnimationValue::SetTransformValue -- does *not* have "Adopt" in its name.)

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: part 1 - Fix leaks in the nsCSSValue::Array handling in the StyleAnimationValue code

Comment 8

[Jonathan Watt [:jwatt]]

Attachment: part 2 - Fix leaks in the nsCSSValuePair handling in the StyleAnimationValue code

Comment 9

[Jonathan Watt [:jwatt]]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> Once you're using Maybe<>, I think you can just directly pass "pairResult"
> here and it'll cast to a pointer automatically. 
> 
> (".ptr()" works as the explicit pointer-value getter, but only use that if
> you really need it -- and I don't think you need to here.)

I do seem to need the .ptr() call here FWIW.

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8747784 [details] [diff] [review]
part 1 - Fix leaks in the nsCSSValue::Array handling in the StyleAnimationValue code

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

r=me, one nit:

::: layout/style/StyleAnimationValue.cpp
@@ +4510,1 @@
>                                                     Unit aUnit)

Please fix indentation before "Unit" here (in StyleAnimationValue::SetCSSValueArrayValue function signature).

Comment 11

[Daniel Holbert [:dholbert]]

(/me shakes fist at Splinter for showing zero lines of context for review comments.)

Comment 12

[Jonathan Watt [:jwatt]]

I think I found it. ;)

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 8747786 [details] [diff] [review]
part 2 - Fix leaks in the nsCSSValuePair handling in the StyleAnimationValue code

># HG changeset patch
># User Jonathan Watt <[e-mail]>
># Parent  730060ecb106b7e71ee542e3c46fbceaf5b79874
>Bug 1266868, part 2 - Fix leaks in the nsCSSValuePair handling in the StyleAnimationValue code. r=dholbert

This commit message doesn't look correct -- this patch isn't fixing any leaks, AFAICT -- it's just making us use stack allocation instead of heap allocation for temporary nsCSSValuePair objects in StyleAnimationValue.

So: please update the commit message along those lines.

r=me with that and with nits below addressed (taking advantage of the fact that Maybe<> lets us align with our Coding Style Guidelines better than UnqiuePtr does):

>@@ -2029,24 +2029,24 @@ AddShapeFunction(nsCSSProperty aProperty
>       resultFuncArgs->Item(5).SetArrayValue(resultRadii, eCSSUnit_Array);
>       // We use an arbitrary border-radius property here to get the appropriate
>       // restrictions for radii since this is a <border-radius> value.
>       uint32_t restrictions =
>         nsCSSProps::ValueRestrictions(eCSSProperty_border_top_left_radius);
>       for (size_t i = 0; i < 4; ++i) {
>         const nsCSSValuePair& pair1 = radii1->Item(i).GetPairValue();
>         const nsCSSValuePair& pair2 = radii2->Item(i).GetPairValue();
>-        UniquePtr<nsCSSValuePair>
>+        const Maybe<nsCSSValuePair>
>           pairResult(AddCSSValuePair(aProperty, restrictions,
>                                      aCoeff1, &pair1,
>                                      aCoeff2, &pair2));

Please change this to use "=" and move pairResult up to the first line, like so:
  const Maybe<nsCSSValuePair> pairResult =
    AddCSSValuePair(...

This brings us into alignment with a coding style guideline -- "In general, initialize variables with nsFoo aFoo = bFoo and not nsFoo aFoo(bFoo)".

I believe UniquePtr<> forced us to violate this guideline, which is why the code looks the way it does now. But Maybe<> is friendlier than UniquePtr in this respect -- so now that this is using Maybe<>, we can actually honor this guideline (which also makes the code more readable IMO).
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#CC_practices

>@@ -2400,25 +2400,27 @@ StyleAnimationValue::AddWeighted(nsCSSPr
>+      Maybe<nsCSSValuePair> result(
>         AddCSSValuePair(aProperty, restrictions,
>                         aCoeff1, aValue1.GetCSSValuePairValue(),
>                         aCoeff2, aValue2.GetCSSValuePairValue()));

Similarly: please change this to "result =".

>+      // We need a heap allocated object to adopt here:
>+      auto res = MakeUnique<nsCSSValuePair>(result.ref());

Two things:
 (1) ".ref()" doesn't seem to be needed here, according to my compiler. So: let's drop that.
 (2) "res" seems like an unnecessarily short/vague name for this variable. Consider something like "heapResult" (i.e. "a heap-allocated copy of |result|").

Comment 14

[Jonathan Watt [:jwatt]]

(In reply to Daniel Holbert [:dholbert] from comment #13)
> This commit message doesn't look correct -- this patch isn't fixing any
> leaks, AFAICT -- it's just making us use stack allocation instead of heap
> allocation for temporary nsCSSValuePair objects in StyleAnimationValue.

The two points in StyleAnimationValue that hold a UniquePtr<nsCSSValuePair> and then .release() it to pass the pointer to nsCSSValue::SetPairValue leak since nsCSSValue::SetPairValue does not take ownership of the passed in value.

Comment 15

[Jonathan Watt [:jwatt]]

(In reply to Daniel Holbert [:dholbert] from comment #13)
>  (1) ".ref()" doesn't seem to be needed here, according to my compiler. So:
> let's drop that.

And the .ref() is required by my compiler on mac so I'll have to leave that.

Comment 16

[Jonathan Watt [:jwatt]]

I've changed the comment to "Bug 1266868, part 2 - Fix leaks of the values passed to nsCSSValue::.SetPairValue in the StyleAnimationValue code. r=dholbert"

Comment 17

[Daniel Holbert [:dholbert]]

(In reply to Jonathan Watt [:jwatt] from comment #14)
> The two points in StyleAnimationValue that hold a UniquePtr<nsCSSValuePair>
> and then .release() it to pass the pointer to nsCSSValue::SetPairValue leak
> since nsCSSValue::SetPairValue does not take ownership of the passed in
> value.

Ah, right. OK, thanks for clarifying.

(In reply to Jonathan Watt [:jwatt] from comment #15)
> And the .ref() is required by my compiler on mac so I'll have to leave that.

Hmm! OK. Odd that that differs.

> (In reply to Jonathan Watt [:jwatt] from comment #16)
> I've changed the comment to "Bug 1266868, part 2 - Fix leaks of the values
> passed to nsCSSValue::.SetPairValue in the StyleAnimationValue code.
> r=dholbert"

Thanks, that's clearer. But I think you have a stray "." there, in "nsCSSValue::.SetPairValue".

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5fd4cf164841
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f25b4796da0

Comment 19

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #17)
> (In reply to Jonathan Watt [:jwatt] from comment #15)
> > And the .ref() is required by my compiler on mac so I'll have to leave that.
> 
> Hmm! OK. Odd that that differs.

Scratch that -- this doesn't differ, I just made a silly mistake in my local edits.

For future reference, though -- I'm just now recalling (from conversations with Seth) that "*foo" is preferred over "foo.ref()", for Maybe<> objects.   (They do the same thing, but "*foo" is shorter and easier to read -- and it matches the illusion that Maybe<> maintains about it being a 'pointer-like' object.)

Feel free to push a followup to tweak that if you like, but also not a big deal.

Comment 20

[Daniel Holbert [:dholbert]]

(That's why I was reacting against the "foo.ref()" -- I strongly recalled that it *shouldn't* be necessary, but I had temporarily forgotten what the actual correct replacement was.)

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d786a6a1f4cd

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5fd4cf164841
https://hg.mozilla.org/mozilla-central/rev/9f25b4796da0
https://hg.mozilla.org/mozilla-central/rev/d786a6a1f4cd