1028148 - Remove dangerous public destructor of mozilla::ipc::SharedMemory

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

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

In bug 1027251 we removed all dangerous public destructors of XPCOM-refcounted classes outside of a finite whitelist, see HasDangerousPublicDestructor. Now we are going over the entries in this whitelist.

One of them is: mozilla::ipc::SharedMemory

Comment 1

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

This is going to be one of the harder ones (among the blockers of bug 1028132).

The big problem is that the IPDL code generator (ipc/ipdl/lower.py) generates nsAutoPtr<SharedMemory>. I suppose that those will need to be changed into nsRefPtr's instead, though I'm not an expert.

Comment 2

[Andrew McCreight [:mccr8]]

I'm supposed to be looking into IPDL anyways, so I'll take a look.

Comment 3

[Andrew McCreight [:mccr8]]

billm pointed out that the functionality that refcounting was added for isn't even used (Adopt something something), so it doesn't really need to be refcounted at all, but maybe it will be used in the Fuuuutureeee.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Make SharedMemory's dtor private.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: part 2 - SharedMemory does not need COUNT_CTOR/DTOR any more.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8457434 [details] [diff] [review]
part 1 - Make SharedMemory's dtor private.

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

r=me with this addressed:

::: ipc/glue/SharedMemoryBasic_android.h
@@ +56,5 @@
>    bool ShareToProcess(base::ProcessHandle aProcess,
>                        Handle* aNewHandle);
>  
>  private:
> +  virtual ~SharedMemoryBasic();

Hrm, a private virtual destructor doesn't make much sense... Does this just need to be marked MOZ_FINAL and then you won't need the virtual any more?

Comment 7

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c951bf5bbc50
https://hg.mozilla.org/integration/mozilla-inbound/rev/11e19dab58af

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/11e19dab58af
https://hg.mozilla.org/mozilla-central/rev/c951bf5bbc50

Comment 9

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

Congrats and thanks Andrew, that was a tough and bad bug!

Comment 10

[Andrew McCreight [:mccr8]]

Thanks.  It wasn't too bad. ;)  Billm was having some kind of SharedMemory leak on e10s that has gone away recently, so maybe some of this work fixed that.

Comment 11

[Andrew McCreight [:mccr8]]

> Does this just need to be marked MOZ_FINAL and then you won't need the virtual any more?
I marked the class FINAL and made the dtor non-virtual.