1027251 - Remove public destructors of NS_*_INLINE_* refcounted classes, outside of a finite explicit whitelist

Status: RESOLVED FIXED

(Core :: General, defect)

Description

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

This is a continuation of the work started by Daniel in bug 984786. There, a lot of refcounted classes had their destructor made private to ensure that it wouldn't be directly called, as in a refcounted class only Release() should ever call the destructor.

This continues this work by adding a static_assert to the refcounting implementation itself, to catch all remaining cases of refcounted classes with public destructors, and fix as many of them as possible. To avoid blocking this indefinitely, we allow adding classes to a whitelist, when it would be too much work for now to make their destructor non-public.

The static assertion uses C++11 std::is_destructible where available, with a home-made crazy SFINAE-using fallback (modeled after the libstdc++ implementation) when C++11 std::is_destructible is not available and the compiler is still modern enough to support such SFINAE. (It turns out that calling a destructor inside of decltype() is triggering at least GCC bugs, so we can't just use that everywhere).

This patch queue starts with a patch that defines the whitelist struct, HasDangerousPublicDestructor, with a bool value defaulting to false. Subsequent patches then address one toplevel directory at a time, adding HasDangerousPublicDestructor specializations as needed (thus inserting not-trivially-fixable cases to the whitelist). Finally, the last patch in this queue adds the static assertions.

Comment 1

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

Attachment: Initial empty HasDangerousPublicDestructor whitelist

Comment 2

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

Attachment: Dangerous public destructors in media/

Comment 3

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

Attachment: Dangerous public destructors in netwerk/

Comment 4

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

Attachment: Dangerous public destructors in gfx/

Comment 5

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

Attachment: Dangerous public destructors in image/

Comment 6

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

Attachment: Dangerous public destructors in ipc/

Comment 7

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

Attachment: Dangerous public destructors in dom/

Comment 8

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

Attachment: Dangerous public destructors in content/media

Comment 9

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

Attachment: Dangerous public destructors in layout/

Comment 10

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

Attachment: Dangerous public destructors in widget/

Comment 11

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

Attachment: Dangerous public destructors in security/

Comment 12

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

Attachment: Dangerous public destructors in view/

Comment 13

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

Attachment: Dangerous public destructors in xpcom

Comment 14

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8442322 [details] [diff] [review]
Dangerous public destructors in security/

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

::: security/manager/ssl/src/SharedSSLState.h
@@ +16,5 @@
>  namespace mozilla {
>  namespace psm {
>  
>  class SharedSSLState {
> +  ~SharedSSLState();

Thanks!

Please add a comment about why it is private. Also, please put it in an explicit private: section at the end of the class.

Comment 15

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

Attachment: Enforce that dangerous public destructors must be explicitly whitelisted

Comment 16

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

Attachment: Enforce that dangerous public destructors must be explicitly whitelisted

Comment 17

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

Comment on attachment 8442319 [details] [diff] [review]
Dangerous public destructors in layout/

Please use explicit "private:" at the start of the class rather than depending on the implicit protection.

r=dbaron with that

Comment 18

[(no longer active)]

Comment on attachment 8442330 [details] [diff] [review]
Enforce that dangerous public destructors must be explicitly whitelisted

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

Can you please file a follow-up for supporting this in MFBT's RefCounted class too while I figure out how to kill it entirely?  Thanks!

Comment 19

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

Comment on attachment 8442317 [details] [diff] [review]
Dangerous public destructors in dom/


> class RevisionAddedEnableStoreCallback MOZ_FINAL :
>   public DataStoreRevisionCallback
> {
>+  ~RevisionAddedEnableStoreCallback() {}

Could you please put this to the explicit private: section after the public: stuff

> class RetrieveRevisionsCounter
> {
>+  ~RetrieveRevisionsCounter() {}
ditto

> class GlobalDirs
> {
>+  ~GlobalDirs() {}
add an explicit private: section for this after the public stuff

> class ServiceWorkerRegistration
> {
>+  ~ServiceWorkerRegistration() {}
explicit private: for this please

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8442302 [details] [diff] [review]
Dangerous public destructors in media/

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

I'm not sure if we can change media/webrtc in this way.

Comment 21

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20)
> Comment on attachment 8442302 [details] [diff] [review]
> Dangerous public destructors in media/
> 
> Review of attachment 8442302 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not sure if we can change media/webrtc in this way.

I assumed that code that uses nsISupportsImpl.h refcounting is automatically "our" code.

Comment 22

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

Landed some of the patches:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6b62998dda53
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3fd4bffe5aee
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d6996ca6e760
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/337dcfffa973
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8cc926659f1c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bc510b111d1c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c2825e722515
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6b66ea2c7d0d

Comment 23

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8442302 [details] [diff] [review]
Dangerous public destructors in media/

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

In the future, please break into media/mtransport, media/mtransport/third_party, media/webrtc/signaling/src/{sipcc/common/callcontrol), media/webrtc/signaling
In general, don't lump media subdirs together.

None of this is in the webrtc.org code.

r+ for all but the mtransport bits, especially the third_part parts.  Please give new patches for those and assign to abr and/or ekr

Comment 24

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

Attachment: Dangerous public destructors in media/mtransport

Asking :abr or :ekr to review the mtransport bits per comment 23.

Comment 25

[Adam Roach [:abr]]

Comment on attachment 8442781 [details] [diff] [review]
Dangerous public destructors in media/mtransport

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

r+ with nits

I'll reiterate other reviewers' calls to put the destructors in an explicit "private:" section.

Also, for those classes that have a HasDangerousPublicDestructor specialization, please add a comment that points to a bug for the final disposition of these classes (I read "too much work for now" as meaning we do plan to fix these eventually, and I assume there is either an existing bug or will be a new bug for the remaining classes).

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6b62998dda53
https://hg.mozilla.org/mozilla-central/rev/3fd4bffe5aee
https://hg.mozilla.org/mozilla-central/rev/d6996ca6e760
https://hg.mozilla.org/mozilla-central/rev/337dcfffa973
https://hg.mozilla.org/mozilla-central/rev/8cc926659f1c
https://hg.mozilla.org/mozilla-central/rev/bc510b111d1c
https://hg.mozilla.org/mozilla-central/rev/c2825e722515
https://hg.mozilla.org/mozilla-central/rev/6b66ea2c7d0d

Comment 27

[Eric Rescorla (:ekr)]

I guess I can live with this, but the machinery needed to bypass
this check seems pretty clunky. Is there a reason there's no macro
or other technique to make it a one-liner?

(In reply to Adam Roach [:abr] from comment #25)
> Comment on attachment 8442781 [details] [diff] [review]
> Dangerous public destructors in media/mtransport
> 
> Review of attachment 8442781 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with nits
> 
> I'll reiterate other reviewers' calls to put the destructors in an explicit
> "private:" section.
> 
> Also, for those classes that have a HasDangerousPublicDestructor
> specialization, please add a comment that points to a bug for the final
> disposition of these classes (I read "too much work for now" as meaning we
> do plan to fix these eventually, and I assume there is either an existing
> bug or will be a new bug for the remaining classes).

Comment 28

[(no longer active)]

(In reply to Eric Rescorla (:ekr) from comment #27)
> I guess I can live with this, but the machinery needed to bypass
> this check seems pretty clunky. Is there a reason there's no macro
> or other technique to make it a one-liner?

I know of no easier way to implement this check in C++...

Comment 29

[Eric Rescorla (:ekr)]

What I had in mind was

#define CLASS_HAS_DANGEROUS_DTOR(C) \
	class C;\
\	 
	template<>\
	struct HasDangerousPublicDestructor<C>\
	{\
	  static const bool value = true;\
	};

Comment 30

[(no longer active)]

(In reply to Eric Rescorla (:ekr) from comment #29)
> What I had in mind was
> 
> #define CLASS_HAS_DANGEROUS_DTOR(C) \
> 	class C;\
> \	 
> 	template<>\
> 	struct HasDangerousPublicDestructor<C>\
> 	{\
> 	  static const bool value = true;\
> 	};

Ah I see, that looks fine to me!

Comment 31

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

I prefer to avoid hiding such things behind macros. This is only temporary anyways: once this lands, we'll file bugs about each of this finite number of exceptions, and we'll fix them one by one. In the meanwhile, I'd rather not have this look like a mysterious macro to anyone.

Comment 32

[Eric Rescorla (:ekr)]

(In reply to Benoit Jacob [:bjacob] from comment #31)
> I prefer to avoid hiding such things behind macros. This is only temporary
> anyways: once this lands, we'll file bugs about each of this finite number
> of exceptions, and we'll fix them one by one. In the meanwhile, I'd rather
> not have this look like a mysterious macro to anyone.

This doesn't seem like it's worth a lot of back-and-forth, but it seems to me
that the situation is quite the opposite. You have some baffling template
that someone needs to work out as opposed to a macro that can have a clear
name and that when you search for it goes somewhere with a comment.

Comment 33

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

It's a common C++ idiom as Ehsan mentioned.

Comment 34

[(no longer active)]

I think this is kind of subjective, and it really depends on how much you love/hate macros.  TBH, given the fact that this bug is doing something very useful and that this whitelist mechanism is temporary, I'm fine with either of the two alternatives.

Comment 35

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

The other problem is that such a macro would have to assume something about the namespace that it's expanded into. People would use it in the wrong namespace and not understand why it doesn't work.

Comment 36

[(no longer active)]

Ah, yeah. :/

Comment 37

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

Actually there's a worse problem: please explain how the macro would work when the class to be forward-declared is in a different namespace. You'd basically have to pass "namespace foo { namespace bar" in an extra macro parameter...

Comment 38

[(no longer active)]

(In reply to comment #37)
> Actually there's a worse problem: please explain how the macro would work when
> the class to be forward-declared is in a different namespace. You'd basically
> have to pass "namespace foo { namespace bar" in an extra macro parameter...

For other places where we use class names as marco arguments, we usually don't forward declare them in the macro, and do that part manually, and then use the macro like MACRO(mozilla::foo::bar::Class)...

Comment 39

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

These other cases probably don't have to forward-declare these classes. I don't think that you can write forward-declarations as

class MyNamespace::MyClass;

Comment 40

[(no longer active)]

(In reply to comment #39)
> These other cases probably don't have to forward-declare these classes. I don't
> think that you can write forward-declarations as
> 
> class MyNamespace::MyClass;

You would write that as:

namespace MyNamespace {
class MyClass;
}

We do that all over the place already.

Comment 41

[(no longer active)]

FWIW, I'm agreeing with Benoit here that using a macro will just make things uglier since people would have to do the forward declaration part manually.  :-)  Sorry for being too cryptic!

Comment 42

[Eric Rescorla (:ekr)]

I remain unconvinced, but since this patch has already landed, I don't think there's a lot of point in continuing to discuss.

Comment 43

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

(It's not already landed).

Comment 44

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

I'll land the rest when this is green (but let me know if you really care about the macro thing, and if you do, how you would handle the forward-declaration-in-namespace issue).

https://tbpl.mozilla.org/?tree=Try&rev=182d744a1b15

Comment 45

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

OK, landed the rest:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/77f59b385654
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a61c4689026f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0d16701f4e3f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fc29e89ab9b1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8e05708379ff
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/77aa55e45693
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6fc08397f297

Comment 46

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

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #18)
> Comment on attachment 8442330 [details] [diff] [review]
> Enforce that dangerous public destructors must be explicitly whitelisted
> 
> Review of attachment 8442330 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you please file a follow-up for supporting this in MFBT's RefCounted
> class too while I figure out how to kill it entirely?  Thanks!

Filed bug 1028122.

Comment 47

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/77f59b385654
https://hg.mozilla.org/mozilla-central/rev/a61c4689026f
https://hg.mozilla.org/mozilla-central/rev/0d16701f4e3f
https://hg.mozilla.org/mozilla-central/rev/fc29e89ab9b1
https://hg.mozilla.org/mozilla-central/rev/8e05708379ff
https://hg.mozilla.org/mozilla-central/rev/77aa55e45693
https://hg.mozilla.org/mozilla-central/rev/6fc08397f297

Comment 48

[Adam Roach [:abr]]

(In reply to Benoit Jacob [:bjacob] from comment #45)
> OK, landed the rest:

Apparently, we have a different idea about what "r+ with nits" is supposed to mean.

I meant "you have my r+ on this patch as long as you fix the following nits," not "this is an unconditional r+, please neither address nor respond to my review comments."

So now there's some stylistically broken code in several of the classes that I generally work on, and I'm pretty annoyed about it.

I would hate to have to put r- on patches because of trivially fixed issues that I can generally trust people to get right, since it wastes both my time and yours to require me to review it again.

I have filed Bug 1028392 to address the review comments that Brian, David, Olli, and I made our approvals conditional upon. Please attach and land a patch to address those issues.

Comment 49

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

I did forget to apply the comments in comment 25, sorry about that, doing it now.

Did I actually forget something in the other comments listed above?

Comment 50

[Adam Roach [:abr]]

(In reply to Benoit Jacob [:bjacob] from comment #49)
> I did forget to apply the comments in comment 25, sorry about that, doing it
> now.
> 
> Did I actually forget something in the other comments listed above?


So, the broader issue here is that I would think that one reviewer making a general comment about style would prompt you to revisit that stylistic issue in *all* of the patches. So, for example, although the reviewer for this patch:

<https://hg.mozilla.org/integration/mozilla-inbound/rev/d6996ca6e760>

...didn't directly say anything about C++ programmers generally expecting private: sections at the end of classes, Brian Smith's comment 14 should have prompted you to review the remaining patches and make appropriate adjustments.

So it's not as much that my comments weren't addressed as much as they shouldn't have been necessary (since other reviewers already made them), *compounded* by the fact that they weren't addressed.

It's a pretty big patch set, so I'm not going to audit the whole thing; but you should consider how reviewers' comments apply to the set as a whole rather than isolating fixes to the parts of the tree whose reviewers managed to catch the defects.

Comment 51

[Adam Roach [:abr]]

(In reply to Adam Roach [:abr] from comment #50)
> ...didn't directly say anything about C++ programmers generally expecting
> private: sections at the end of classes

Mea culpa; David did actually say the beginning of the class (which I find perplexing, but I'm not going to start that discussion here). Sorry about that.

Comment 52

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

Attachment: Address comment 25

Comment 53

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

The reason why I didn't automatically extend this to directories where it hadn't been requested, is that in the present case, it is a pure matter of taste.

In general, I understand that explicit trumps implicit and especially when safety depends on that, it is better to write "private:" explicitly.

But here, precisely, we are adding a static assertion that guards that, so we couldn't accidentally make things public. The safety concern then evaporates and only the stylistic consideration is left. I absolutely should follow reviewers recommendations on style and I'm sorry that I forgot to do that for this patch.

Comment 54

[Adam Roach [:abr]]

Comment on attachment 8443727 [details] [diff] [review]
Address comment 25

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

Thanks.

Comment 55

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/12ac0e627154

Comment 56

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

So, am I missing something, or does this not cover nsISupports derivatives at all?

Comment 57

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

You are not missing anything, this only covers the _INLINE_ refcounting macros. Let's rename this bug to reflect that.

Comment 58

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

You could address nsISupports derivatives by adding that MOZ_ASSERT_TYPE_OK_FOR_REFCOUNTING(_class) to whichever macros are used by nsISupports derivatives. Note that it requires _class to be a complete type, so: either put it in some _IMPL_ macro, or, if you put it in a _DECL_ macro, do like I did and put it inside of a method definition's body.

Comment 59

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

Filed bug 1028588 to finish this work. Thanks for pointing this out.

Comment 60

[Brian Birtles (:birtles, Feb 26~ parental leave)]

This appears to have broken building on VS Express 2013. On a clobber debug build of m-c on Win 8 I get:

  nsProxyRelease.h(149) : error C2338: Reference-counted classes should not have a public destructor.

followed by many similar errors.

Switching to VS Express 2010 the build succeeds. From similar comments in IRC I notice people on Win 7 have also encountered this.

Comment 61

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

That's bug 1028427.  Apparently nobody has merged inbound all weekend.

Comment 62

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/da007249e737
https://hg.mozilla.org/mozilla-central/rev/12ac0e627154