931283 - Cycle collect nsVariant

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

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.

Comment 1

[Andrew McCreight [:mccr8]]

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

Comment 2

[Andrew McCreight [:mccr8]]

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

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: part 1: implement a generic factory constructor that allows creating different classes on different threads, WIP

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.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: part 2: make nsVariant cycle collected, WIP

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.

Comment 5

[Andrew McCreight [:mccr8]]

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.

Comment 6

[Benjamin Smedberg]

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.

Comment 7

[Andrew McCreight [:mccr8]]

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

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Sounds like this could be useful for mystor.

Comment 9

[Andrew McCreight [:mccr8]]

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?

Comment 10

[Andrew McCreight [:mccr8]]

In IRC, Olli said it is DataTransfer.

Comment 11

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Rename nsVariant to nsVariantBase.

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.

Comment 12

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Split out nsVariant into a subclass.

Comment 13

[Andrew McCreight [:mccr8]]

Attachment: part 3 - Implement a cycle collected version of nsVariant.

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

Comment 14

[Andrew McCreight [:mccr8]]

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

Comment 15

[Nathan Froyd [:froydnj]]

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.

Comment 16

[Pulsebot]

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

Comment 17

[Andrew McCreight [:mccr8]]

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

Comment 18

[Carsten Book [:Tomcat]]

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