Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
342683002:
Oilpan: Use weak processing to stop a closed RTCPeerConnection (Closed)
|
Created:
6 years, 6 months ago by keishi Modified:
6 years, 6 months ago CC:
blink-reviews, tommyw+watchlist_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Project:
blink Visibility:
Public. |
DescriptionOilpan: Use weak processing to stop a closed RTCPeerConnection
BUG=373690
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176481
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fixed #
Total comments: 10
Patch Set 3 : Fixed #
Total comments: 3
Messages
Total messages: 17 (0 generated)
https://codereview.chromium.org/342683002/diff/1/Source/modules/mediastream/R... File Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/342683002/diff/1/Source/modules/mediastream/R... Source/modules/mediastream/RTCPeerConnection.cpp:746: if (!visitor->isAlive(this) && m_closed) This will not work because weak processing only happens when the trace method is called. The trace method is only called when the object is alive and therefore this condition will always be false because we will only get here when visitor->isAlive(this). When the RTCPeerConnection dies, what we need to make sure is that the RTCDataChannels are stopped (that is the only thing that RTCPeerConnection::stop does that matters when RTCPeerConnection is dead). So, we should add a weak pointer from RTCDataChannel to RTCPeerConnection and add custom weak processing for that one to make sure that we call stop on the DataChannel when it outlives the PeerConnection.
https://codereview.chromium.org/342683002/diff/1/Source/modules/mediastream/R... File Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/342683002/diff/1/Source/modules/mediastream/R... Source/modules/mediastream/RTCPeerConnection.cpp:746: if (!visitor->isAlive(this) && m_closed) On 2014/06/18 08:19:50, Mads Ager (chromium) wrote: > This will not work because weak processing only happens when the trace method is > called. The trace method is only called when the object is alive and therefore > this condition will always be false because we will only get here when > visitor->isAlive(this). > > When the RTCPeerConnection dies, what we need to make sure is that the > RTCDataChannels are stopped (that is the only thing that RTCPeerConnection::stop > does that matters when RTCPeerConnection is dead). > > So, we should add a weak pointer from RTCDataChannel to RTCPeerConnection and > add custom weak processing for that one to make sure that we call stop on the > DataChannel when it outlives the PeerConnection. Done.
LGTM https://codereview.chromium.org/342683002/diff/20001/Source/modules/mediastre... File Source/modules/mediastream/RTCDataChannel.cpp (right): https://codereview.chromium.org/342683002/diff/20001/Source/modules/mediastre... Source/modules/mediastream/RTCDataChannel.cpp:87: m_handler->setClient(0); Do we need to guard this: if (!m_stopped) m_handler->setClient(0); ? Not sure if it matters to get multiple setClient(0) calls? In any case we should probably make the difference between non-oilpan and oilpan clear here. How about: #if ENABLE(OILPAN) // If the peer connection and the data channel die in the same // GC cycle stop has not been called and we need to notify the // client that the channel is gone. if (!m_stopped) m_handler->setClient(0); #else ASSERT(m_stopped); #endif https://codereview.chromium.org/342683002/diff/20001/Source/modules/mediastre... Source/modules/mediastream/RTCDataChannel.cpp:331: #if ENABLE(OILPAN) Do you need this #if? If you do I would just put the entire body inside of the #if since non of this code will be run non-oilpan anyway.
LGTM. https://codereview.chromium.org/342683002/diff/20001/Source/modules/mediastre... File Source/modules/mediastream/RTCDataChannel.cpp (right): https://codereview.chromium.org/342683002/diff/20001/Source/modules/mediastre... Source/modules/mediastream/RTCDataChannel.cpp:87: m_handler->setClient(0); On 2014/06/18 09:09:26, Mads Ager (chromium) wrote: > Do we need to guard this: > > if (!m_stopped) > m_handler->setClient(0); > > ? > > Not sure if it matters to get multiple setClient(0) calls? > > In any case we should probably make the difference between non-oilpan and oilpan > clear here. How about: > > #if ENABLE(OILPAN) > // If the peer connection and the data channel die in the same > // GC cycle stop has not been called and we need to notify the > // client that the channel is gone. > if (!m_stopped) > m_handler->setClient(0); > #else > ASSERT(m_stopped); > #endif Looks nicer. https://codereview.chromium.org/342683002/diff/20001/Source/modules/mediastre... Source/modules/mediastream/RTCDataChannel.cpp:332: visitor->trace(m_connection); You can remove this trace, and explicitly do 'm_connection = nullptr' in clearWeakMembers(). https://codereview.chromium.org/342683002/diff/20001/Source/modules/mediastre... File Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/342683002/diff/20001/Source/modules/mediastre... Source/modules/mediastream/RTCPeerConnection.cpp:491: ASSERT(m_peerHandler); What is this ASSERT for?
https://codereview.chromium.org/342683002/diff/20001/Source/modules/mediastre... File Source/modules/mediastream/RTCDataChannel.cpp (right): https://codereview.chromium.org/342683002/diff/20001/Source/modules/mediastre... Source/modules/mediastream/RTCDataChannel.cpp:332: visitor->trace(m_connection); On 2014/06/18 09:17:08, haraken wrote: > > You can remove this trace, and explicitly do 'm_connection = nullptr' in > clearWeakMembers(). Auch, good catch Haraken! Actually you *have* to remove this one. Otherwise m_connection will have been set to 0 before you get into clearWeakMembers (because weak cell callbacks are run before object weak callbacks).
https://codereview.chromium.org/342683002/diff/20001/Source/modules/mediastre... File Source/modules/mediastream/RTCDataChannel.cpp (right): https://codereview.chromium.org/342683002/diff/20001/Source/modules/mediastre... Source/modules/mediastream/RTCDataChannel.cpp:87: m_handler->setClient(0); On 2014/06/18 09:09:26, Mads Ager (chromium) wrote: > Do we need to guard this: > > if (!m_stopped) > m_handler->setClient(0); > > ? > > Not sure if it matters to get multiple setClient(0) calls? > > In any case we should probably make the difference between non-oilpan and oilpan > clear here. How about: > > #if ENABLE(OILPAN) > // If the peer connection and the data channel die in the same > // GC cycle stop has not been called and we need to notify the > // client that the channel is gone. > if (!m_stopped) > m_handler->setClient(0); > #else > ASSERT(m_stopped); > #endif Done. https://codereview.chromium.org/342683002/diff/20001/Source/modules/mediastre... Source/modules/mediastream/RTCDataChannel.cpp:331: #if ENABLE(OILPAN) On 2014/06/18 09:09:26, Mads Ager (chromium) wrote: > Do you need this #if? If you do I would just put the entire body inside of the > #if since non of this code will be run non-oilpan anyway. Done. We need it because m_connection is oilpan only https://codereview.chromium.org/342683002/diff/20001/Source/modules/mediastre... Source/modules/mediastream/RTCDataChannel.cpp:332: visitor->trace(m_connection); On 2014/06/18 09:17:08, haraken wrote: > > You can remove this trace, and explicitly do 'm_connection = nullptr' in > clearWeakMembers(). Done. https://codereview.chromium.org/342683002/diff/20001/Source/modules/mediastre... File Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/342683002/diff/20001/Source/modules/mediastre... Source/modules/mediastream/RTCPeerConnection.cpp:491: ASSERT(m_peerHandler); On 2014/06/18 09:17:09, haraken wrote: > > What is this ASSERT for? Removed.
https://codereview.chromium.org/342683002/diff/40001/Source/modules/mediastre... File Source/modules/mediastream/RTCDataChannel.cpp (right): https://codereview.chromium.org/342683002/diff/40001/Source/modules/mediastre... Source/modules/mediastream/RTCDataChannel.cpp:94: ASSERT(m_stopped); I'm not sure why we would assert this?
https://codereview.chromium.org/342683002/diff/40001/Source/modules/mediastre... File Source/modules/mediastream/RTCDataChannel.cpp (right): https://codereview.chromium.org/342683002/diff/40001/Source/modules/mediastre... Source/modules/mediastream/RTCDataChannel.cpp:94: ASSERT(m_stopped); On 2014/06/18 11:49:13, Henrik Grunell wrote: > I'm not sure why we would assert this? Is it not the case that an RTCDataChannel needs to call m_handler->setClient(0) before it dies? That is done via stop when the owning RTCPeerConnection dies and this assert verifies that it has been done and adds documentation that without oilpan that is the expectation. The reason why I suggested adding it is that it gives context for the oilpan code above. That we need to call setClient(0) if we are dying in the same GC round as the RTCPeerConnection. That said, I'm perfectly fine with removing the ASSERT if it is wrong or if you do not find it helpful. :)
https://codereview.chromium.org/342683002/diff/40001/Source/modules/mediastre... File Source/modules/mediastream/RTCDataChannel.cpp (right): https://codereview.chromium.org/342683002/diff/40001/Source/modules/mediastre... Source/modules/mediastream/RTCDataChannel.cpp:94: ASSERT(m_stopped); On 2014/06/18 12:35:13, Mads Ager (chromium) wrote: > On 2014/06/18 11:49:13, Henrik Grunell wrote: > > I'm not sure why we would assert this? > > Is it not the case that an RTCDataChannel needs to call m_handler->setClient(0) > before it dies? That is done via stop when the owning RTCPeerConnection dies and > this assert verifies that it has been done and adds documentation that without > oilpan that is the expectation. The reason why I suggested adding it is that it > gives context for the oilpan code above. That we need to call setClient(0) if we > are dying in the same GC round as the RTCPeerConnection. I'm by no means an expert on the data channels itself (or blink for that matter). From what I can see, hasActivityPending isn't overridden so it could be deleted after all refs are released and that could happen without stop being called. And |m_handler| would be deleted when the data channel is deleted avoiding calls into the deleted object. Does this sound correct? > > That said, I'm perfectly fine with removing the ASSERT if it is wrong or if you > do not find it helpful. :)
On Wed, Jun 18, 2014 at 8:21 PM, <grunell@chromium.org> wrote: > > https://codereview.chromium.org/342683002/diff/40001/ > Source/modules/mediastream/RTCDataChannel.cpp > File Source/modules/mediastream/RTCDataChannel.cpp (right): > > https://codereview.chromium.org/342683002/diff/40001/ > Source/modules/mediastream/RTCDataChannel.cpp#newcode94 > Source/modules/mediastream/RTCDataChannel.cpp:94: ASSERT(m_stopped); > On 2014/06/18 12:35:13, Mads Ager (chromium) wrote: > >> On 2014/06/18 11:49:13, Henrik Grunell wrote: >> > I'm not sure why we would assert this? >> > > Is it not the case that an RTCDataChannel needs to call >> > m_handler->setClient(0) > >> before it dies? That is done via stop when the owning >> > RTCPeerConnection dies and > >> this assert verifies that it has been done and adds documentation that >> > without > >> oilpan that is the expectation. The reason why I suggested adding it >> > is that it > >> gives context for the oilpan code above. That we need to call >> > setClient(0) if we > >> are dying in the same GC round as the RTCPeerConnection. >> > > I'm by no means an expert on the data channels itself (or blink for that > matter). From what I can see, hasActivityPending isn't overridden so it > could be deleted after all refs are released and that could happen > without stop being called. And |m_handler| would be deleted when the > data channel is deleted avoiding calls into the deleted object. Does > this sound correct? RTCDataChannels are only created in RTCPeerConnection. In these two places: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... In both cases the DataChannel is added to the list of data channels for the PeerConnection. That list is a list of RefPtrs to DataChannels so they cannot die before they are removed from that list. However, that only happens when the PeerConnection calls stop or dies and therefore calls stop (which calls stop on all data channels). So, no, I don't see how a DataChannel can die without a call to stop via the PeerConnection. :) Cheers, -- Mads To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On 2014/06/18 18:44:29, Mads Ager (chromium) wrote: > On Wed, Jun 18, 2014 at 8:21 PM, <mailto:grunell@chromium.org> wrote: > > > > > https://codereview.chromium.org/342683002/diff/40001/ > > Source/modules/mediastream/RTCDataChannel.cpp > > File Source/modules/mediastream/RTCDataChannel.cpp (right): > > > > https://codereview.chromium.org/342683002/diff/40001/ > > Source/modules/mediastream/RTCDataChannel.cpp#newcode94 > > Source/modules/mediastream/RTCDataChannel.cpp:94: ASSERT(m_stopped); > > On 2014/06/18 12:35:13, Mads Ager (chromium) wrote: > > > >> On 2014/06/18 11:49:13, Henrik Grunell wrote: > >> > I'm not sure why we would assert this? > >> > > > > Is it not the case that an RTCDataChannel needs to call > >> > > m_handler->setClient(0) > > > >> before it dies? That is done via stop when the owning > >> > > RTCPeerConnection dies and > > > >> this assert verifies that it has been done and adds documentation that > >> > > without > > > >> oilpan that is the expectation. The reason why I suggested adding it > >> > > is that it > > > >> gives context for the oilpan code above. That we need to call > >> > > setClient(0) if we > > > >> are dying in the same GC round as the RTCPeerConnection. > >> > > > > I'm by no means an expert on the data channels itself (or blink for that > > matter). From what I can see, hasActivityPending isn't overridden so it > > could be deleted after all refs are released and that could happen > > without stop being called. And |m_handler| would be deleted when the > > data channel is deleted avoiding calls into the deleted object. Does > > this sound correct? > > > RTCDataChannels are only created in RTCPeerConnection. In these two places: > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > In both cases the DataChannel is added to the list of data channels for the > PeerConnection. That list is a list of RefPtrs to DataChannels so they > cannot die before they are removed from that list. However, that only > happens when the PeerConnection calls stop or dies and therefore calls stop > (which calls stop on all data channels). So, no, I don't see how a > DataChannel can die without a call to stop via the PeerConnection. :) > > Cheers, -- Mads > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:blink-reviews+unsubscribe@chromium.org. Of course, the PC reference. LGTM
The CQ bit was checked by keishi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/keishi@chromium.org/342683002/40001
Message was sent while issue was closed.
Change committed as 176481 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
