1028139 - DataBuffer shouldn't be refcounted

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: DataBuffer

Comment 1

[Anuj Agarwal [:anujagarwal464]]

Can I work on this and other look alike?

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

As far as I am concerned, yes, please do! Thanks!

Comment 3

[Anuj Agarwal [:anujagarwal464]]

@bjacob: Could you tell me what needs do be done here?
http://mxr.mozilla.org/mozilla-central/source/media/mtransport/databuffer.h

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

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!

Comment 5

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

needinfo forwarded to ekr

Comment 6

[Eric Rescorla (:ekr)]

I'm not sure what the question is. This is a class that holds a block of data. It seems self explanatory enough.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

The needinfo request was for you to find someone to do this.

Comment 8

[Anuj Agarwal [:anujagarwal464]]

I'm working on this!

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Great!  Let us know if you need help.

Comment 10

[Anuj Agarwal [:anujagarwal464]]

Attachment: bug1028139.diff

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 12

[Eric Rescorla (:ekr)]

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.

Comment 13

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

(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).

Comment 14

[Andrew McCreight [:mccr8]]

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>.

Comment 15

[Anuj Agarwal [:anujagarwal464]]

Attachment: bug1028139.diff

@mccr8: Thank you! :)

Comment 16

[Andrew McCreight [:mccr8]]

Attachment: Privatizing public destructor of DataBuffer.

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

Comment 17

[Andrew McCreight [:mccr8]]

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.

Comment 18

[Andrew McCreight [:mccr8]]

Attachment: DataBuffer should not be a refcounted class.

I'm just fixing up the commit message.  Carrying forward jesup's r+.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc19e46ce305

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fc19e46ce305