Closed Bug 964092 Opened 6 years ago Closed 4 years ago

Don't let DataChannels be GC'd if there's an onfoo attached to them

Categories

(Core :: WebRTC: Signaling, defect, P2)

defect
Points:
1

Tracking

()

RESOLVED FIXED
mozilla34
Tracking Status
firefox49 --- fixed
Blocking Flags:

People

(Reporter: jesup, Assigned: jesup)

Details

Attachments

(4 files, 1 obsolete file)

This is needed to match the spec and mirror how WebSockets handles GC. Note that when the DataChannel closes, it should become available for GC/CC.
Priority: -- → P2
Target Milestone: --- → mozilla33
Whiteboard: p=1
Target Milestone: mozilla33 → mozilla34
I just filed https://code.google.com/p/chromium/issues/detail?id=405545 when hitting the same problem in chrome.

http://hancke.name/tmp/dc-loss/ can be used to reproduce this. Fortunately, the close callback gets called in Firefox which makes this alot easier to notice
backlog: --- → webRTC+
Points: --- → 1
Rank: 25
Whiteboard: p=1
*bump* the chrome bug has just been fixed ;-)
Since this is all about DOM lifetimes and spec compliance, r? to smaug.  Tested on the dc-loss URL; the one channel that gets .close()ed get's GC'd, and until then GCs have no impact.  I'll also modify it to drop the callbacks and verify it get's gc'd.
Attachment #8744517 - Flags: review?(bugs)
If the DataChannel is open when the window is closed/collected, do we end up keeping the channel open forever? Or is nsDOMDataChannel::OnChannelClosed called when window is closed?

WebSocket::DisconnectFromOwner() for example closes the connection explicitly.
Websocket also observes the state of the window in WebSocketImpl::Observe.
(In reply to Olli Pettay [:smaug] from comment #4)
> If the DataChannel is open when the window is closed/collected, do we end up
> keeping the channel open forever? Or is nsDOMDataChannel::OnChannelClosed
> called when window is closed?
> 
> WebSocket::DisconnectFromOwner() for example closes the connection
> explicitly.

The channel should be destroyed on window close; I'll check this and revise if needed (in case the notification somehow is blocked; I don't think it is).

WebSocket has a slightly different impl pattern in terms of objects as well, so the code isn't identical (though it's quite similar).
On tab close (+GC), 1 of the two channels (send and receive) is being GC'd.  I'll investigate why the other isn't
likely a simple fix
Actually it appears both are being GC'd just one took an extra GC cycle, but not reliably.  I added a ON_CHANNEL_CLOSED dispatch from the backend code on connection/PeerConnection close, and it's now reliable.
Also verified by modifying the test page that removing the remaining onfoo's inside of an onfoo makes it available for GC
Attachment #8744550 - Flags: review?(bugs)
Attachment #8744517 - Attachment is obsolete: true
Attachment #8744517 - Flags: review?(bugs)
Could you explain a bit how the channels are closed when window is closed? What code where ends up closing the channels?
Flags: needinfo?(rjesup)
(In reply to Olli Pettay [:smaug] from comment #10)
> Could you explain a bit how the channels are closed when window is closed?
> What code where ends up closing the channels?

When a window/tab is closed, or on navigation in the tab, all PeerConnections associated with that window are closed (see PeerConnection.js).  This leads to PeerConnectionImpl::CloseInt() in media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp; this calls mDataConnection->Destroy(), which closes all DataChannels for the connection (DataChannelConnection::CloseAll() in netwerk/sctp/datachannel/DataChannel.cpp).  That calls DataChannel::Close() for each open channel, and that is where I added the Dispatch of ON_CHANNEL_CLOSED.
Flags: needinfo?(rjesup)
I should also double-check I'm not double-reporting Closed() now in any path.  I don't think we were reliably reporting Closed in all circumstances, so this may actually help things elsewhere, but I want to be sure we can't report it twice - probably a safety boolean in the DOM code to only call the onclose max once is the best/safest solution, and just requires the low-level code to notify us on all closure/failure paths
MozReview-Commit-ID: 9QsuebQhLCd
Attachment #8744596 - Flags: review?(bugs)
Comment on attachment 8744550 [details] [diff] [review]
don't let DOM DataChannels get GC'd if they have an active callback

> nsDOMDataChannel::OnChannelClosed(nsISupports* aContext)
> {
>   LOG(("%p(%p): %s - Dispatching\n",this,(void*)mDataChannel,__FUNCTION__));
> 
>-  return OnSimpleEvent(aContext, NS_LITERAL_STRING("close"));
>+  nsresult rv = OnSimpleEvent(aContext, NS_LITERAL_STRING("close"));
>+  // no more events can happen
>+  RefPtr<nsDOMDataChannel> self = this;
>+  // ATTENTION, when calling this method the object can be released
>+  // (and possibly collected).
>+  DontKeepAliveAnyMore();
>+  return rv;
Don't you want to do RefPtr<nsDOMDataChannel> self = this;
before dispatching the event, since event listener might have already removed the listeners and nothing, expect possibly the caller, is keeping the object alive.
But then, DontKeepAliveAnyMore is doing release async, so don't see the reason for self.
Please clarify.

>+      case DataChannel::CONNECTING:
>+      case DataChannel::WAITING_TO_OPEN:
>+      {
>+        if (mListenerManager->HasListenersFor(nsGkAtoms::onopen) ||
>+            mListenerManager->HasListenersFor(nsGkAtoms::onmessage) ||
>+            mListenerManager->HasListenersFor(nsGkAtoms::onerror) ||
>+            mListenerManager->HasListenersFor(nsGkAtoms::onclose)) {
Can mDataChannel->GetBufferedAmount() be non-null here, and if yes, should it keep the object alive?



>+++ b/netwerk/sctp/datachannel/DataChannel.cpp
>@@ -2567,16 +2567,19 @@ DataChannel::DestroyLocked()
>   mConnection->mLock.AssertCurrentThreadOwns();
>   ENSURE_DATACONNECTION;
> 
>   LOG(("Destroying Data channel %u", mStream));
>   MOZ_ASSERT_IF(mStream != INVALID_STREAM,
>                 !mConnection->FindChannelByStream(mStream));
>   mStream = INVALID_STREAM;
>   mState = CLOSED;
>+  NS_DispatchToMainThread(do_AddRef(new DataChannelOnMessageAvailable(
>+                                      DataChannelOnMessageAvailable::ON_CHANNEL_CLOSED,
>+                                      mConnection, this)));
This part should be reviewed by someone else I think.
Attachment #8744550 - Flags: review?(bugs) → review+
Attachment #8744596 - Flags: review?(bugs) → review+
Thanks for the detailed review!

(In reply to Olli Pettay [:smaug] from comment #14)
> Comment on attachment 8744550 [details] [diff] [review]
> don't let DOM DataChannels get GC'd if they have an active callback
> 
> > nsDOMDataChannel::OnChannelClosed(nsISupports* aContext)
> > {
> >   LOG(("%p(%p): %s - Dispatching\n",this,(void*)mDataChannel,__FUNCTION__));
> > 
> >-  return OnSimpleEvent(aContext, NS_LITERAL_STRING("close"));
> >+  nsresult rv = OnSimpleEvent(aContext, NS_LITERAL_STRING("close"));
> >+  // no more events can happen
> >+  RefPtr<nsDOMDataChannel> self = this;
> >+  // ATTENTION, when calling this method the object can be released
> >+  // (and possibly collected).
> >+  DontKeepAliveAnyMore();
> >+  return rv;
> Don't you want to do RefPtr<nsDOMDataChannel> self = this;
> before dispatching the event, since event listener might have already
> removed the listeners and nothing, expect possibly the caller, is keeping
> the object alive.

The caller of OnChannelClosed holds a ref to us (and a lock in fact).

> But then, DontKeepAliveAnyMore is doing release async, so don't see the
> reason for self.
> Please clarify.

Since we force it to release async (true for second param), it can't be released before we finish, so long as we're on mainthread, which we assert.  So the self-ref here isn't needed (though that presumes things about the internal implementation of DontKeepAliveAnyMore()).  And per above, the caller of OnChannelClosed has a ref anyways.  So we can remove this.

> 
> >+      case DataChannel::CONNECTING:
> >+      case DataChannel::WAITING_TO_OPEN:
> >+      {
> >+        if (mListenerManager->HasListenersFor(nsGkAtoms::onopen) ||
> >+            mListenerManager->HasListenersFor(nsGkAtoms::onmessage) ||
> >+            mListenerManager->HasListenersFor(nsGkAtoms::onerror) ||
> >+            mListenerManager->HasListenersFor(nsGkAtoms::onclose)) {
> Can mDataChannel->GetBufferedAmount() be non-null here, and if yes, should
> it keep the object alive?

mDataChannel must exist until the nsDOMDataChannel is deleted (see ~nsDOMDataChannel above).
We would keep it alive if it was buffering, but wouldn't release that when the buffer drained.  Added a message to tell the DOM code that buffering has ended so it can update the keepalive state.

I noticed that we also need to watch for ::onbufferedamountlow (which WebSockets doesn't have).

I'll upload an interdiff to avoid confusion
MozReview-Commit-ID: 2iPT1ZT2eLP
Attachment #8745041 - Flags: review?(jib)
Attachment #8745041 - Flags: review?(bugs)
Comment on attachment 8745041 [details] [diff] [review]
interdiffs for review

>       case DataChannel::OPEN:
>       case DataChannel::CLOSING:
>       {
>         if (mListenerManager->HasListenersFor(nsGkAtoms::onmessage) ||
>             mListenerManager->HasListenersFor(nsGkAtoms::onerror) ||
>+            mListenerManager->HasListenersFor(nsGkAtoms::onbufferedamountlow) ||
>             mListenerManager->HasListenersFor(nsGkAtoms::onclose) ||
>             mDataChannel->GetBufferedAmount() != 0) {
Now I realized... do we need to care about mDataChannel->GetBufferedAmount() != 0 always when state is OPEN or CLOSING, since this block is executed only when there is mEventListenerManager.
So what if there are no listeners added and state OPEN or CLOSING and mDataChannel->GetBufferedAmount() != 0

That fixed or explained, r+
Attachment #8745041 - Flags: review?(bugs) → review+
Attachment #8745041 - Flags: review?(jib) → review+
(In reply to Olli Pettay [:smaug] from comment #17)
> Comment on attachment 8745041 [details] [diff] [review]
> interdiffs for review
> 
> >       case DataChannel::OPEN:
> >       case DataChannel::CLOSING:
> >       {
> >         if (mListenerManager->HasListenersFor(nsGkAtoms::onmessage) ||
> >             mListenerManager->HasListenersFor(nsGkAtoms::onerror) ||
> >+            mListenerManager->HasListenersFor(nsGkAtoms::onbufferedamountlow) ||
> >             mListenerManager->HasListenersFor(nsGkAtoms::onclose) ||
> >             mDataChannel->GetBufferedAmount() != 0) {
> Now I realized... do we need to care about mDataChannel->GetBufferedAmount()
> != 0 always when state is OPEN or CLOSING, since this block is executed only
> when there is mEventListenerManager.
> So what if there are no listeners added and state OPEN or CLOSING and
> mDataChannel->GetBufferedAmount() != 0
> 
> That fixed or explained, r+

If we have data buffered, we need to keep it alive until the data is drained.  We can only have data buffered in OPEN or CLOSING (once CLOSED, we're good letting everything go away).  I.e. we may not be called back by any event if all listeners and local refs are gone, but there's still state that needs to be transmitted to the *other* end.
So we need to have something outside the if (mListenerManager) check.
(In reply to Olli Pettay [:smaug] from comment #19)
> So we need to have something outside the if (mListenerManager) check.

I see your point now.  I'll re-arrange the if()'s
MozReview-Commit-ID: EqfoHUOlxP8
Attachment #8745166 - Flags: review?(bugs)
Attachment #8745166 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/7471df96cc3a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.