Open Bug 1307961 Opened 3 years ago Updated 2 years ago

Require consistent bloatview reporting

Categories

(Core :: XPCOM, defect, P3)

defect

Tracking

()

REOPENED
Tracking Status
firefox52 --- wontfix

People

(Reporter: mccr8, Unassigned)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
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.
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.
"Am I so out of touch? No, it's the children who are wrong."
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.
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.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → INCOMPLETE
Duplicate of this bug: 977380
I've kind of given up on this, but if anybody else wants to look at it, this is what I came up with.
Attachment #8798508 - Attachment is obsolete: true
Assignee: continuation → nobody
Status: RESOLVED → REOPENED
Resolution: INCOMPLETE → ---
Reminding myself to look at this.
Flags: needinfo?(nfroyd)
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?
Attachment #8817485 - Flags: review?(continuation)
Flags: needinfo?(nfroyd)
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.
Attachment #8817485 - Flags: review?(continuation) → review+
(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.
(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...
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aba6c73511a2
require consistent bloatview reporting for nsISupports classes; r=froydnj
Backout by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8acf32ac3d00
Backout aba6c73511a2 for massive test bustage resulting in a CLOSED TREE; r=alltheorange
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.
(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...
(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.
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!
Flags: needinfo?(nfroyd)
(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.
Flags: needinfo?(nfroyd)
(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?
Duplicate of this bug: 1321338
Mass wontfix for bugs affecting firefox 52.
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.