1028140 - Remove dangerous public destructor of DummySocket

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, 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: DummySocket

Comment 1

[Anuj Agarwal [:anujagarwal464]]

Attachment: bug1028140.diff

Comment 2

[Andrew McCreight [:mccr8]]

Comment on attachment 8483980 [details] [diff] [review]
bug1028140.diff

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

::: media/mtransport/test/buffered_stun_socket_unittest.cpp
@@ +240,5 @@
>  
>    nr_socket *socket() { return test_socket_; }
>  
>   protected:
> +  nsRefPtr<DummySocket> dummy_;

You can get rid of the initialization of dummy_ in the constructor for this class, as nsRefPtr does that automatically.  This code is a little odd, in that it looks like it used to just leak DummySocket, but I suppose it is okay to free it instead.

Comment 3

[Eric Rescorla (:ekr)]

Comment on attachment 8483980 [details] [diff] [review]
bug1028140.diff

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

::: media/mtransport/test/buffered_stun_socket_unittest.cpp
@@ +240,5 @@
>  
>    nr_socket *socket() { return test_socket_; }
>  
>   protected:
> +  nsRefPtr<DummySocket> dummy_;

- Convention in this code is to initialize even values with sensible default ctors.
- Have you run this test under ASan to verify that it's fine. Some of these unit tests
are a bit sloppy in terms of shutdown memory discipline.

Comment 4

[Andrew McCreight [:mccr8]]

(In reply to Eric Rescorla (:ekr) from comment #3)
> - Convention in this code is to initialize even values with sensible default
> ctors.
I'm not quite sure what you mean here, but nsRefPtr<> will automatically initialize the code to null.  Do you mean the initialization should be left in despite that?  (So the initializer list is complete or whatever.)

> - Have you run this test under ASan to verify that it's fine. Some of these
> unit tests are a bit sloppy in terms of shutdown memory discipline.
Anuj, you can do an ASan try run with:
  try: -b o -p linux64-asan -u all -t none

Comment 5

[Eric Rescorla (:ekr)]

(In reply to Andrew McCreight [:mccr8] from comment #4)
> (In reply to Eric Rescorla (:ekr) from comment #3)
> > - Convention in this code is to initialize even values with sensible default
> > ctors.
> I'm not quite sure what you mean here, but nsRefPtr<> will automatically
> initialize the code to null.  Do you mean the initialization should be left
> in despite that?  (So the initializer list is complete or whatever.)

Yes. That's what I meant by "even with sensible default ctors".



> > - Have you run this test under ASan to verify that it's fine. Some of these
> > unit tests are a bit sloppy in terms of shutdown memory discipline.
> Anuj, you can do an ASan try run with:
>   try: -b o -p linux64-asan -u all -t none

Comment 6

[Eric Rescorla (:ekr)]

(In reply to Andrew McCreight [:mccr8] from comment #4)
> (In reply to Eric Rescorla (:ekr) from comment #3)
> > - Convention in this code is to initialize even values with sensible default
> > ctors.
> I'm not quite sure what you mean here, but nsRefPtr<> will automatically
> initialize the code to null.  Do you mean the initialization should be left
> in despite that?  (So the initializer list is complete or whatever.)
> 
> > - Have you run this test under ASan to verify that it's fine. Some of these
> > unit tests are a bit sloppy in terms of shutdown memory discipline.
> Anuj, you can do an ASan try run with:
>   try: -b o -p linux64-asan -u all -t none

Actually, I'm not sure that's true. Do these tests run under try?

Comment 7

[Anuj Agarwal [:anujagarwal464]]

https://tbpl.mozilla.org/?tree=Try&rev=01508c7f1a3e

Comment 8

[Andrew McCreight [:mccr8]]

Hmm.  That doesn't look good, but nothing that is failing seems related to this.  I'll try a local ASan build and figure out how to run this test.

Comment 9

[Andrew McCreight [:mccr8]]

Sorry I haven't finished this yet, I'm having some problems with ASan on my machine.

Comment 10

[Andrew McCreight [:mccr8]]

I finally got ASan running again, but now I get this error message when running cppunit tests:
  Caught exception running cpp unit tests: [Errno 13] Permission denied
I'm not sure what is going on there.  I'll try to figure it out.

Comment 11

[Andrew McCreight [:mccr8]]

Adam, is this something you could test, in an ASan build?  I wasn't able to run this test locally.  Thanks.

Comment 12

[Adam Roach [:abr]]

Andrew: With the caveat that I'm running this under OS X, I get a clean run on buffered_stun_socket_unittest under ASAN. I'm tagging you for needinfo just so you don't miss my response here (it's been a few days, I know).

Comment 13

[Andrew McCreight [:mccr8]]

Comment on attachment 8483980 [details] [diff] [review]
bug1028140.diff

Thanks for checking!

I'll ask ekr for review because I think he took a look at this patch before.

Comment 14

[Andrew McCreight [:mccr8]]

ekr, are you going to be able to review this soon, or should I ask somebody else?  Thanks.

Comment 15

[Andrew McCreight [:mccr8]]

Comment on attachment 8483980 [details] [diff] [review]
bug1028140.diff

Is this something you could review, Adam?  Thanks.

Comment 16

[Adam Roach [:abr]]

(In reply to Andrew McCreight [:mccr8] from comment #15)
> Comment on attachment 8483980 [details] [diff] [review]
> bug1028140.diff
> 
> Is this something you could review, Adam?  Thanks.

Maybe, but not soon. I have a pretty full plate right now. Maybe Byron could take a look at it after the sdparta work lands?

Comment 17

[Andrew McCreight [:mccr8]]

Review ping? This has been waiting for review for almost two months now.

Comment 19

[Byron Campen [:bwc]]

Ok, bug 1091242 has reached a quiet period, I'll look at this now.

Comment 20

[Byron Campen [:bwc]]

Comment on attachment 8483980 [details] [diff] [review]
bug1028140.diff

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

Ok, seems to run fine under ASAN, and I don't see anything objectionable.

Comment 21

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f954beef9cc

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2f954beef9cc