Closed
Bug 1028139
Opened 10 years ago
Closed 10 years ago
DataBuffer shouldn't be refcounted
Categories
(Core :: WebRTC: Networking, defect)
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: bjacob, Assigned: anujagarwal464)
References
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
mccr8
:
review+
|
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: DataBuffer
Flags: needinfo?(rjesup)
Flags: needinfo?(khuey) → needinfo?(bjacob)
Reporter | ||
Comment 2•10 years ago
|
||
As far as I am concerned, yes, please do! Thanks!
Flags: needinfo?(bjacob)
Assignee | ||
Comment 3•10 years ago
|
||
@bjacob: Could you tell me what needs do be done here? http://mxr.mozilla.org/mozilla-central/source/media/mtransport/databuffer.h
Flags: needinfo?(bjacob)
Reporter | ||
Comment 4•10 years ago
|
||
I don't know this databuffer class or any mtransport specifics, really. I can only tell you the general approach to this category of bugs. The problem here is that DataBuffer is reference-counted, so the only thing that should ever be calling its destructor should be its Release() macro, which is implemented by this NS_INLINE_DECL_THREADSAFE_REFCOUNTING(DataBuffer) macro. So its destructor should not be public. Yet, it is public (that's the problem) and when I tried to make it protected, I got compilation errors that didn't look trivial to fix, though I don't remember the details. So you should make its destructor protected, remove the HasDangerousPublicDestructor<DataBuffer> specialization there, try to rebuild, get compilation errors, and try to fix those. That part is going to depend on the specifics of how DataBuffer is used, so if you need help with that, ask people who have been working on this part of the codebase. Good luck!
Flags: needinfo?(bjacob)
Comment 6•10 years ago
|
||
I'm not sure what the question is. This is a class that holds a block of data. It seems self explanatory enough.
Updated•10 years ago
|
Flags: needinfo?(ekr)
The needinfo request was for you to find someone to do this.
Great! Let us know if you need help.
Assignee: nobody → anujagarwal464
Comment on attachment 8453910 [details] [diff] [review] bug1028139.diff Review of attachment 8453910 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/mtransport/databuffer.h @@ +14,5 @@ > #include <nsISupportsImpl.h> > > namespace mozilla { > > class DataBuffer; remove this forward declaration now that it is unnecessary.
Attachment #8453910 -
Flags: feedback?(khuey) → review?(rjesup)
Comment 12•10 years ago
|
||
Comment on attachment 8453910 [details] [diff] [review] bug1028139.diff Review of attachment 8453910 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h @@ +187,5 @@ > virtual nsresult SendRtpPacket(const void* data, int len); > virtual nsresult SendRtcpPacket(const void* data, int len); > > private: > + virtual nsresult SendRtpPacket_s(nsRefPtr<DataBuffer> data); This seems to change the memory discipline, since now both the caller and the callee are holding pointers to the memory.
Attachment #8453910 -
Flags: review-
Comment 13•10 years ago
|
||
(In reply to Eric Rescorla (:ekr) from comment #12) > Comment on attachment 8453910 [details] [diff] [review] > bug1028139.diff > > Review of attachment 8453910 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h > @@ +187,5 @@ > > virtual nsresult SendRtpPacket(const void* data, int len); > > virtual nsresult SendRtcpPacket(const void* data, int len); > > > > private: > > + virtual nsresult SendRtpPacket_s(nsRefPtr<DataBuffer> data); > > This seems to change the memory discipline, since now both the caller and > the callee are holding pointers to the memory. If we want to keep the same pattern, we'd need to transfer the reference, which gets a bit tricky with WrapRunnable, since it inherits the types of the arguments - for nsAutoPtr, it ends up transferring the ownership (via nsAutoPtr = nsAutoPtr); for nsRefPtr, the runnable takes a second reference (nsRefPtr = nsRefPtr). However... this shouldn't be refcounted. The *only* place that uses it with refcounting is the transport_unittests.cpp code; all product code uses it with nsAutoPtr, as it always has a single owner. Remove the refcounting, and fix transport_unittests (which I believe can likely just be swapped to nsAutoPtr as well).
Updated•10 years ago
|
Attachment #8453910 -
Flags: review?(rjesup) → review-
Comment 14•10 years ago
|
||
Anuj, you need to something like the following: 1. Remove the line NS_INLINE_DECL_THREADSAFE_REFCOUNTING(DataBuffer) from DataBuffer. That will make the class no longer refcounted. 2. Delete the HasDangerousPublicDestructor<DataBuffer> template thing. DataBuffer is no longer refcounted, so it is okay to have a public destructor, so you don't need to change the dtor. 3. Fix the compilation errors. As Randell says, this is probably mostly going to be fixing transport_unittests, and changing RefPtr<DataBuffer> to nsAutoPtr<DataBuffer>.
Assignee | ||
Comment 15•10 years ago
|
||
@mccr8: Thank you! :)
Attachment #8453910 -
Attachment is obsolete: true
Comment 16•10 years ago
|
||
transport_unittests.cpp isn't build on Windows, so I fixed up the patch for Anuj. It was just a simple search and replace, I think. try run: https://tbpl.mozilla.org/?tree=Try&rev=608baae2032d
Attachment #8454569 -
Attachment is obsolete: true
Updated•10 years ago
|
Summary: Remove dangerous public destructor of DataBuffer → DataBuffer shouldn't be refcounted
Comment 17•10 years ago
|
||
Comment on attachment 8454588 [details] [diff] [review] Privatizing public destructor of DataBuffer. try run: https://tbpl.mozilla.org/?tree=Try&rev=608baae2032d Thanks for the analysis here, Randell.
Attachment #8454588 -
Flags: review?(rjesup)
Updated•10 years ago
|
Attachment #8454588 -
Flags: review?(rjesup) → review+
Updated•10 years ago
|
Keywords: checkin-needed
Updated•10 years ago
|
Keywords: checkin-needed
Comment 18•10 years ago
|
||
I'm just fixing up the commit message. Carrying forward jesup's r+.
Attachment #8454588 -
Attachment is obsolete: true
Attachment #8454781 -
Flags: review+
Updated•10 years ago
|
Keywords: checkin-needed
Comment 19•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc19e46ce305
Keywords: checkin-needed
Comment 20•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fc19e46ce305
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in
before you can comment on or make changes to this bug.
Description
•