920122 - nsCOMPtr.h/nsAutoPtr.h/etc. unnecessarily pull in cycle collection headers

Status: NEW

(Core :: XPCOM, defect)

Description

[Nathan Froyd [:froydnj]]

nsCOMPtr.h, nsAutoPtr.h, and a couple other xpcom headers pull in nsCycleCollectionNoteChild.h so they can define ImplCycleCollection* template functions.

I propose that we should have nsCOMPtrCycleCollection.h, which looks something like:

#ifndef ...
#define ...

#include "nsCOMPtr.h"
#include "nsCycleCollectionNoteChild.h"

// appropriate functions defined here

#endif

and so on for all the other headers.  The point is that headers pulling in nsCOMPtr.h &co for member definitions or the like don't need to pull in all the cycle collection bits.  Implementation files can include the *CycleCollection.h files so that their cycle-collector-y bits come out correctly.

Comment 1

[Andrew McCreight [:mccr8]]

Are there really a lot of headers that include nsCOMPtr but not nsCCNoteChild?  I guess maybe.

How many of these are there?  It seems pretty awful and fiddly to make every CCed class include these helper headers.  Pretty much every CCed class is going to need at least the COMPtr one.  Maybe nsCycleCollectionNoteChild could define the helpers for COMPtr and AutoPtr?  That will hurt classes that are cycle collected but use no COMPtrs or RefPtrs, but I can't imagine there are a lot of those.

Relatedly, I have idly considered splitting out nsCycleCollectionParticipant.h into one header for definitions and one for declarations, though I'm not sure how much that will help.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Yes, there are a lot of places that need ref-pointer classes such as provided by nsCOMPtr.h and nsAutoPtr.h, but do not need to implement cycle collection:

 - there are a lot of places that could/should use MFBT RefPtr instead, but currently don't because MFBT RefPtr was only recently added;

 - there are a lot of places that only want to use/reference refcounted things, but do not need to implement any cycle collection themselves, so they do not need the cycle collection bits.

So I think that the basic proposal here is a very good one (I haven't looked much into the _specifics_).

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Note: in the world of DOM bindings and things exposed to JS, it is true that most strong references must be declared to the CC, so for that kind of code, the present proposal is not interesting. But there is a big world outside of things not exposed to JS that need to have strong references but do not need to implement CC themselves, and in many cases, do not rely on any CC at all (that is in fact the premise underlying MFBT RefPtr's existence!)

Comment 4

[Andrew McCreight [:mccr8]]

Thanks for the detailed explanation!  That does sound like a good goal.

It would still be nice if we could do this in some way that moves them out of nsCOMPtr etc. without making implementing a cycle collected class require piling on a bunch of weird includes.