If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Remove dangerous public destructor of DummySocket

RESOLVED FIXED in mozilla36

Status

()

Core
WebRTC: Networking
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bjacob, Assigned: anujagarwal464, Mentored)

Tracking

Other Branch
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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
Flags: needinfo?(rjesup)
Mentor: continuation@gmail.com
(Assignee)

Comment 1

3 years ago
Created attachment 8483980 [details] [diff] [review]
bug1028140.diff
Assignee: nobody → anujagarwal464
Attachment #8483980 - Flags: feedback?(continuation)
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.
Attachment #8483980 - Flags: feedback?(continuation) → feedback+

Comment 3

3 years ago
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.
(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

3 years ago
(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

3 years ago
(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?
(Assignee)

Comment 7

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=01508c7f1a3e
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.
Flags: needinfo?(rjesup) → needinfo?(continuation)
Sorry I haven't finished this yet, I'm having some problems with ASan on my machine.
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.
Adam, is this something you could test, in an ASan build?  I wasn't able to run this test locally.  Thanks.
Flags: needinfo?(continuation) → needinfo?(adam)

Comment 12

3 years ago
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).
Flags: needinfo?(adam) → needinfo?(continuation)
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.
Attachment #8483980 - Flags: review?(ekr)
Flags: needinfo?(continuation)
ekr, are you going to be able to review this soon, or should I ask somebody else?  Thanks.
Flags: needinfo?(ekr)
Comment on attachment 8483980 [details] [diff] [review]
bug1028140.diff

Is this something you could review, Adam?  Thanks.
Attachment #8483980 - Flags: review?(adam)

Comment 16

3 years ago
(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?
Flags: needinfo?(docfaraday)
Review ping? This has been waiting for review for almost two months now.

Comment 19

3 years ago
Ok, bug 1091242 has reached a quiet period, I'll look at this now.
Flags: needinfo?(docfaraday)

Comment 20

3 years ago
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.
Attachment #8483980 - Flags: review+
Keywords: checkin-needed
Attachment #8483980 - Flags: review?(ekr)
Attachment #8483980 - Flags: review?(adam)
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f954beef9cc
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2f954beef9cc
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Flags: needinfo?(ekr)
You need to log in before you can comment on or make changes to this bug.