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

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Justin Lebar (not reading bugmail)]

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.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 2

[Justin Lebar (not reading bugmail)]

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...

Comment 3

[Mounir Lamouri (:mounir)]

(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.

Comment 4

[Justin Lebar (not reading bugmail)]

(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.)

Comment 5

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

Comment 6

[Justin Lebar (not reading bugmail)]

Attachment: Patch v2

Comment 7

[Mounir Lamouri (:mounir)]

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.

Comment 8

[Justin Lebar (not reading bugmail)]

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."

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 10

[Justin Lebar (not reading bugmail)]

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.

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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.

Comment 12

[Justin Lebar (not reading bugmail)]

> 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.  :)

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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.

Comment 14

[Justin Lebar (not reading bugmail)]

In bug 679966, we store a bona fide nsTArray in a ClearOnShutdown'ed nsAutoPtr.

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Well, nsTArray doesn't have AddRef/Release, which was my point, but ok.

Comment 16

[Justin Lebar (not reading bugmail)]

Chris, would you feel comfortable reviewing this?  It's blocking two vibrator patches.

Comment 17

[Benjamin Smedberg]

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.

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

If we do that we need to teach ClearOnShutdown whether it needs to Release, delete, etc.

Comment 19

[Justin Lebar (not reading bugmail)]

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.

Comment 20

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

jlebar, are you planning on posting an updated patch?

Comment 21

[Justin Lebar (not reading bugmail)]

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?

Comment 22

[Justin Lebar (not reading bugmail)]

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.

Comment 23

[(not currently active) Ted Mielczarek]

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. :-/

Comment 24

[Justin Lebar (not reading bugmail)]

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 25

[Benjamin Smedberg]

Comment on attachment 578641 [details] [diff] [review]
Patch v2

ok, in this case please update the coding style guide.

Comment 26

[Justin Lebar (not reading bugmail)]

> 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

Comment 27

[Benjamin Smedberg]

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.

Comment 28

[Justin Lebar (not reading bugmail)]

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...

Comment 29

[Justin Lebar (not reading bugmail)]

I landed on inbound a bit earlier, btw.

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

Comment 30

[Mike Hommey [:glandium]]

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.

Comment 31

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ab38cc8b5866