984786 - Make a bunch of classes with NS_INLINE_DECL_REFCOUNTING/NS_INLINE_DECL_THREADSAFE_REFCOUNTING less footgun-ish (w/ MOZ_FINAL & non-public destructor)

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Daniel Holbert [:dholbert]]

Refcounted classes should generally
 (a) not be subclassed (unless we know what we're doing & have a virtual destructor, to make sure Release() reliably cleans things up)

 (b) be declared with a private destructor (so that it's a compile error to declare an instance of that variable, e.g. on the stack or as a member-var)  

(Read "private" as "protected" for classes who know what they're doing w.r.t. inheritance, per (a).)

From a quick MXR search, it looks like generally we don't bother with either (a) nor (b). This is Bad. We should fix this.

Filing this bug on doing that. Starting this out as security-sensitive to be on the safe side, since it seems possible this might turn up something dangerous, but probably/hopefully not.

Comment 1

[Daniel Holbert [:dholbert]]

(Per bug summary, I'm scoping this just to classes that use NS_INLINE_DECL_REFCOUNTING for now, since there are lots of them. But really, this all should be true of any class with AddRef/Release, I think.)

Comment 2

[(no longer active)]

I think for XPCOM classes these days we usually either make them final or give them a virtual dtor if they have subclasses, perhaps with a few exceptions.

Making the dtors private does more than just prevent creating objects of that type on the stack though, it will also make it impossible to delete them (which I think is fine...)

Comment 3

[Andrew McCreight [:mccr8]]

Willy-nilly deleting refcounted objects is a bad idea.

Comment 4

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

Fwiw I've started requiring private destructors for refcounted objects in my reviews because people misuse them and assign to nsAutoPtr, manually delete, etc.

Comment 5

[(no longer active)]

Err, comment 2 is missing another paragraph which I thought I put in, but apparently not.  I meant to add, it's not exactly clear to me what Daniel is proposing to do here, so it's hard for me to say either yay or nay.  So comment 2 is basically a bunch of random thoughts on the issue.

(+1 to both comment 3 and 4, FWIW)

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) (PTO on 3/20) from comment #5)
> it's not exactly clear to me what Daniel is
> proposing to do here,

I'm adding MOZ_FINAL and marking the destructor as private in a bunch of refcounted classes, to prevent problems like comment 4.

(You're right that this will make it impossible to delete them. If any such class really needs to be deleted by something other than ::Release, then it's worth forcing the person writing the code to think twice about that, and if they *really* need it, then they can make the destructor public or add a friend class or whatever)

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> I'm adding MOZ_FINAL and marking the destructor as private in a bunch of
> refcounted classes, to prevent problems like comment 4.

[...and also to increase the likelihood that future refcounted classes will be written with MOZ_FINAL & have a private destructor, due to their authors cribbing from other code with those features.]

Comment 8

[(no longer active)]

That sounds like a good idea to me.  Thanks for doing this.

Comment 9

[Daniel Holbert [:dholbert]]

(I don't think this belongs in XPCOM -- moving it back to "General". I'm not intending to touch XPCOM code specifically; just miscellaneous refcounted classes (probably none that inherit from nsISupports) in various directories.)

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: part 1: in layout, add private destructor and MOZ_FINAL to classes that use NS_INLINE_DECL_REFCOUNTING

This patch fixes up all of the NS_INLINE_DECL_REFCOUNTING classes in layout, except for URLValue/ImageValue, which have special problems. I filed bug 984780 on those.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8393988 [details] [diff] [review]
part 1: in layout, add private destructor and MOZ_FINAL to classes that use NS_INLINE_DECL_REFCOUNTING

r=dbaron

Comment 12

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

(private dtor of course doesn't ensure that the object is deleted only via Release.
 One can still call delete this; in some other method. So the comments in the patch are a bit misleading.)

Comment 13

[Daniel Holbert [:dholbert]]

Yeah, "ensure" might be too strong of a word.

How about:
   // Private destructor, to prevent accidental deletion outside of Release():

Comment 14

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

well, private: doesn't prevent that in the base class which has the refcounting defined.

So, to prevent accidental deletion outside the class which defined refcounting, or some such.

Comment 15

[Daniel Holbert [:dholbert]]

I think it's worth emphasizing that Release() is the only thing that we're expecting to do the deletion. (I also want to keep it short so that it fits on one line.)

Per IRC, I'll go with:
   // Private destructor, to discourage deletion outside of Release():

Try run: https://tbpl.mozilla.org/?tree=Try&rev=ac11ed17f840

(Patches for other areas of code likely coming over the next few days, particularly on my flight over the weekend.)

Comment 16

[Daniel Holbert [:dholbert]]

Landed first patch:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/cb183f7c1687

Also un-hiding -- I initially hid this (as mentioned @ end of comment 1) in case I ran across any classes where this fix wouldn't compile and revealed that something maybe-dangerous might be happening. But if I find anything like that, I'll just spin off specific maybe-hidden bugs to address them.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cb183f7c1687

Comment 18

[Daniel Holbert [:dholbert]]

Attachment: part 2: in a11y, make destructor private/protected destructor to 2 classes, & MOZ_FINAL to one

There are 2 classes with NS_INLINE_DECL_REFCOUNTING in a11y.

The first (Notification) has subclasses, so this patch moves its existing virtual destructor to its "protected" section to prevent accidental deletion.

The second (SelData) doesn't have subclasses, so this patch marks it as MOZ_FINAL and gives it a trivial private destructor (again, to prevent accidental deletion/instantiation-on-the-stack).

Comment 19

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8400277 [details] [diff] [review]
part 2: in a11y, make destructor private/protected destructor to 2 classes, & MOZ_FINAL to one

I'm not super thrilled about adding comments all over just to help people who might copy stuff, but whatever.

Comment 20

[Daniel Holbert [:dholbert]]

Landed part 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/82ed5fd863e8

Comment 21

[Daniel Holbert [:dholbert]]

Attachment: part 3: fix 2 classes in /content subdirs

Comment 22

[Daniel Holbert [:dholbert]]

Attachment: part 4: /dom

Here's the patch for /dom.

Notes:
 - For the classes that I'm making MOZ_FINAL, I also convert any existing 'protected' to 'private', to make it more explicit that there are no subclasses. (e.g. in one case, the class used to have both "private" and "protected", which makes no sense given the lack of subclasses.)

 - Some of these classes already have MOZ_FINAL and/or a private destructor, which is why the patch only adds one or the other in a few places. (And some classes have subclasses, which means they don't get MOZ_FINAL -- and I made sure they have a virtual destructor, which makes refcounting them w/ subclasses OK.)

 - In cases where the class already has a "private"/"protected" block and a trivial-ish destructor, I'm just moving the destructor to the existing private/protected block. When there's no existing block, I generally just wrap the destructor in 'private: [...] public:' since that seemed cleanest to me Happy to adjust if you like.

Comment 23

[Daniel Holbert [:dholbert]]

Attachment: part 5: /content/media

Comment 24

[Daniel Holbert [:dholbert]]

Attachment: part 6: /editor

Comment 25

[Daniel Holbert [:dholbert]]

Attachment: part 7: /netwerk

Comment 26

[Daniel Holbert [:dholbert]]

Comment on attachment 8400358 [details] [diff] [review]
part 3: fix 2 classes in /content subdirs

Landed part 3: https://hg.mozilla.org/integration/mozilla-inbound/rev/90b50be3a6e1

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/82ed5fd863e8
https://hg.mozilla.org/mozilla-central/rev/90b50be3a6e1

Comment 28

[Daniel Holbert [:dholbert]]

Landed part 4: https://hg.mozilla.org/integration/mozilla-inbound/rev/ed66b2c115a5
   and part 5: https://hg.mozilla.org/integration/mozilla-inbound/rev/91a6fdfb2ad1
   and part 6: https://hg.mozilla.org/integration/mozilla-inbound/rev/793324f7c684

Comment 29

[Daniel Holbert [:dholbert]]

Comment on attachment 8400398 [details] [diff] [review]
part 7: /netwerk

Landed part 7: https://hg.mozilla.org/integration/mozilla-inbound/rev/e52d5c70955e

Comment 32

[Daniel Holbert [:dholbert]]

Attachment: part 8: /content/canvas and /gfx

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ed66b2c115a5
https://hg.mozilla.org/mozilla-central/rev/91a6fdfb2ad1
https://hg.mozilla.org/mozilla-central/rev/793324f7c684
https://hg.mozilla.org/mozilla-central/rev/e52d5c70955e

Comment 34

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

Comment on attachment 8401027 [details] [diff] [review]
part 8: /content/canvas and /gfx

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

This is soooooo r+. Thanks for doing it!

Is there a c++ wizardry trick that would allow to guard that with an assertion or something?

Comment 35

[Daniel Holbert [:dholbert]]

Thanks for the review! And, I'm not sure what you're suggesting about an assertion. This bug's patches should already make it a compile error for anyone to do "MyRefcountedClass foo;" on the stack*, per comment 0 point (b) -- so, no assertion needed, because we catch it at compile time. :)

* (except in code with 'private' privileges for the class, e.g. methods / friend classes)

Comment 36

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

I meant an assertion ensuring that you won't have to go over and over again fix all of mozilla-central. That is, RefCounted<T> would static_assert that T is final and that its destructor is not public. I'm told offline that the former is type_traits is_final in C++11, but the latter is more distant on the horizon unfortunately, and that it wouldn't be possible to implement it with sfinae at all.

Comment 37

[Daniel Holbert [:dholbert]]

Comment on attachment 8401027 [details] [diff] [review]
part 8: /content/canvas and /gfx

Landed part 8: https://hg.mozilla.org/integration/mozilla-inbound/rev/50e3d7132fa9

Comment 38

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/50e3d7132fa9

Comment 39

[Daniel Holbert [:dholbert]]

I'm going to close this out, since I'm not actively working on adding more patches at the moment, and nobody likes bugs w/ patches that land across large spans of time.

Subsequent patches to fix up issues of this sort can land in new bug(s).

Comment 40

[Daniel Holbert [:dholbert]]

I posted a PSA to dev.platform, to increase the likelihood that new code aligns with the changes that I've made here: https://groups.google.com/forum/?fromgroups#!topic/mozilla.dev.platform/jMDAQtBzdCA

Comment 41

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

Bug 1027251 continues this work by addressing remaining cases outside of a finite, explicit whitelist. After that's landed, we'll be able to file bugs about those remaining explicit bad destructors and fix them one by one.