Closed
Bug 964092
Opened 10 years ago
Closed 8 years ago
Don't let DataChannels be GC'd if there's an onfoo attached to them
Categories
(Core :: WebRTC: Signaling, defect, P2)
Core
WebRTC: Signaling
Tracking
()
RESOLVED
FIXED
mozilla34
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
backlog | webrtc/webaudio+ |
People
(Reporter: jesup, Assigned: jesup)
Details
Attachments
(4 files, 1 obsolete file)
8.80 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
2.45 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
8.48 KB,
patch
|
smaug
:
review+
jib
:
review+
|
Details | Diff | Splinter Review |
2.98 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•10 years ago
|
Priority: -- → P2
Target Milestone: --- → mozilla33
Updated•10 years ago
|
Whiteboard: p=1
Updated•10 years ago
|
Target Milestone: mozilla33 → mozilla34
Comment 1•10 years ago
|
||
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
Updated•9 years ago
|
backlog: --- → webRTC+
Points: --- → 1
Rank: 25
Whiteboard: p=1
Comment 2•8 years ago
|
||
*bump* the chrome bug has just been fixed ;-)
Assignee | ||
Comment 3•8 years ago
|
||
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)
Comment 4•8 years ago
|
||
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.
Comment 5•8 years ago
|
||
Websocket also observes the state of the window in WebSocketImpl::Observe.
Assignee | ||
Comment 6•8 years ago
|
||
(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).
Assignee | ||
Comment 7•8 years ago
|
||
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
Assignee | ||
Comment 8•8 years ago
|
||
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.
Assignee | ||
Comment 9•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8744517 -
Attachment is obsolete: true
Attachment #8744517 -
Flags: review?(bugs)
Comment 10•8 years ago
|
||
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)
Assignee | ||
Comment 11•8 years ago
|
||
(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)
Assignee | ||
Comment 12•8 years ago
|
||
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
Assignee | ||
Comment 13•8 years ago
|
||
MozReview-Commit-ID: 9QsuebQhLCd
Attachment #8744596 -
Flags: review?(bugs)
Comment 14•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8744596 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 15•8 years ago
|
||
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
Assignee | ||
Comment 16•8 years ago
|
||
MozReview-Commit-ID: 2iPT1ZT2eLP
Attachment #8745041 -
Flags: review?(jib)
Attachment #8745041 -
Flags: review?(bugs)
Comment 17•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8745041 -
Flags: review?(jib) → review+
Assignee | ||
Comment 18•8 years ago
|
||
(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.
Comment 19•8 years ago
|
||
So we need to have something outside the if (mListenerManager) check.
Assignee | ||
Comment 20•8 years ago
|
||
(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
Assignee | ||
Comment 21•8 years ago
|
||
MozReview-Commit-ID: EqfoHUOlxP8
Attachment #8745166 -
Flags: review?(bugs)
Updated•8 years ago
|
Attachment #8745166 -
Flags: review?(bugs) → review+
Comment 23•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7471df96cc3a
You need to log in
before you can comment on or make changes to this bug.
Description
•