This is part of an invisible cycle that seems to make us take an extra CC at shutdown whenever I mess with CC (bug 927601 and ICC). The hard parts of this are already implemented, so this is easy.

This extra CC problem specifically only shows up in the RSS feed unit tests in toolkit/components/feeds.

Apparently nsVariant is used off the main thread, which will make this trickier...

This is a little hacky. The property I'm really interested in is whether a thread supports cycle collection or not, but I don't really feel like implementing a little function to detect that right now. So that means that if you use XPCOM on workers to instantiated one of these classes you'll get a non-CCed class, but I don't think workers really use XPCOM so that should be okay.

This splits nsVariant into three separate classes: 1. nsVariantBase, which implements everything that nsVariant does right now, except for the ISupports methods, which it leaves abstract. 2. nsVariant, which is cycle collected. The existing places that create nsVariant all look very main-thready, so I think this is a good default. 3. nsVariantNonCC. This is the same as the current nsVariant, and is only supposed to be used on threads that don't support cycle collection. With these classes specified, I use the new generic factory constructor macro defined in part 1, so that we use nsVariant on the main thread and nsVariantNonCC, when somebody tries to instantiate an nsIVariant. This works well enough to not immediately crash. To reduce code churn a bit, I've left the giant pile of static methods in nsVariant. Personally, I think the current setup is a bit weird. It makes more sense to me for these to be static methods on nsDiscriminatedUnion or some other kind of nsDiscriminatedUnionHelper class, but I decided to leave it alone.

Well, that doesn't compile on GCC, because it gets mad that nsVariant has methods with the same name as its parent. There's a comment in nsDiscriminatedUnion dating back to bug 44675 saying "It has no methods. So, its use requires no linkage to the xpcom module." bsmedberg, is this still something we need to worry about? I would guess not. If I can put methods on nsDiscriminatedUnion, then I could change the static methods on nsVariant to regular methods on nsDiscriminatedUnion, which seems better to me.

That's only an issue if code outside libxul needs to use this class. I don't know whether that applies to code from comm-central or not.

Thanks. I don't see any instances of nsDiscriminatedUnion in comm-central aside from nsVariant and XPCVariant, so I think it should be okay.

Sounds like this could be useful for mystor.

I started looking at this. It will be a little tedious to fix because I need to audit each place and figure out if it is used off-main-thread or not. Though maybe I'll just leave things threadsafe for the moment and people can opt in. mystor, which nsVariant is causing the leak?

In IRC, Olli said it is DataTransfer.

Leave a typedef for compatibility. nsVariant will be defined as a separate class in the next patch. Also, remove an obsolete comment and fix some whitespace.

Also, use this class for the component manager etc. in XPCOM.

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

Comment on attachment 8671405 [details] [diff] [review] part 2 - Split out nsVariant into a subclass. Review of attachment 8671405 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/ds/nsVariant.h @@ +178,5 @@ > { > public: > + NS_IMETHOD QueryInterface(REFNSIID aIID, void** aInstancePtr) override = 0; > + NS_IMETHOD_(MozExternalRefCountType) AddRef(void) override = 0; > + NS_IMETHOD_(MozExternalRefCountType) Release(void) override = 0; I think you can remove these manual declarations, NS_DECL_*_ISUPPORTS in the derived classes should be sufficient.

https://hg.mozilla.org/integration/mozilla-inbound/rev/99164ec56e7b https://hg.mozilla.org/integration/mozilla-inbound/rev/a9cfb80772ac https://hg.mozilla.org/integration/mozilla-inbound/rev/465a87f5d4e6

(In reply to Nathan Froyd [:froydnj] from comment #15) > I think you can remove these manual declarations, NS_DECL_*_ISUPPORTS in the > derived classes should be sufficient. Good point. I guess the NS_DECL should probably be in the leaf classes, too, but meh.

https://hg.mozilla.org/mozilla-central/rev/99164ec56e7b https://hg.mozilla.org/mozilla-central/rev/a9cfb80772ac https://hg.mozilla.org/mozilla-central/rev/465a87f5d4e6