807929 - DataChannel object needs to be refcounted - hard to manage the lifetime during deferring close

Status: VERIFIED FIXED

(Core :: WebRTC: Networking, defect)

Description

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

The raw pointers to DataChannels in the mStreamsOut and mStreamsIn arrays, and ::Close queuing the actual close to be handled by resetting the stream caused a lifetime problem for DataChannel.  Also a known problem was managing the lifetime during a runnable.

Switching to a refcounted (threadsafe!) impl fixes most of these things. Some fancy footwork is required because nsDeque isn't a template library, so it's hard to store already_AddRefed<> objects in it directly.

Note that this means a number of things like NS_NewDOMDataChannel take already_AddRefed<DataChannel>'s, etc.

Comment 1

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

Attachment: Make DataChannel refcounted

Comment 2

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

This was crashing in shutdown in Ted's testcase from Bug 807647 once the patch there is applied

Comment 3

[Patrick McManus [:mcmanus]]

Comment on attachment 677700 [details] [diff] [review]
Make DataChannel refcounted

Review of attachment 677700 [details] [diff] [review]:
-----------------------------------------------------------------

I like the ref counts and anything that gets rid of nsAutoPtr I generally approve of. (I dislike the syntax where a = b ends up modifying b.. )

but you've got more uses of already_addrefed in here than I've ever seen.. maybe its just a style thing but I think this would be a lot more reliable if you could get rid of them. I understand you need it for deque, but that can be much more limited, yes?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +660,5 @@
>    NS_ENSURE_TRUE(dataChannel,NS_ERROR_FAILURE);
>  
>    CSFLogDebugS(logTag, __FUNCTION__ << ": making DOMDataChannel");
>  
> +  return NS_NewDOMDataChannel(dataChannel.forget(), mWindow, aRetval);

here is a good example.. you are forgetting a reference just to get an already addref'd pointer that gets assigned to a refptr inside DOMDataChannel.

why not just pass in dataChannel in this function as a plain DataChannel * in the dom datachannel prototypes which then gets assigned (this time adding a reference) to a refptr. The dataChannel on this stack will soon go out of scope and drop the corresponding reference so they'll still balance.

that's the normal pattern afaik.

::: netwerk/sctp/datachannel/DataChannel.cpp
@@ +972,3 @@
>      channel->mFlags |= DATA_CHANNEL_FLAGS_FINISH_RSP;
> +    channel->AddRef();
> +    mPending.Push(channel); // Can't cast away already_AddRefed<> from channel.forget()

DataChannel *tmp = channel.get();
channel.forget();
mPending.Push(tmp);
 ??

Comment 4

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

(In reply to Patrick McManus [:mcmanus] from comment #3)
> Comment on attachment 677700 [details] [diff] [review]
> Make DataChannel refcounted
> 
> Review of attachment 677700 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I like the ref counts and anything that gets rid of nsAutoPtr I generally
> approve of. (I dislike the syntax where a = b ends up modifying b.. )
> 
> but you've got more uses of already_addrefed in here than I've ever seen..
> maybe its just a style thing but I think this would be a lot more reliable
> if you could get rid of them. I understand you need it for deque, but that
> can be much more limited, yes?

I also need (ok, it saves add/release cycles) to use them in ::Open() and
in HandleOpenRequest(). These need to be internally asynchronous (::Open and ::OpenFinish),
and so  need to keep references.  Since they end up holding a reference, and are
"giving it up" in returning it, .forget() makes sense, which means
already_AddRefed<> (or equivalent; there are several pseudo-synonyms).

I think the API chosen for a lot of things in the tree does something similar, but ends up using
getter_AddRefs().

I'll comb through an eliminate ones I can.

> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
> @@ +660,5 @@
> >    NS_ENSURE_TRUE(dataChannel,NS_ERROR_FAILURE);
> >  
> >    CSFLogDebugS(logTag, __FUNCTION__ << ": making DOMDataChannel");
> >  
> > +  return NS_NewDOMDataChannel(dataChannel.forget(), mWindow, aRetval);
> 
> here is a good example.. you are forgetting a reference just to get an
> already addref'd pointer that gets assigned to a refptr inside
> DOMDataChannel.

That one is ::Open - it has to have a reference, and then return it and give it up
without having it destroyed.  I could return it as a raw pointer from a .forget(),
but that's just losing the compiler checking things for me.

> ::: netwerk/sctp/datachannel/DataChannel.cpp
> @@ +972,3 @@
> >      channel->mFlags |= DATA_CHANNEL_FLAGS_FINISH_RSP;
> > +    channel->AddRef();
> > +    mPending.Push(channel); // Can't cast away already_AddRefed<> from channel.forget()
> 
> DataChannel *tmp = channel.get();
> channel.forget();
> mPending.Push(tmp);
>  ??

Ah, yes, that avoids the add/release pair, thanks.  Fought with the compiler for a while there with casts and basically gave up.

Comment 5

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

Attachment: Delta patch from review comments

Comment 6

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

Attachment: Make DataChannel refcounted

Comment 7

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

Comment on attachment 677784 [details] [diff] [review]
Make DataChannel refcounted

https://bug807929.bugzilla.mozilla.org/attachment.cgi?id=677783
has the delta between this patch and the last.

Reviewing it, all the already_AddRefed<> uses were either ::Open/OpenFinish or ::HandleOpenRequest/Finish, which per before need a ref internally, and the nsDeque stuff.

One of the nsDeque AddRef()s is still needed (we need to add it to a deque and return an already_AddRefed<> pointer)

I'm open to a different pattern, but this was what worked with the code paths without a redesign (and I'm not sure that would help anyways).

Comment 8

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

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

May make sense to upload this after a short bake/verify

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Jason Smith [:jsmith]]

Verified through testing the other bug.