931285 - Make a cycle collected version of nsHashPropertyBag and use it from JS

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8] (PTO until 12-2)]

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).  This extra CC problem specifically only shows up in the RSS feed unit tests in toolkit/components/feeds.  This is the same idea as nsArray vs nsArrayCC.  Networking stuff uses nsHashPropertyBag, so I think I have to leave that alone.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Yay! I remember looking at the cycle collector over a year ago and noticing that we should be CCing this class. I never fixed it though.

Comment 2

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Attachment: implement CCed subclass, WIP

This is kind of the minimum viable patch.  It works well enough to not immediately blow up.  It implements a new subclass that is cycle collected.

I'm not a big fan of the way this (and nsArrayCC) shadows the refcounting stuff.  It also has the deficiency that if you use XPCOM to get a bag off the main thread, you'll end up with something that will not work.

I think a better approach may be more like bug 931283.
1. Create a base class that leaves ISupports methods abstract, but implements everything else.
2. Create a nsHashPropertyBagMT class that is same as the current nsHashPropertyBag class, with threadsafe refcounting, and switch over existing uses of the class that aren't on the main thread to use it.
3. Create a main-thread-only nsHashPropertyBag like I've defined in this patch. Maybe name it BagCC or maybe not.

Then use the thready factory constructor method I created in bug 931283 to select an implementation depending on which thread you are on.

One weird thing about the bag class is that a number of the methods create nsVariants, which is a single threaded class.  I guess they are only ever called from a single thread?  Maybe I can split the interface out a bit so those methods don't exist for the threadsafe property bag variant.

Comment 4

[Andrew McCreight [:mccr8] (PTO until 12-2)]

For some reason hash property bag causes a leak in the child process when used from JS.

Comment 5

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Attachment: part 1 - Factor out a base class for nsHashPropertyBag that does not implement refcounting.

Also remove the unused NS_NewHashPropertyBag.

Comment 6

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Attachment: part 2 - Implement a cycle collected version of nsHashPropertyBag and use it from JS.

Comment 7

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Comment on attachment 8485135 [details] [diff] [review]
part 1 - Factor out a base class for nsHashPropertyBag that does not implement refcounting.

Try run looks green.  well, except for the failures which I'm pretty sure are from other stuff in the queue.

Anyways, the idea here is that we factor out an abstract base class that does not implement refcounting, then subclass it twice, once for threadsafe stuff for Necko and whoever else, then a second one for JS that is CCed.

Comment 8

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Comment on attachment 8485136 [details] [diff] [review]
part 2 - Implement a cycle collected version of nsHashPropertyBag and use it from JS.

try: https://tbpl.mozilla.org/?tree=Try&rev=0f0399454863

I could try to be clever and factor out the QI, too, though QI macro code scares me so I just did a copy-paste-modify thing.

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 8485135 [details] [diff] [review]
part 1 - Factor out a base class for nsHashPropertyBag that does not implement refcounting.

Review of attachment 8485135 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the changes below.

::: xpcom/ds/nsHashPropertyBag.cpp
@@ -15,5 @@
> -nsresult
> -NS_NewHashPropertyBag(nsIWritablePropertyBag** aResult)
> -{
> -  nsRefPtr<nsHashPropertyBag> hpb = new nsHashPropertyBag();
> -  hpb.forget(aResult);

It's too bad that deleting dead code removes one of the few ways we do these NS_New* constructors correctly. =/

::: xpcom/ds/nsHashPropertyBag.h
@@ +32,2 @@
>  
> +class nsHashPropertyBag : public nsHashPropertyBagBase

Since we're already here, maybe make this MOZ_FINAL?

@@ +40,1 @@
>    virtual ~nsHashPropertyBag() {}

Probably doesn't matter much, since this is nsISupports stuff, but I think you want to have the (protected!) virtual destructor in the base class, for code cleanliness, if nothing else.

Comment 10

[Andrew McCreight [:mccr8] (PTO until 12-2)]

(In reply to Nathan Froyd (:froydnj) from comment #9)
> It's too bad that deleting dead code removes one of the few ways we do these
> NS_New* constructors correctly. =/
I dunno, this looks so trivial I wasn't sure how useful it was to have it. :)

> Since we're already here, maybe make this MOZ_FINAL?
A ton of networking code inherits from nsHashPropertyBag.  That's why I have to go through this whole rigamarole in the first place. :)

> Probably doesn't matter much, since this is nsISupports stuff, but I think
> you want to have the (protected!) virtual destructor in the base class, for
> code cleanliness, if nothing else.

Good point, I'll try that.

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8485136 [details] [diff] [review]
part 2 - Implement a cycle collected version of nsHashPropertyBag and use it from JS.

Review of attachment 8485136 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with changes and answers.

::: xpcom/build/XPCOMModule.inc
@@ +62,5 @@
>  
>      COMPONENT(VARIANT, nsVariantConstructor)
>      COMPONENT(INTERFACEINFOMANAGER_SERVICE, nsXPTIInterfaceInfoManagerGetSingleton)
>  
> +    COMPONENT(HASH_PROPERTY_BAG, nsHashPropertyBagCCConstructor)

Have you audited all the places that create these things through XPCOM to see if the newly created objects get passed to other threads?  Admittedly, they were probably doing dodgy things before, but the dodginess at least had the virtue of working.  This change will loudly break them.

::: xpcom/ds/nsHashPropertyBag.cpp
@@ +279,5 @@
> +
> +NS_IMPL_CYCLE_COLLECTION(nsHashPropertyBagCC, mPropertyHash)
> +
> +NS_IMPL_CYCLE_COLLECTING_ADDREF(nsHashPropertyBagCC)
> +NS_IMPL_CYCLE_COLLECTING_RELEASE(nsHashPropertyBagCC)

No higher-order macro for all this stuff?  Very disappointed. ;)

::: xpcom/ds/nsHashPropertyBag.h
@@ +42,5 @@
>    virtual ~nsHashPropertyBag() {}
>  };
>  
> +/* A cycle collected nsHashPropertyBag for use from JS. */
> +class nsHashPropertyBagCC : public nsHashPropertyBagBase

MOZ_FINAL, please.

Comment 12

[Andrew McCreight [:mccr8] (PTO until 12-2)]

> Have you audited all the places that create these things through XPCOM to see if the newly created objects get passed to other threads?

My impression is that the users who care about threadsafety don't use XPCOM, but I'll look.  Would that basically be looking at places where @mozilla.org/hash-property-bag;1 appears in the code base?  eg http://mxr.mozilla.org/mozilla-central/search?string=@mozilla.org/hash-property-bag;1

Comment 13

[Nathan Froyd [:froydnj]]

(In reply to Andrew McCreight [:mccr8] from comment #10)
> > Since we're already here, maybe make this MOZ_FINAL?
> A ton of networking code inherits from nsHashPropertyBag.  That's why I have
> to go through this whole rigamarole in the first place. :)

That sounds...not so good.  Are all the other nsHashPropertyBag users paying for threadsafe AddRef/Release, when it's really Necko's weird use of this as a base class that requires it, for instance?  File a followup bug, please?

> > Probably doesn't matter much, since this is nsISupports stuff, but I think
> > you want to have the (protected!) virtual destructor in the base class, for
> > code cleanliness, if nothing else.
> 
> Good point, I'll try that.

...and since I didn't realize Necko used it as a base class, the virtual destructor can be kept in this class too.

(In reply to Andrew McCreight [:mccr8] from comment #12)
> > Have you audited all the places that create these things through XPCOM to see if the newly created objects get passed to other threads?
> 
> My impression is that the users who care about threadsafety don't use XPCOM,
> but I'll look.  Would that basically be looking at places where
> @mozilla.org/hash-property-bag;1 appears in the code base?  eg
> http://mxr.mozilla.org/mozilla-central/search?string=@mozilla.org/hash-
> property-bag;1

Yeah.  Looking at those, it seems like none of them have off-main-thread usages, so I think we're good here.

Comment 14

[Andrew McCreight [:mccr8] (PTO until 12-2)]

I filed bug 1064361 for changing C++ consumers.  To that end, I changed the comment on nsHashPropertyBagCC to "A cycle collected nsHashPropertyBag for main-thread-only use." instead of saying "for use from JS".

Comment 15

[Andrew McCreight [:mccr8] (PTO until 12-2)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/55f41b0d3ab2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/98d7d9f16f17

Comment 16

[Andrew McCreight [:mccr8] (PTO until 12-2)]

The only other change I made was to mark nsHashPropertyBagCC MOZ_FINAL as that seemed to be where the discussion here left things.  Thanks for the review.

Comment 17

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Oh, and while I'm bugspamming, this did seem to be the last leak in M4 e10s, with the rest of my sketchy patch queue:
  https://tbpl.mozilla.org/?tree=Try&rev=0f0399454863

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/55f41b0d3ab2
https://hg.mozilla.org/mozilla-central/rev/98d7d9f16f17