1291885 - stylo: Fix various leaks

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Bobby Holley (:bholley)]

I noticed we have some leaks reported when running reftests. It turns out that there were a couple of issues, which I've got patches for here and on the Servo side.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Rejigger init hook and add shutdown hook. v1

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Use dont_AddRef for already-addrefed ServoComputedValues. v1

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 3 - Use leak logging in a few more places. v1

Comment 4

[Bobby Holley (:bholley)]

This requires corresponding servo code in https://github.com/servo/servo/pull/12725

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8777545 [details] [diff] [review]
Part 2 - Use dont_AddRef for already-addrefed ServoComputedValues. v1

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

::: layout/style/ServoStyleSet.cpp
@@ +350,5 @@
>    MOZ_ASSERT(aType < CSSPseudoElementType::Count);
>    nsIAtom* pseudoTag = nsCSSPseudoElements::GetPseudoAtom(aType);
>  
>    RefPtr<ServoComputedValues> computedValues =
> +    dont_AddRef(Servo_GetComputedValuesForPseudoElement(

Hmm... Seeing this makes me sad, we should really tackle bug 1275913. Though in practice I don't think it'd be a big deal once we have leak checks?

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8777546 [details] [diff] [review]
Part 3 - Use leak logging in a few more places. v1

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

::: layout/style/nsStyleContext.cpp
@@ +91,5 @@
>  #ifdef DEBUG
>    , mFrameRefCnt(0)
>    , mComputingStruct(nsStyleStructID_None)
>  #endif
> +{ MOZ_COUNT_CTOR(nsStyleContext); }

Maybe putting this in its own line?
{
  MOZ_COUNT_CTOR(nsStyleContext);
}

Comment 7

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #5)
> Hmm... Seeing this makes me sad, we should really tackle bug 1275913. Though
> in practice I don't think it'd be a big deal once we have leak checks?

I think it's definitely still important and that we should fix it ASAP.

It's might be kind of up Manish's alley given the other FFI safety stuff he's been working on. Manish, any chance you might have the cycles to fix our already_AddRefed story at some point?

Comment 8

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #6)
> Comment on attachment 8777546 [details] [diff] [review]
> Part 3 - Use leak logging in a few more places. v1
> 
> Review of attachment 8777546 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/style/nsStyleContext.cpp
> @@ +91,5 @@
> >  #ifdef DEBUG
> >    , mFrameRefCnt(0)
> >    , mComputingStruct(nsStyleStructID_None)
> >  #endif
> > +{ MOZ_COUNT_CTOR(nsStyleContext); }
> 
> Maybe putting this in its own line?
> {
>   MOZ_COUNT_CTOR(nsStyleContext);
> }

Done.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a8f0dbcb786

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/867f638fcc50
Rejigger init hook and add shutdown hook. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/14c9cd79e79e
Use dont_AddRef for already-addrefed ServoComputedValues. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/2dbf104fa4b4
Use leak logging in a few more places. r=emilio

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/867f638fcc50
https://hg.mozilla.org/mozilla-central/rev/14c9cd79e79e
https://hg.mozilla.org/mozilla-central/rev/2dbf104fa4b4

Comment 11

[Manish Goregaokar [:manishearth]]

> It's might be kind of up Manish's alley given the other FFI safety stuff
> he's been working on. Manish, any chance you might have the cycles to fix
> our already_AddRefed story at some point?

At some point -- probably, but not too soon. I can prioritize it if it needs to be though.

I'm also not yet clear on how we're using it in Servo at the moment, so I'd have to investigate that.

Leaving this needinfo open till I have a closer look.