Closed Bug 1028140 Opened 10 years ago Closed 10 years ago

Remove dangerous public destructor of DummySocket

Categories

(Core :: WebRTC: Networking, defect)

Other Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: bjacob, Assigned: anujagarwal464, Mentored)

References

Details

Attachments

(1 file)

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
Mentor: continuation
Attached patch bug1028140.diffSplinter Review
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 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
(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
(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?
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)
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)
(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.
Ok, bug 1091242 has reached a quiet period, I'll look at this now.
Flags: needinfo?(docfaraday)
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+
Attachment #8483980 - Flags: review?(ekr)
Attachment #8483980 - Flags: review?(adam)
https://hg.mozilla.org/mozilla-central/rev/2f954beef9cc
Status: NEW → RESOLVED
Closed: 10 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.