Closed
Bug 1028140
Opened 10 years ago
Closed 10 years ago
Remove dangerous public destructor of DummySocket
Categories
(Core :: WebRTC: Networking, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: bjacob, Assigned: anujagarwal464, Mentored)
References
Details
Attachments
(1 file)
2.62 KB,
patch
|
bwc
:
review+
mccr8
:
feedback+
|
Details | Diff | Splinter Review |
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)
Updated•10 years ago
|
Mentor: continuation
Assignee | ||
Comment 1•10 years ago
|
||
Assignee: nobody → anujagarwal464
Attachment #8483980 -
Flags: feedback?(continuation)
Comment 2•10 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_; 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•10 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.
Comment 4•10 years ago
|
||
(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•10 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•10 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•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=01508c7f1a3e
Comment 8•10 years ago
|
||
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)
Comment 9•10 years ago
|
||
Sorry I haven't finished this yet, I'm having some problems with ASan on my machine.
Comment 10•10 years ago
|
||
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•10 years ago
|
||
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•10 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 13•10 years ago
|
||
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)
Comment 14•10 years ago
|
||
ekr, are you going to be able to review this soon, or should I ask somebody else? Thanks.
Flags: needinfo?(ekr)
Comment 15•10 years ago
|
||
Comment on attachment 8483980 [details] [diff] [review] bug1028140.diff Is this something you could review, Adam? Thanks.
Attachment #8483980 -
Flags: review?(adam)
Comment 16•10 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)
Comment 17•10 years ago
|
||
Review ping? This has been waiting for review for almost two months now.
Comment 19•10 years ago
|
||
Ok, bug 1091242 has reached a quiet period, I'll look at this now.
Flags: needinfo?(docfaraday)
Comment 20•10 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+
Updated•10 years ago
|
Keywords: checkin-needed
Updated•10 years ago
|
Attachment #8483980 -
Flags: review?(ekr)
Attachment #8483980 -
Flags: review?(adam)
Comment 21•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f954beef9cc
Keywords: checkin-needed
Comment 22•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2f954beef9cc
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Updated•9 years ago
|
Flags: needinfo?(ekr)
You need to log in
before you can comment on or make changes to this bug.
Description
•