Closed Bug 1027251 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: bjacob, Assigned: bjacob)

References

Details

Attachments

(16 files, 1 obsolete file)

877 bytes, patch
ehsan
: review+
Details | Diff | Splinter Review
29.69 KB, patch
jesup
: review+
Details | Diff | Splinter Review
4.09 KB, patch
mcmanus
: review+
Details | Diff | Splinter Review
9.03 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
5.73 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
1.76 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
4.97 KB, patch
smaug
: review+
Details | Diff | Splinter Review
5.41 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
5.17 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
1.75 KB, patch
roc
: review+
Details | Diff | Splinter Review
711 bytes, patch
briansmith
: review+
Details | Diff | Splinter Review
832 bytes, patch
roc
: review+
Details | Diff | Splinter Review
1.99 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
5.14 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
7.34 KB, patch
abr
: review+
Details | Diff | Splinter Review
3.23 KB, patch
abr
: review+
Details | Diff | Splinter Review
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.
Attachment #8442300 - Flags: review?(ehsan) → review+
Attachment #8442315 - Flags: review?(bent.mozilla)
Attachment #8442318 - Attachment description: xx-content → Dangerous public destructors in content/
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.
Attachment #8442322 - Flags: review?(brian) → review+
Attachment #8442328 - Attachment is obsolete: true
Attachment #8442328 - Flags: review?(ehsan)
Attachment #8442330 - Flags: review?(ehsan)
Attachment #8442306 - Flags: review?(jmuizelaar) → review+
Attachment #8442310 - Flags: review?(jmuizelaar) → review+
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
Attachment #8442319 - Flags: review?(dbaron) → review+
Attachment #8442325 - Flags: review?(ehsan) → review+
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!
Attachment #8442330 - Flags: review?(ehsan) → review+
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
Attachment #8442317 - Flags: review?(bugs) → review+
Attachment #8442315 - Flags: review?(bent.mozilla) → review+
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.
Attachment #8442302 - Flags: review?(roc) → review?(rjesup)
(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.
Attachment #8442318 - Attachment description: Dangerous public destructors in content/ → Dangerous public destructors in content/media
Attachment #8442318 - Flags: review?(dholbert) → review?(cpearce)
Keywords: leave-open
Whiteboard: [leave open]
Version: Other Branch → Trunk
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
Attachment #8442302 - Flags: review?(rjesup) → review+
Attachment #8442304 - Flags: review?(mcmanus) → review+
Asking :abr or :ekr to review the mtransport bits per comment 23.
Attachment #8442781 - Flags: review?(ekr)
Attachment #8442781 - Flags: review?(adam)
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).
Attachment #8442781 - Flags: review?(ekr)
Attachment #8442781 - Flags: review?(adam)
Attachment #8442781 - Flags: review+
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).
(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++...
What I had in mind was

#define CLASS_HAS_DANGEROUS_DTOR(C) \
	class C;\
\	 
	template<>\
	struct HasDangerousPublicDestructor<C>\
	{\
	  static const bool value = true;\
	};
(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!
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.
(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.
It's a common C++ idiom as Ehsan mentioned.
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.
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.
Ah, yeah. :/
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...
(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)...
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;
(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.
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!
I remain unconvinced, but since this patch has already landed, I don't think there's a lot of point in continuing to discuss.
Attachment #8442318 - Flags: review?(cpearce) → review+
(It's not already landed).
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
Assignee: nobody → bjacob
Keywords: leave-open
Blocks: 1028122
(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.
Blocks: 1028132
Blocks: 1028134
Blocks: 1028136
Blocks: 1028139
Blocks: 1028140
Blocks: 1028141
Blocks: 1028142
Blocks: 1028143
Blocks: 1028144
Blocks: 1028146
Blocks: 1028147
Blocks: 1028148
(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.
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?
(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.
(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.
Attachment #8443727 - Flags: review?(adam)
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 on attachment 8443727 [details] [diff] [review]
Address comment 25

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

Thanks.
Attachment #8443727 - Flags: review?(adam) → review+
Depends on: 1028427
Depends on: 1028428
So, am I missing something, or does this not cover nsISupports derivatives at all?
You are not missing anything, this only covers the _INLINE_ refcounting macros. Let's rename this bug to reflect that.
Summary: Remove public destructors of refcounted classes, outside of a finite explicit whitelist → Remove public destructors of NS_*_INLINE_* refcounted classes, outside of a finite explicit whitelist
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.
Blocks: 1028588
Filed bug 1028588 to finish this work. Thanks for pointing this out.
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.
That's bug 1028427.  Apparently nobody has merged inbound all weekend.
Depends on: 1028997
Blocks: 1037691
Blocks: 1049379
You need to log in before you can comment on or make changes to this bug.