The default bug view has changed. See this FAQ.

Add ClearOnShutdown() function which will clear a smart pointer on shutdown

RESOLVED FIXED in mozilla11

Status

()

Core
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: Justin Lebar (not reading bugmail))

Tracking

({dev-doc-needed})

Trunk
mozilla11
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
I've needed to create global auto/ref pointers which are cleared on shutdown in each of the past three patches I've written.

I have code for this; I'll upload it in a moment.
(Assignee)

Updated

5 years ago
Blocks: 679966
(Assignee)

Updated

5 years ago
Blocks: 694862
OS: Linux → All
Hardware: x86_64 → All
Version: unspecified → Trunk
I'm kind of opposed to this because I think it'll make writing bad code easier, but I'll wait for the patch before passing judgement.
(Assignee)

Comment 2

5 years ago
I don't entirely disagree, but the amount of boilerplate you need in order to have a global TArray or otherwise leak-checked object is just nuts.

It looks like I may have to redo this as a function |ClearOnShutdown(&gSmartPtr);|.  I dunno if that would be any more palatable...
OS: All → Linux
Hardware: All → x86_64
Version: Trunk → unspecified
(In reply to Justin Lebar [:jlebar] from comment #2)
> I don't entirely disagree, but the amount of boilerplate you need in order
> to have a global TArray or otherwise leak-checked object is just nuts.

How does the leak reporter works? Could we inform it that some objects shouldn't be considered as leaking? That's a real question, I really have no idea how that things work.

I definitely agree with Justin though, it's quite annoying to have global objects and not have them seen as leaking.
OS: Linux → All
Hardware: x86_64 → All
Version: unspecified → Trunk
(Assignee)

Comment 4

5 years ago
(Sorry for setting the platform back; I refreshed, but must not have shift-refreshed.)

You can definitely say "don't track this *class*", but I don't see how to say "don't track this *object*".  See http://hg.mozilla.org/mozilla-central/file/68f5a6758770/xpcom/glue/nsTArray-inl.h#l46

I'm tempted to make this class a nop when leak-checking is disabled.  There's really no reason to destroy these objects on shutdown except for our leak-checking, so why bother?  (In general, we free() *tons* of stuff on shutdown, when we really should just quit.)
(Assignee)

Updated

5 years ago
Summary: Add auto/ref-pointer specializations which are cleared on shutdown → Add ClearOnShutdown() function which will clear a smart pointer on shutdown
(Assignee)

Comment 5

5 years ago
Created attachment 578617 [details] [diff] [review]
Patch v1
Attachment #578617 - Flags: review?(benjamin)
(Assignee)

Comment 6

5 years ago
Created attachment 578641 [details] [diff] [review]
Patch v2
Attachment #578641 - Flags: review?(benjamin)
(Assignee)

Updated

5 years ago
Attachment #578617 - Attachment is obsolete: true
Attachment #578617 - Flags: review?(benjamin)
(Assignee)

Updated

5 years ago
Attachment #578641 - Attachment description: A hunk was missing from the last version. → Patch v2
Wouldn't that be better to have only one object doing that? The object could keep a list of variables to nullify when the shutdown notification happens. Seems to be more scalable in the sense of we don't end up having a ton of observers doing all the same thing.
(Assignee)

Comment 8

5 years ago
It gets complicated because of the templates.  Each observer is different code at the moment.  I guess you could use inheritance and virtual methods to make it work...

Or we can weasel out of it by saying "don't create a lot of these."
Yeah, I had the same thought.  I think it's better to give up the templates than to have lots of these objects floating around.
(Assignee)

Comment 10

5 years ago
You had the same thought as Mounir, or as me?

I'm not convinced Mounir's idea is worth the trouble, at least as I understand it:

Suppose we kept a single list of nsRefPtr<X>s.  We could do this, because we know that all nsRefPtr<T>s share the same layout.  Now we want to destroy them.  So pull a nsRefPtr<X> out of the list.  You need to know the value of X in order to run the correct destructor.  But how can we know?

The only way I can think of to do this is to instead store a list of WrapperBase objects.  WrapperBase is an abstract class, and we create concrete objects of the form

  Wrapper<T> : public WrapperBase {
    virtual DestructMe() {
      *((nsRefPtr<T>*) WrapperBase::mPtr) = NULL;
    }
  };

but at this point, we end up creating one new object for each entry in the list, and new code for each type -- exactly what Mounir wanted to avoid, right?

We create less code for each type, but otherwise, this is very similar to registering an observer.

I'm not sure we should over-engineer this for the case where we have lots of objects to destroy on shutdown.  We just shouldn't reach that state.  And if, for some reason, someone wants to destroy 1000 objects at shutdown, *they* can keep a list of them and register just one observer.
(In reply to Justin Lebar [:jlebar] from comment #10)
> You had the same thought as Mounir, or as me?

Both, actually.

> I'm not convinced Mounir's idea is worth the trouble, at least as I
> understand it:
> 
> Suppose we kept a single list of nsRefPtr<X>s.  We could do this, because we
> know that all nsRefPtr<T>s share the same layout.  Now we want to destroy
> them.  So pull a nsRefPtr<X> out of the list.  You need to know the value of
> X in order to run the correct destructor.  But how can we know?

This is only a problem if AddRef/Release are non-virtual.  How often does that happen?

> I'm not sure we should over-engineer this for the case where we have lots of
> objects to destroy on shutdown.  We just shouldn't reach that state.  And
> if, for some reason, someone wants to destroy 1000 objects at shutdown,
> *they* can keep a list of them and register just one observer.

That gets back to the question of whether we need this at all.  I'm not convinced that things that we don't want people to do should be easy to do.
(Assignee)

Comment 12

5 years ago
> This is only a problem if AddRef/Release are non-virtual.  How often does that happen?

nsTArray has non-virtual AddRef/Release, and I'd like to use it here.

> I'm not convinced that things that we don't want people to do should be easy to do.

There are legitimate uses for this, I think.  I've identified two.  But I do think that we shouldn't make things we don't want people to do *performant*.  So that's how I weasel my way out of making this smarter.  :)
(In reply to Justin Lebar [:jlebar] from comment #12)
> > This is only a problem if AddRef/Release are non-virtual.  How often does that happen?
> 
> nsTArray has non-virtual AddRef/Release, and I'd like to use it here.

I don't think nsTArray is what you meant to say here, but I'll accept that we want to use it for things that have non-virtual AddRef/Release

> > I'm not convinced that things that we don't want people to do should be easy to do.
> 
> There are legitimate uses for this, I think.  I've identified two.  But I do
> think that we shouldn't make things we don't want people to do *performant*.
> So that's how I weasel my way out of making this smarter.  :)

ok.
(Assignee)

Comment 14

5 years ago
In bug 679966, we store a bona fide nsTArray in a ClearOnShutdown'ed nsAutoPtr.
Well, nsTArray doesn't have AddRef/Release, which was my point, but ok.
(Assignee)

Comment 16

5 years ago
Chris, would you feel comfortable reviewing this?  It's blocking two vibrator patches.
(Assignee)

Updated

5 years ago
Attachment #578641 - Flags: review?(jones.chris.g)
Comment on attachment 578641 [details] [diff] [review]
Patch v2

Our coding guide in the past has said "no static COMPtrs", so this would seem to be encouraging a change in the guide. I would expect existing usage to be a raw static pointer:

static Foo* sFoo;

And the API would/should be void ClearOnShutdown(Foo**);

It seems unfortunate that we have to allocate these heavyweight objects to accomplish this, but I can't think of a simpler typesafe way to do this.
Attachment #578641 - Flags: review?(benjamin) → review-
If we do that we need to teach ClearOnShutdown whether it needs to Release, delete, etc.
(Assignee)

Comment 19

5 years ago
You could have

  DeleteOnShutdown()
  ReleaseOnShutdown()

But then you'd have to manually addref the pointer before you assigned it to the raw pointer.

Having a static COM/Ref/Auto pointer seems preferable to me, but I'm happy either way.
jlebar, are you planning on posting an updated patch?
(Assignee)

Comment 21

5 years ago
I didn't think we had consensus that DeleteOnShutdown / ReleaseOnShutdown were the way to go.

It's not a big deal either way, but it seems to me that our coding guide says no static COMPtrs because there's no way to keep them from leaking.  Storing a refcounted object in a raw pointer seems error-prone to me.  For example, in one patch, I overwrite the value of the static pointer, so I'd have to remember to manually release before assigning -- isn't that what smart pointers are for?
(Assignee)

Comment 22

5 years ago
Comment on attachment 578641 [details] [diff] [review]
Patch v2

Re-flagging for review for feedback on comments 19 and 21.  I'm happy to do DeleteOnShutdown / ReleaseOnShutdown if you think that's the right thing, but it's not clear from your previous review comment that you do.
Attachment #578641 - Flags: review?(jones.chris.g)
Attachment #578641 - Flags: review?(benjamin)
Attachment #578641 - Flags: review-
Seems a little crummy to add an observer-per-pointer here. I'm not sure if it's straightforward to hold onto a list, though, because the types would all vary. :-/
(Assignee)

Comment 24

5 years ago
I feel like adding a list would be a premature optimization.  We'd have to keep a list of objects with virtual functions, which is exactly what the observer code needs to do.  Maybe we could do it with less overhead, but unless we have hundreds of clear-on-shutdown objects, will it matter?
Comment on attachment 578641 [details] [diff] [review]
Patch v2

ok, in this case please update the coding style guide.
Attachment #578641 - Flags: review?(benjamin) → review+
(Assignee)

Updated

5 years ago
Whiteboard: [needs landing]
(Assignee)

Comment 26

5 years ago
> ok, in this case please update the coding style guide.

I can't seem to find this rule in the style guide.  (I searched for "static" and "COMPtr" in [1].)

Can you point me to it?

[1] https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide
Well hrm, the closest rule we have is https://developer.mozilla.org/en/C++_Portability_Guide#Don%27t_use_static_constructors

If static nsCOMPtr actually cases a static constructor to run, then perhaps we should continue to disallow it.
(Assignee)

Comment 28

5 years ago
Well, the nsCOMPtr default constructor just zeroes mRawPtr (I assume the same is true for nsRefPtr and nsAutoPtr).  But I'd guess that the compiler isn't smart enough to know that static memory is already initialized to 0 and that it doesn't need to run the constructor.

Of the four downsides listed, the only one which is applicable is perf.  I doubt it's a real perf issue until we have thousands of ClearOnShutdown objects...
(Assignee)

Comment 29

5 years ago
I landed on inbound a bit earlier, btw.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab38cc8b5866
(Assignee)

Updated

5 years ago
Whiteboard: [needs landing]
(Assignee)

Updated

5 years ago
Assignee: nobody → justin.lebar+bug
I /think/ a global nsCOMPtr initialized to nothing just ends up in bss. But I'm pretty sure this ends up creating a constructor that registers the destructor for shutdown time. Yes, there are .dtors entries in a library, but the C++ ABI requires that destructors are registered with __cxa_atexit in a .ctor. This is Linux knowledge, but I'm pretty confident it is the same everywhere.
https://hg.mozilla.org/mozilla-central/rev/ab38cc8b5866
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Keywords: dev-doc-needed
(Assignee)

Updated

5 years ago
Depends on: 711602
(Assignee)

Updated

5 years ago
Depends on: 716523
You need to log in before you can comment on or make changes to this bug.