1307961 - Require consistent bloatview reporting

Status: REOPENED

(Core :: XPCOM, defect, P3)

Description

[Andrew McCreight [:mccr8]]

You can add things to the bloatview via refcounting or MOZ_COUNT_CTOR/DTOR. There's no good reason to allow this, so we should dynamically track that these are being used consistently, by adding a new field to BloatEntry and asserting that it is consistent, like is done for the class size. Maybe this will turn up some oddness that will fix bug 1271182.

Comment 1

[Andrew McCreight [:mccr8]]

I wonder if we have a static analysis for this already. I'm surprised I was able to start up the browser without hitting my assertion.

Comment 2

[Andrew McCreight [:mccr8]]

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=846fe4bc1159b56a6a2c330aae628f3e4ad2f900

Phew, there is plenty of orange.

Comment 3

[Andrew McCreight [:mccr8]]

Ok, there's tons of classes that fail immediately on startup. I must not have actually tested this with a correct build. This is common enough I should probably add static asserts.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: Require consistent usage of bloat reporting.

"Am I so out of touch? No, it's the children who are wrong."

Comment 5

[Andrew McCreight [:mccr8]]

I added a static assert that a class reported to COUNT_CTOR or DTOR must not be a subclass of nsISupports, and it finds a lot of stuff.

Comment 6

[Andrew McCreight [:mccr8]]

This might be nice, but there are too many places that violate this rule, and I'm not sure if this would really improve anything.

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: Require consistent usage of bloat reporting.

I've kind of given up on this, but if anybody else wants to look at it, this is what I came up with.

Comment 9

[Nathan Froyd [:froydnj]]

Reminding myself to look at this.

Comment 10

[Nathan Froyd [:froydnj]]

Attachment: require consistent bloatview reporting

I didn't have to do very much here; I think there was some dead code in
nsTraceRefcnt.cpp and then there were only two or three extra places that
needed MOZ_COUNT_{C,D}TOR to be removed.

I feel sort of silly asking you for review, since you wrote pretty much the
entire patch: we can land this in your name with me as the reviewer, sound
good?

Comment 11

[Andrew McCreight [:mccr8]]

Comment on attachment 8817485 [details] [diff] [review]
require consistent bloatview reporting

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

Thanks for finishing this up.

::: layout/style/FontFace.cpp
@@ +108,5 @@
>    , mSourceBufferLength(0)
>    , mFontFaceSet(aFontFaceSet)
>    , mInFontFaceSet(false)
>  {
> +  nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(aParent);

I assume this is some rebase error, by either you or me, and this should all be deleted.

::: xpcom/reflect/xptcall/xptcall.cpp
@@ +27,5 @@
>  
>  NS_IMETHODIMP_(MozExternalRefCountType)
>  nsXPTCStubBase::AddRef()
>  {
> +    // XXX Do log addref here.

This probably needs to get fixed? I don't remember if I wrote this or not.

Comment 12

[Andrew McCreight [:mccr8]]

(In reply to Nathan Froyd [:froydnj] from comment #10)
> I feel sort of silly asking you for review, since you wrote pretty much the
> entire patch: we can land this in your name with me as the reviewer, sound
> good?

That sounds fine to me.

Comment 13

[Nathan Froyd [:froydnj]]

(In reply to Andrew McCreight [:mccr8] from comment #11)
> ::: layout/style/FontFace.cpp
> @@ +108,5 @@
> >    , mSourceBufferLength(0)
> >    , mFontFaceSet(aFontFaceSet)
> >    , mInFontFaceSet(false)
> >  {
> > +  nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(aParent);
> 
> I assume this is some rebase error, by either you or me, and this should all
> be deleted.

Ah, yes.

> ::: xpcom/reflect/xptcall/xptcall.cpp
> @@ +27,5 @@
> >  
> >  NS_IMETHODIMP_(MozExternalRefCountType)
> >  nsXPTCStubBase::AddRef()
> >  {
> > +    // XXX Do log addref here.
> 
> This probably needs to get fixed? I don't remember if I wrote this or not.

I'm just going to remove these XXX comments; I don't think it's worth tracking what happens to these stubs.  The only use in Firefox comes from an RAII helper.  If we wanted to MOZ_COUNT_{C,D}TOR the stub, I guess we could do that...

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aba6c73511a2
require consistent bloatview reporting for nsISupports classes; r=froydnj

Comment 15

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8acf32ac3d00
Backout aba6c73511a2 for massive test bustage resulting in a CLOSED TREE; r=alltheorange

Comment 16

[Nathan Froyd [:froydnj]]

OK, so I didn't think through the implications of the dynamic analysis in nsTraceRefcnt.cpp.  That code means that we should probably just land the nsISupports stuff so the problem doesn't get any worse, and then deal with general AddRef/Release.  I think I will do the first part in a separate bug, and deal with the general case here.

A small case could be made for allowing MOZ_COUNT_CTOR on addref'd classes: what if you create a class and then forget to do anything with it?  I feel like the possibilities of leaks like that are reasonably small nowadays, so it doesn't seem like a case worth supporting.

Comment 17

[(no longer active)]

(In reply to Nathan Froyd [:froydnj] from comment #16)
> A small case could be made for allowing MOZ_COUNT_CTOR on addref'd classes:
> what if you create a class and then forget to do anything with it?  I feel
> like the possibilities of leaks like that are reasonably small nowadays, so
> it doesn't seem like a case worth supporting.

Hmm, like this?

RefCountedFoo* foo = new RefCountedFoo();
// Forget |foo|

We should probably just outright ban assigning the return value of a new RefCountedFoo() to anything other than a RefPtr.  I wonder how many places in the code we need to fix for that...

Comment 18

[Nathan Froyd [:froydnj]]

(In reply to :Ehsan Akhgari from comment #17)
> Hmm, like this?
> 
> RefCountedFoo* foo = new RefCountedFoo();
> // Forget |foo|
> 
> We should probably just outright ban assigning the return value of a new
> RefCountedFoo() to anything other than a RefPtr.  I wonder how many places
> in the code we need to fix for that...

Indeed!  Bug 1323455 has been filed for this static analysis.

Comment 19

[Andrew Overholt [:overholt]]

Nathan, do you know what we're planning to do here? I ask because Ehsan told me you'd rather land this than the patch in bug 1321338 and that bug is tracked as a regression so it's on various radars. Thanks!

Comment 20

[Nathan Froyd [:froydnj]]

(In reply to Andrew Overholt [:overholt] from comment #19)
> Nathan, do you know what we're planning to do here? I ask because Ehsan told
> me you'd rather land this than the patch in bug 1321338 and that bug is
> tracked as a regression so it's on various radars. Thanks!

I have a patch; I was going to hold off landing in until the static analysis from comment 17 and comment 18 landed, but I can land it sooner if that would make people's lives easier.

Bug 1321338 is a regression, but it's only a regression for debug builds, which we don't ship to users.  We could try to backport patches, I suppose, but I don't know how useful that would be.

Comment 21

[(no longer active)]

(In reply to Nathan Froyd [:froydnj] from comment #20)
> (In reply to Andrew Overholt [:overholt] from comment #19)
> > Nathan, do you know what we're planning to do here? I ask because Ehsan told
> > me you'd rather land this than the patch in bug 1321338 and that bug is
> > tracked as a regression so it's on various radars. Thanks!
> 
> I have a patch; I was going to hold off landing in until the static analysis
> from comment 17 and comment 18 landed, but I can land it sooner if that
> would make people's lives easier.
> 
> Bug 1321338 is a regression, but it's only a regression for debug builds,
> which we don't ship to users.  We could try to backport patches, I suppose,
> but I don't know how useful that would be.

I doubt backporting a fix is worth anyone's time honestly...  Perhaps we should close bug 1321338 to get it off people's radars for now while this is in flux?

Comment 23

[Julien Cristau [:jcristau]]

Mass wontfix for bugs affecting firefox 52.