Closed Bug 931283 Opened 6 years ago Closed 4 years ago

Cycle collect nsVariant

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

Attachments

(3 files, 2 obsolete files)

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.
Assignee: nobody → continuation
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.
Flags: needinfo?(benjamin)
Depends on: 931571
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.
Flags: needinfo?(benjamin)
Thanks.  I don't see any instances of nsDiscriminatedUnion in comm-central aside from nsVariant and XPCVariant, so I think it should be okay.
No longer blocks: IncrementalCC
Sounds like this could be useful for mystor.
Flags: needinfo?(continuation)
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?
Flags: needinfo?(continuation) → needinfo?(michael)
In IRC, Olli said it is DataTransfer.
Flags: needinfo?(michael)
Depends on: 1210517
Attachment #822794 - Attachment is obsolete: true
Blocks: 1210591
No longer blocks: 906420
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.
Attachment #8671404 - Flags: review?(nfroyd)
Also, use this class for the component manager etc. in XPCOM.
Attachment #8671406 - Flags: review?(nfroyd)
Attachment #8671404 - Flags: review?(nfroyd) → review+
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.
Attachment #8671405 - Flags: review?(nfroyd) → review+
Attachment #822795 - Attachment is obsolete: true
Attachment #8671406 - Flags: review?(nfroyd) → review+
(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.
Depends on: 1220667
You need to log in before you can comment on or make changes to this bug.