Closed Bug 1028148 Opened 6 years ago Closed 5 years ago

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

Categories

(Core :: IPC, defect)

Other Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: bjacob, Assigned: mccr8)

References

Details

Attachments

(2 files)

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
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.
I'm supposed to be looking into IPDL anyways, so I'll take a look.
Assignee: nobody → continuation
Depends on: 1038853
Depends on: 1038855
No longer depends on: 1038853
Depends on: 1038966
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.
Attachment #8457434 - Flags: review?(bent.mozilla)
Attachment #8457435 - Flags: review?(bent.mozilla)
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?
Attachment #8457434 - Flags: review?(bent.mozilla) → review+
Attachment #8457435 - Flags: review?(bent.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/11e19dab58af
https://hg.mozilla.org/mozilla-central/rev/c951bf5bbc50
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Congrats and thanks Andrew, that was a tough and bad bug!
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.
> 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.
You need to log in before you can comment on or make changes to this bug.