Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
329093002:
Allow PeerConnection to be garbage collected after close(). (Closed)
|
Created:
6 years, 6 months ago by Henrik Grunell Modified:
6 years, 6 months ago Reviewers:
tommi (sloooow) - chröme, Mads Ager (chromium), keishi, abarth-chromium, haraken, oilpan-reviews, Julien - ping for review CC:
blink-reviews, tommyw+watchlist_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionAllow PeerConnection to be garbage collected after close().
* Add m_closed flag and allow gc after close().
* Assert that no calls from the client comes after close().
* Ensure no callbacks from async operation after close() is called, except getStats.
* Call stop() in the destructor.
* Add tests for garbage collected after close(), but not gc if close() is not called.
* Add tests for getStats() called after close().
BUG=373690
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176349
Patch Set 1 #Patch Set 2 : Use ref counting for pending activity for a async operations. #
Total comments: 4
Patch Set 3 : Code review, fix and clean up todo comments. #Patch Set 4 : Some fixes, added test for getStats after close. #Patch Set 5 : Minor fix. #Patch Set 6 : Added gc test using observeGC. #Patch Set 7 : Rebase #
Total comments: 28
Patch Set 8 : Code review. #Patch Set 9 : Data channel now has a weak ptr to the peer connection that created it. Stops itself if creator goe… #
Total comments: 10
Patch Set 10 : Code review. #
Total comments: 3
Patch Set 11 : Rebase #Patch Set 12 : Reverted to call stop() in the destructor. No ptr to PeerConnection in DataChannel. #Patch Set 13 : Code review. #
Total comments: 14
Patch Set 14 : Code review. #Messages
Total messages: 55 (0 generated)
This is a first stab at allowing PeerConnections to be garbage collected. Adding Julien since we looked a little bit at this together. There's some uncertainties still I'm bringing up in mail thread.
Adding some better reviewers for a garbage collection + ActiveDOMObject change.
I'm going to be away for the next days, so just rubberstamping. lgtm.
I don't fully understand what lifetime problem you're going to solve. Would you elaborate on it a bit more? Also you need to add a test for this change. https://codereview.chromium.org/329093002/diff/20001/Source/modules/mediastre... File Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/329093002/diff/20001/Source/modules/mediastre... Source/modules/mediastream/RTCPeerConnection.cpp:203: setPendingActivity(this); setPendingActivity/unsetPendingActivity are deprecated, so please keep using hasPendingActivity. I guess you can return something like '!m_closed || !m_stopped' from hasPendingActivity? https://codereview.chromium.org/329093002/diff/20001/Source/modules/mediastre... File Source/modules/mediastream/RTCSessionDescriptionRequestImpl.h (right): https://codereview.chromium.org/329093002/diff/20001/Source/modules/mediastre... Source/modules/mediastream/RTCSessionDescriptionRequestImpl.h:69: RTCPeerConnection* m_requester; I guess this needs to be a RefPtr. Otherwise, the RTCPeerConnection object can be gone while RTCSessionDescriptionRequest is alive. If we use a RefPtr here, do we have a risk of creating a reference cycle? (As far as I see, there would be no risk.)
On 2014/06/12 18:32:00, tommi (OOO June 13-23) wrote: > I'm going to be away for the next days, so just rubberstamping. lgtm. Please don't rubberstamp this kind of non-trivial CL. This CL still has a couple of "TODO BEFORE COMMIT"s and is definitely not in a state where we can commit.
On 2014/06/13 01:25:05, haraken wrote: > On 2014/06/12 18:32:00, tommi (OOO June 13-23) wrote: > > I'm going to be away for the next days, so just rubberstamping. lgtm. > > Please don't rubberstamp this kind of non-trivial CL. This CL still has a couple > of "TODO BEFORE COMMIT"s and is definitely not in a state where we can commit. Not lgtm. Ok, rubberstamping is not enough for a cl to be committed. I have discussed this with Henrik offline and he is well aware of the review process. There is no risk here. I'm going to be on vacation next week and today so I'm relying on you to do the review - as Henrik has asked for. I'm retracting the rs so im guessing you'll either call me for the stamp or tbr the change?
Adding Mads as well.
Also fixed and cleaned up todo comments. https://codereview.chromium.org/329093002/diff/20001/Source/modules/mediastre... File Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/329093002/diff/20001/Source/modules/mediastre... Source/modules/mediastream/RTCPeerConnection.cpp:203: setPendingActivity(this); On 2014/06/13 01:23:00, haraken wrote: > > setPendingActivity/unsetPendingActivity are deprecated, so please keep using > hasPendingActivity. I guess you can return something like '!m_closed || > !m_stopped' from hasPendingActivity? Oh, I didn't know that. Maybe a comment should be added in ActiveDOMObject.h and http://www.chromium.org/blink/activedomobject be updated? Changed it to use m_closed and m_stopped. https://codereview.chromium.org/329093002/diff/20001/Source/modules/mediastre... File Source/modules/mediastream/RTCSessionDescriptionRequestImpl.h (right): https://codereview.chromium.org/329093002/diff/20001/Source/modules/mediastre... Source/modules/mediastream/RTCSessionDescriptionRequestImpl.h:69: RTCPeerConnection* m_requester; On 2014/06/13 01:23:00, haraken wrote: > > I guess this needs to be a RefPtr. Otherwise, the RTCPeerConnection object can > be gone while RTCSessionDescriptionRequest is alive. Well, it should have been prevented with the ref count using setPendingActivity, but now I've removed that. Changed to RefPtr. > If we use a RefPtr here, do we have a risk of creating a reference cycle? (As > far as I see, there would be no risk.) No, there's no ref cycle.
On 2014/06/13 01:23:00, haraken wrote: > I don't fully understand what lifetime problem you're going to solve. Would you > elaborate on it a bit more? Well, the problem is that PeerConnection objects are never garbage collected. (See repro steps in bug.) Currently, stop() must be called before hasPendingActivity() returns false. We want it to return false after calling close(). > Also you need to add a test for this change. Sure, will do.
I've added a test for calling getStats after close. And fixed some issues found when running existing tests. I have tested manually (dtor printout + force gc) that: * the object is garbage collected with my change and calling close(), * it's not gc'd without the change, and * it's not gc'd with the change but not calling close(). Automatically testing in javascript that the object actually has been garbage collecting is not possible afaik. It's not attached to anything countable. Suggestions for how to do this? C++ test?
On 2014/06/13 14:40:25, Henrik Grunell wrote: > I've added a test for calling getStats after close. And fixed some issues found > when running existing tests. > > I have tested manually (dtor printout + force gc) that: > * the object is garbage collected with my change and calling close(), > * it's not gc'd without the change, and > * it's not gc'd with the change but not calling close(). > > Automatically testing in javascript that the object actually has been garbage > collecting is not possible afaik. It's not attached to anything countable. > Suggestions for how to do this? C++ test? Thanks for testing! I think you can use window.internals.observeGC in layout tests. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
On 2014/06/13 04:32:42, tommi (OOO June 13-23) wrote: > On 2014/06/13 01:25:05, haraken wrote: > > On 2014/06/12 18:32:00, tommi (OOO June 13-23) wrote: > > > I'm going to be away for the next days, so just rubberstamping. lgtm. > > > > Please don't rubberstamp this kind of non-trivial CL. This CL still has a > couple > > of "TODO BEFORE COMMIT"s and is definitely not in a state where we can commit. > > Not lgtm. > > Ok, rubberstamping is not enough for a cl to be committed. I have discussed this > with Henrik offline and he is well aware of the review process. There is no risk > here. I'm going to be on vacation next week and today so I'm relying on you to > do the review - as Henrik has asked for. > I'm retracting the rs so im guessing you'll either call me for the stamp or tbr > the change? I'm sorry if my comment sounded too harsh; I understand your situation. I just wanted to be careful about LG from a module owner because it has a power :)
No worries. Better safe than sorry :-) I'll follow along and wait for things to be ready. On Jun 13, 2014 5:05 PM, <haraken@chromium.org> wrote: > On 2014/06/13 04:32:42, tommi (OOO June 13-23) wrote: > >> On 2014/06/13 01:25:05, haraken wrote: >> > On 2014/06/12 18:32:00, tommi (OOO June 13-23) wrote: >> > > I'm going to be away for the next days, so just rubberstamping. lgtm. >> > >> > Please don't rubberstamp this kind of non-trivial CL. This CL still has >> a >> couple >> > of "TODO BEFORE COMMIT"s and is definitely not in a state where we can >> > commit. > > Not lgtm. >> > > Ok, rubberstamping is not enough for a cl to be committed. I have >> discussed >> > this > >> with Henrik offline and he is well aware of the review process. There is >> no >> > risk > >> here. I'm going to be on vacation next week and today so I'm relying on >> you to >> do the review - as Henrik has asked for. >> I'm retracting the rs so im guessing you'll either call me for the stamp >> or >> > tbr > >> the change? >> > > I'm sorry if my comment sounded too harsh; I understand your situation. I > just > wanted to be careful about LG from a module owner because it has a power :) > > > https://codereview.chromium.org/329093002/ > To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Added gc test using observeGC. PTAL.
+keishi for the RTCPeerConnection destruction issue. https://codereview.chromium.org/329093002/diff/120001/LayoutTests/fast/medias... File LayoutTests/fast/mediastream/RTCPeerConnection-lifetime.html (right): https://codereview.chromium.org/329093002/diff/120001/LayoutTests/fast/medias... LayoutTests/fast/mediastream/RTCPeerConnection-lifetime.html:44: // Test that the PeerConnection object is not gc'd if close is not called. I'm fine with this behavior, but I'm a bit concerned that this behavior will produce a lot of memory leak in real worlds. If people forget to call close() explicitly, the peer connection starts leaking. (I understand this is a separate discussion from this CL though.) https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... File Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... Source/modules/mediastream/RTCPeerConnection.cpp:222: stop(); This is not allowed in oilpan, because in oilpan destructors are not allowed to touch other on-heap objects (since there is no guarantee about the order in which on-heap objects are destructed). keishi@: Do you have any idea on how to address this destruction issue? https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... File Source/modules/mediastream/RTCSessionDescriptionRequestImpl.cpp (right): https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... Source/modules/mediastream/RTCSessionDescriptionRequestImpl.cpp:66: bool mayFireCallback = m_requester ? m_requester->requestMayFireNonStatsCallback() : true; Just help me understand: Why is this 'true', not 'false'? I thought we don't want to fire a callback when there is no requester. https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... File Source/modules/mediastream/RTCSessionDescriptionRequestImpl.h (right): https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... Source/modules/mediastream/RTCSessionDescriptionRequestImpl.h:51: static PassRefPtr<RTCSessionDescriptionRequestImpl> create(ExecutionContext*, PassRefPtr<RTCPeerConnection>, PassOwnPtr<RTCSessionDescriptionCallback>, PassOwnPtr<RTCErrorCallback>); RTCPeerConnection is already on oilpan's heap, so would you use PassRefPtrWillBeRawPtr? https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... Source/modules/mediastream/RTCSessionDescriptionRequestImpl.h:61: RTCSessionDescriptionRequestImpl(ExecutionContext*, PassRefPtr<RTCPeerConnection>, PassOwnPtr<RTCSessionDescriptionCallback>, PassOwnPtr<RTCErrorCallback>); PassRefPtrWillBeRawPtr https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... Source/modules/mediastream/RTCSessionDescriptionRequestImpl.h:68: RefPtr<RTCPeerConnection> m_requester; RefPtrWillBePersistent https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... File Source/modules/mediastream/RTCStatsRequestImpl.h (right): https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... Source/modules/mediastream/RTCStatsRequestImpl.h:43: static PassRefPtr<RTCStatsRequestImpl> create(ExecutionContext*, PassRefPtr<RTCPeerConnection>, PassOwnPtr<RTCStatsCallback>, PassRefPtr<MediaStreamTrack>); PassRefPtrWillBeRawPtr https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... Source/modules/mediastream/RTCStatsRequestImpl.h:56: RTCStatsRequestImpl(ExecutionContext*, PassRefPtr<RTCPeerConnection>, PassOwnPtr<RTCStatsCallback>, PassRefPtr<MediaStreamTrack>); PassRefPtrWillBeRawPtr https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... Source/modules/mediastream/RTCStatsRequestImpl.h:63: RefPtr<RTCPeerConnection> m_requester; RefPtrWillBePersistent https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... File Source/modules/mediastream/RTCVoidRequestImpl.h (right): https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... Source/modules/mediastream/RTCVoidRequestImpl.h:45: static PassRefPtr<RTCVoidRequestImpl> create(ExecutionContext*, PassRefPtr<RTCPeerConnection>, PassOwnPtr<VoidCallback>, PassOwnPtr<RTCErrorCallback>); PassRefPtrWillBeRawPtr https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... Source/modules/mediastream/RTCVoidRequestImpl.h:56: RTCVoidRequestImpl(ExecutionContext*, PassRefPtr<RTCPeerConnection>, PassOwnPtr<VoidCallback>, PassOwnPtr<RTCErrorCallback>); PassRefPtrWillBeRawPtr https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... Source/modules/mediastream/RTCVoidRequestImpl.h:63: RefPtr<RTCPeerConnection> m_requester; RefPtrWillBePersistent
https://codereview.chromium.org/329093002/diff/120001/LayoutTests/fast/medias... File LayoutTests/fast/mediastream/RTCPeerConnection-lifetime.html (right): https://codereview.chromium.org/329093002/diff/120001/LayoutTests/fast/medias... LayoutTests/fast/mediastream/RTCPeerConnection-lifetime.html:44: // Test that the PeerConnection object is not gc'd if close is not called. On 2014/06/15 13:43:57, haraken wrote: > > I'm fine with this behavior, but I'm a bit concerned that this behavior will > produce a lot of memory leak in real worlds. If people forget to call close() > explicitly, the peer connection starts leaking. (I understand this is a separate > discussion from this CL though.) Agree. The proposed spec change only says that it can be garbage collected if in closed state. However, I have brought this up in a mail thread since it makes sense if one does (as a trivial example) var pc = new ...; pc = null; that it should be gc'd. I see that a follow-up if that should be done. This CL does improve the current state where when we never allow gc at all (unless stop is called). https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... File Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... Source/modules/mediastream/RTCPeerConnection.cpp:222: stop(); On 2014/06/15 13:43:57, haraken wrote: > > This is not allowed in oilpan, because in oilpan destructors are not allowed to > touch other on-heap objects (since there is no guarantee about the order in > which on-heap objects are destructed). Help me understand this, RTCDataChannel is ref counted and RTCPeerConnection holds a reference to it. Why is this not safe? > > keishi@: Do you have any idea on how to address this destruction issue? keishi@: Please comment on this. https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... File Source/modules/mediastream/RTCSessionDescriptionRequestImpl.cpp (right): https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... Source/modules/mediastream/RTCSessionDescriptionRequestImpl.cpp:66: bool mayFireCallback = m_requester ? m_requester->requestMayFireNonStatsCallback() : true; On 2014/06/15 13:43:57, haraken wrote: > > Just help me understand: Why is this 'true', not 'false'? I thought we don't > want to fire a callback when there is no requester. Yes, it should be false. Changed. https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... File Source/modules/mediastream/RTCSessionDescriptionRequestImpl.h (right): https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... Source/modules/mediastream/RTCSessionDescriptionRequestImpl.h:51: static PassRefPtr<RTCSessionDescriptionRequestImpl> create(ExecutionContext*, PassRefPtr<RTCPeerConnection>, PassOwnPtr<RTCSessionDescriptionCallback>, PassOwnPtr<RTCErrorCallback>); On 2014/06/15 13:43:57, haraken wrote: > > RTCPeerConnection is already on oilpan's heap, so would you use > PassRefPtrWillBeRawPtr? I don't know much about oilpan. You know that better than me, should I? It seems like RTCPeerConnection is on the oilpan's heap and RTCSessionDescriptionRequestImpl is not, but how to pass ptrs for this case I don't know. Changed it. https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... Source/modules/mediastream/RTCSessionDescriptionRequestImpl.h:61: RTCSessionDescriptionRequestImpl(ExecutionContext*, PassRefPtr<RTCPeerConnection>, PassOwnPtr<RTCSessionDescriptionCallback>, PassOwnPtr<RTCErrorCallback>); On 2014/06/15 13:43:57, haraken wrote: > > PassRefPtrWillBeRawPtr Done. https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... Source/modules/mediastream/RTCSessionDescriptionRequestImpl.h:68: RefPtr<RTCPeerConnection> m_requester; On 2014/06/15 13:43:57, haraken wrote: > > RefPtrWillBePersistent Done. https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... File Source/modules/mediastream/RTCStatsRequestImpl.h (right): https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... Source/modules/mediastream/RTCStatsRequestImpl.h:43: static PassRefPtr<RTCStatsRequestImpl> create(ExecutionContext*, PassRefPtr<RTCPeerConnection>, PassOwnPtr<RTCStatsCallback>, PassRefPtr<MediaStreamTrack>); On 2014/06/15 13:43:57, haraken wrote: > > PassRefPtrWillBeRawPtr Done. https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... Source/modules/mediastream/RTCStatsRequestImpl.h:56: RTCStatsRequestImpl(ExecutionContext*, PassRefPtr<RTCPeerConnection>, PassOwnPtr<RTCStatsCallback>, PassRefPtr<MediaStreamTrack>); On 2014/06/15 13:43:57, haraken wrote: > > PassRefPtrWillBeRawPtr Done. https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... Source/modules/mediastream/RTCStatsRequestImpl.h:63: RefPtr<RTCPeerConnection> m_requester; On 2014/06/15 13:43:57, haraken wrote: > > RefPtrWillBePersistent Done. https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... File Source/modules/mediastream/RTCVoidRequestImpl.h (right): https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... Source/modules/mediastream/RTCVoidRequestImpl.h:45: static PassRefPtr<RTCVoidRequestImpl> create(ExecutionContext*, PassRefPtr<RTCPeerConnection>, PassOwnPtr<VoidCallback>, PassOwnPtr<RTCErrorCallback>); On 2014/06/15 13:43:58, haraken wrote: > > PassRefPtrWillBeRawPtr Done. https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... Source/modules/mediastream/RTCVoidRequestImpl.h:56: RTCVoidRequestImpl(ExecutionContext*, PassRefPtr<RTCPeerConnection>, PassOwnPtr<VoidCallback>, PassOwnPtr<RTCErrorCallback>); On 2014/06/15 13:43:57, haraken wrote: > > PassRefPtrWillBeRawPtr Done. https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... Source/modules/mediastream/RTCVoidRequestImpl.h:63: RefPtr<RTCPeerConnection> m_requester; On 2014/06/15 13:43:57, haraken wrote: > > RefPtrWillBePersistent Done.
https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... File Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... Source/modules/mediastream/RTCPeerConnection.cpp:222: stop(); On 2014/06/16 07:48:59, Henrik Grunell wrote: > On 2014/06/15 13:43:57, haraken wrote: > > > > This is not allowed in oilpan, because in oilpan destructors are not allowed > to > > touch other on-heap objects (since there is no guarantee about the order in > > which on-heap objects are destructed). > > Help me understand this, RTCDataChannel is ref counted and RTCPeerConnection > holds a reference to it. Why is this not safe? > > > > > keishi@: Do you have any idea on how to address this destruction issue? > > keishi@: Please comment on this. I think we might need to use weak processing to call stop() before destruction. The problem seems to be that the RTCDataChannel can outlive RTCPeerConnection by holding a reference to it in JS and that could fire events after RTCPeerConnection has been destroyed. We could have a RTCDataChannelObserver to call RTCDataChannel::stop() before destruction. (like DatabaseSync::TransactionObserver) Or maybe RTCDataChannel could keep RTCPeerConnection as a weak member, check it before dispatching events, and call RTCDataChannel::stop on itself when the member becomes null. This might be better if it works.
https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... File Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... Source/modules/mediastream/RTCPeerConnection.cpp:593: ASSERT(!m_closed); When I run your patch locally I hit this assertion in fast/mediastream/RTCPeerConnection-state.html
https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... File Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... Source/modules/mediastream/RTCPeerConnection.cpp:222: stop(); On 2014/06/16 08:04:01, keishi wrote: > On 2014/06/16 07:48:59, Henrik Grunell wrote: > > On 2014/06/15 13:43:57, haraken wrote: > > > > > > This is not allowed in oilpan, because in oilpan destructors are not allowed > > to > > > touch other on-heap objects (since there is no guarantee about the order in > > > which on-heap objects are destructed). > > > > Help me understand this, RTCDataChannel is ref counted and RTCPeerConnection > > holds a reference to it. Why is this not safe? > > > > > > > > keishi@: Do you have any idea on how to address this destruction issue? > > > > keishi@: Please comment on this. > > I think we might need to use weak processing to call stop() before destruction. > The problem seems to be that the RTCDataChannel can outlive RTCPeerConnection by > holding a reference to it in JS and that could fire events after > RTCPeerConnection has been destroyed. > > We could have a RTCDataChannelObserver to call RTCDataChannel::stop() before > destruction. (like DatabaseSync::TransactionObserver) > Or maybe RTCDataChannel could keep RTCPeerConnection as a weak member, check it > before dispatching events, and call RTCDataChannel::stop on itself when the > member becomes null. This might be better if it works. Thanks for the input. Actually, the right thing could be to stop the DataChannels in close(). I'll look into that, then we don't have to deal with this. https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... Source/modules/mediastream/RTCPeerConnection.cpp:593: ASSERT(!m_closed); On 2014/06/16 08:06:35, keishi wrote: > When I run your patch locally I hit this assertion in > fast/mediastream/RTCPeerConnection-state.html Ah, right, it's a problem in the mock in Chromium. I'll land the fix asap. https://codereview.chromium.org/331143003/
Den 16 jun 2014 10:34 skrev <grunell@chromium.org>: > > > https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... > File Source/modules/mediastream/RTCPeerConnection.cpp (right): > > https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... > Source/modules/mediastream/RTCPeerConnection.cpp:222: stop(); > On 2014/06/16 08:04:01, keishi wrote: >> >> On 2014/06/16 07:48:59, Henrik Grunell wrote: >> > On 2014/06/15 13:43:57, haraken wrote: >> > > >> > > This is not allowed in oilpan, because in oilpan destructors are > > not allowed >> >> > to >> > > touch other on-heap objects (since there is no guarantee about the > > order in >> >> > > which on-heap objects are destructed). >> > >> > Help me understand this, RTCDataChannel is ref counted and > > RTCPeerConnection >> >> > holds a reference to it. Why is this not safe? >> > >> > > >> > > keishi@: Do you have any idea on how to address this destruction > > issue? >> >> > >> > keishi@: Please comment on this. > > >> I think we might need to use weak processing to call stop() before > > destruction. >> >> The problem seems to be that the RTCDataChannel can outlive > > RTCPeerConnection by >> >> holding a reference to it in JS and that could fire events after >> RTCPeerConnection has been destroyed. > > >> We could have a RTCDataChannelObserver to call RTCDataChannel::stop() > > before >> >> destruction. (like DatabaseSync::TransactionObserver) >> Or maybe RTCDataChannel could keep RTCPeerConnection as a weak member, > > check it >> >> before dispatching events, and call RTCDataChannel::stop on itself > > when the >> >> member becomes null. This might be better if it works. > > > Thanks for the input. Actually, the right thing could be to stop the > DataChannels in close(). I'll look into that, then we don't have to deal > with this. So, can't call stop on data channels in close. Help me understand why we can't touch it in the dtor. AFAIK the dcs are referenced from PC. So they can't be deleted before the PC, isn't that correct? I'm probably lacking oil pan knowledge. > > > https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... > Source/modules/mediastream/RTCPeerConnection.cpp:593: ASSERT(!m_closed); > On 2014/06/16 08:06:35, keishi wrote: >> >> When I run your patch locally I hit this assertion in >> fast/mediastream/RTCPeerConnection-state.html > > > Ah, right, it's a problem in the mock in Chromium. I'll land the fix > asap. https://codereview.chromium.org/331143003/ This has landed. > > https://codereview.chromium.org/329093002/ To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On 2014/06/16 21:21:34, blink-reviews_chromium.org wrote: > Den 16 jun 2014 10:34 skrev <mailto:grunell@chromium.org>: > > > > > > > https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... > > File Source/modules/mediastream/RTCPeerConnection.cpp (right): > > > > > https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... > > Source/modules/mediastream/RTCPeerConnection.cpp:222: stop(); > > On 2014/06/16 08:04:01, keishi wrote: > >> > >> On 2014/06/16 07:48:59, Henrik Grunell wrote: > >> > On 2014/06/15 13:43:57, haraken wrote: > >> > > > >> > > This is not allowed in oilpan, because in oilpan destructors are > > > > not allowed > >> > >> > to > >> > > touch other on-heap objects (since there is no guarantee about the > > > > order in > >> > >> > > which on-heap objects are destructed). > >> > > >> > Help me understand this, RTCDataChannel is ref counted and > > > > RTCPeerConnection > >> > >> > holds a reference to it. Why is this not safe? > >> > > >> > > > >> > > keishi@: Do you have any idea on how to address this destruction > > > > issue? > >> > >> > > >> > keishi@: Please comment on this. > > > > > >> I think we might need to use weak processing to call stop() before > > > > destruction. > >> > >> The problem seems to be that the RTCDataChannel can outlive > > > > RTCPeerConnection by > >> > >> holding a reference to it in JS and that could fire events after > >> RTCPeerConnection has been destroyed. > > > > > >> We could have a RTCDataChannelObserver to call RTCDataChannel::stop() > > > > before > >> > >> destruction. (like DatabaseSync::TransactionObserver) > >> Or maybe RTCDataChannel could keep RTCPeerConnection as a weak member, > > > > check it > >> > >> before dispatching events, and call RTCDataChannel::stop on itself > > > > when the > >> > >> member becomes null. This might be better if it works. > > > > > > Thanks for the input. Actually, the right thing could be to stop the > > DataChannels in close(). I'll look into that, then we don't have to deal > > with this. > > So, can't call stop on data channels in close. Help me understand why we > can't touch it in the dtor. AFAIK the dcs are referenced from PC. So they > can't be deleted before the PC, isn't that correct? I'm probably lacking > oil pan knowledge. GC marks all the objects that can be reached from the root set, and the rest gets destroyed in no particular order. So if RTCDataChannel and RTCPeerConnection both get destructed in the gc cycle, when RTCPeerConnection's dtor gets called, RTCDataChannel could already have been destroyed.
On 2014/06/16 21:56:04, keishi wrote: > On 2014/06/16 21:21:34, http://blink-reviews_chromium.org wrote: > > Den 16 jun 2014 10:34 skrev <mailto:grunell@chromium.org>: > > > > > > > > > > > > https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... > > > File Source/modules/mediastream/RTCPeerConnection.cpp (right): > > > > > > > > > https://codereview.chromium.org/329093002/diff/120001/Source/modules/mediastr... > > > Source/modules/mediastream/RTCPeerConnection.cpp:222: stop(); > > > On 2014/06/16 08:04:01, keishi wrote: > > >> > > >> On 2014/06/16 07:48:59, Henrik Grunell wrote: > > >> > On 2014/06/15 13:43:57, haraken wrote: > > >> > > > > >> > > This is not allowed in oilpan, because in oilpan destructors are > > > > > > not allowed > > >> > > >> > to > > >> > > touch other on-heap objects (since there is no guarantee about the > > > > > > order in > > >> > > >> > > which on-heap objects are destructed). > > >> > > > >> > Help me understand this, RTCDataChannel is ref counted and > > > > > > RTCPeerConnection > > >> > > >> > holds a reference to it. Why is this not safe? > > >> > > > >> > > > > >> > > keishi@: Do you have any idea on how to address this destruction > > > > > > issue? > > >> > > >> > > > >> > keishi@: Please comment on this. > > > > > > > > >> I think we might need to use weak processing to call stop() before > > > > > > destruction. > > >> > > >> The problem seems to be that the RTCDataChannel can outlive > > > > > > RTCPeerConnection by > > >> > > >> holding a reference to it in JS and that could fire events after > > >> RTCPeerConnection has been destroyed. > > > > > > > > >> We could have a RTCDataChannelObserver to call RTCDataChannel::stop() > > > > > > before > > >> > > >> destruction. (like DatabaseSync::TransactionObserver) > > >> Or maybe RTCDataChannel could keep RTCPeerConnection as a weak member, > > > > > > check it > > >> > > >> before dispatching events, and call RTCDataChannel::stop on itself > > > > > > when the > > >> > > >> member becomes null. This might be better if it works. > > > > > > > > > Thanks for the input. Actually, the right thing could be to stop the > > > DataChannels in close(). I'll look into that, then we don't have to deal > > > with this. > > > > So, can't call stop on data channels in close. Help me understand why we > > can't touch it in the dtor. AFAIK the dcs are referenced from PC. So they > > can't be deleted before the PC, isn't that correct? I'm probably lacking > > oil pan knowledge. > > GC marks all the objects that can be reached from the root set, and the rest > gets destroyed in no particular order. So if RTCDataChannel and > RTCPeerConnection both get destructed in the gc cycle, when RTCPeerConnection's > dtor gets called, RTCDataChannel could already have been destroyed. s/the gc cycle/the same gc cycle/
Data channel now has a weak ptr to the peer connection that created it. Stops itself if creator goes away.
https://codereview.chromium.org/329093002/diff/150001/Source/modules/mediastr... File Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/329093002/diff/150001/Source/modules/mediastr... Source/modules/mediastream/RTCPeerConnection.cpp:223: stop(); We should remove this now.
https://codereview.chromium.org/329093002/diff/150001/Source/modules/mediastr... File Source/modules/mediastream/RTCDataChannel.h (right): https://codereview.chromium.org/329093002/diff/150001/Source/modules/mediastr... Source/modules/mediastream/RTCDataChannel.h:124: WeakPtrWillBeWeakMember<RTCPeerConnection> m_creator; We need to trace members. You need to add this line to RTCPeerConnection::trace visitor->trace(m_creator); https://codereview.chromium.org/329093002/diff/150001/Source/modules/mediastr... File Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/329093002/diff/150001/Source/modules/mediastr... Source/modules/mediastream/RTCPeerConnection.cpp:515: RefPtrWillBeRawPtr<RTCDataChannel> channel = RTCDataChannel::create(executionContext(), m_weakPtrFactory.createWeakPtr(), m_peerHandler.get(), label, init, exceptionState); We need to pass m_weakPtrFactory.createWeakPtr() for !ENABLE(OILPAN) and this for ENABLE(OILPAN). https://codereview.chromium.org/329093002/diff/150001/Source/modules/mediastr... Source/modules/mediastream/RTCPeerConnection.cpp:649: RefPtrWillBeRawPtr<RTCDataChannel> channel = RTCDataChannel::create(executionContext(), m_weakPtrFactory.createWeakPtr(), adoptPtr(handler)); Ditto. https://codereview.chromium.org/329093002/diff/150001/Source/modules/mediastr... File Source/modules/mediastream/RTCPeerConnection.h (right): https://codereview.chromium.org/329093002/diff/150001/Source/modules/mediastr... Source/modules/mediastream/RTCPeerConnection.h:181: WeakPtrFactory<RTCPeerConnection> m_weakPtrFactory; Could you wrap this in !ENABLE(OILPAN)?
https://codereview.chromium.org/329093002/diff/150001/Source/modules/mediastr... File Source/modules/mediastream/RTCDataChannel.h (right): https://codereview.chromium.org/329093002/diff/150001/Source/modules/mediastr... Source/modules/mediastream/RTCDataChannel.h:124: WeakPtrWillBeWeakMember<RTCPeerConnection> m_creator; On 2014/06/17 07:58:21, keishi wrote: > We need to trace members. You need to add this line to RTCPeerConnection::trace > visitor->trace(m_creator); Done. https://codereview.chromium.org/329093002/diff/150001/Source/modules/mediastr... File Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/329093002/diff/150001/Source/modules/mediastr... Source/modules/mediastream/RTCPeerConnection.cpp:223: stop(); On 2014/06/17 07:53:10, keishi wrote: > We should remove this now. Done. https://codereview.chromium.org/329093002/diff/150001/Source/modules/mediastr... Source/modules/mediastream/RTCPeerConnection.cpp:515: RefPtrWillBeRawPtr<RTCDataChannel> channel = RTCDataChannel::create(executionContext(), m_weakPtrFactory.createWeakPtr(), m_peerHandler.get(), label, init, exceptionState); On 2014/06/17 07:58:21, keishi wrote: > We need to pass m_weakPtrFactory.createWeakPtr() for !ENABLE(OILPAN) and this > for ENABLE(OILPAN). Done. https://codereview.chromium.org/329093002/diff/150001/Source/modules/mediastr... Source/modules/mediastream/RTCPeerConnection.cpp:649: RefPtrWillBeRawPtr<RTCDataChannel> channel = RTCDataChannel::create(executionContext(), m_weakPtrFactory.createWeakPtr(), adoptPtr(handler)); On 2014/06/17 07:58:21, keishi wrote: > Ditto. Done. https://codereview.chromium.org/329093002/diff/150001/Source/modules/mediastr... File Source/modules/mediastream/RTCPeerConnection.h (right): https://codereview.chromium.org/329093002/diff/150001/Source/modules/mediastr... Source/modules/mediastream/RTCPeerConnection.h:181: WeakPtrFactory<RTCPeerConnection> m_weakPtrFactory; On 2014/06/17 07:58:21, keishi wrote: > Could you wrap this in !ENABLE(OILPAN)? Done.
https://codereview.chromium.org/329093002/diff/170001/LayoutTests/fast/medias... File LayoutTests/fast/mediastream/RTCPeerConnection-lifetime.html (right): https://codereview.chromium.org/329093002/diff/170001/LayoutTests/fast/medias... LayoutTests/fast/mediastream/RTCPeerConnection-lifetime.html:40: gc(); I think its better to use asyncGC(). It ensures that precise garbage collection is run (instead of the conservative stack scanning). https://codereview.chromium.org/329093002/diff/170001/Source/modules/mediastr... File Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/329093002/diff/170001/Source/modules/mediastr... Source/modules/mediastream/RTCPeerConnection.cpp:184: , m_weakPtrFactory(this) nit: Wrap in #if ENABLE(OILPAN)
I'm not sure this will work. Is the following possible: PeerConnection and DataChannels are all dead at the same time and there are no events pending for the DataChannels. In that case stop will never be called on the DataChannels. Is that scenario possible and is it OK to not call stop in that case. It looks like there is a notification of a chromium side object that will not happen in that case (m_handler->setClient(0))? Maybe the above can be fixed by having the DataChannel destructor call m_handler->setClient(0) if m_stopped is false? I think weak processing is the right idea for Oilpan here. However, having to explicitly check in all events seems unfortunate. I think we should use weak processing only for Oilpan and deal with this with a custom weak callback? In other words, leave the call to stop in the destructor of PeerConnection under a #if !ENABLE(OILPAN). Add the WeakMember to DataChannel only for oilpan and register a custom weak callback that will call stop on the DataChannel when the DataChannel is still alive and the PeerConnection is dead (plus add the m_handler->setClient(0) call to the destructor of DataChannel for the case where it all dies in the same round of GC). That way we can avoid the checks in all the events. Henrik, I think it would be fine if you go back to the version that just has the stop call in the destructor for this changelist. Then Keishi or I can make a follow-up changelist to fix the oilpan version of this (and CC you of course).
On 2014/06/17 08:26:01, Mads Ager (chromium) wrote: > I'm not sure this will work. Is the following possible: > > PeerConnection and DataChannels are all dead at the same time and there are no > events pending for the DataChannels. In that case stop will never be called on > the DataChannels. Is that scenario possible and is it OK to not call stop in > that case. It looks like there is a notification of a chromium side object that > will not happen in that case (m_handler->setClient(0))? > > Maybe the above can be fixed by having the DataChannel destructor call > m_handler->setClient(0) if m_stopped is false? m_handler is an OwnPtr in DataChannel. Doesn't that mean that it is deleted when DataChannel is? > I think weak processing is the right idea for Oilpan here. However, having to > explicitly check in all events seems unfortunate. I think we should use weak > processing only for Oilpan and deal with this with a custom weak callback? In > other words, leave the call to stop in the destructor of PeerConnection under a > #if !ENABLE(OILPAN). Add the WeakMember to DataChannel only for oilpan and > register a custom weak callback that will call stop on the DataChannel when the > DataChannel is still alive and the PeerConnection is dead (plus add the > m_handler->setClient(0) call to the destructor of DataChannel for the case where > it all dies in the same round of GC). That way we can avoid the checks in all > the events. > > Henrik, I think it would be fine if you go back to the version that just has the > stop call in the destructor for this changelist. Then Keishi or I can make a > follow-up changelist to fix the oilpan version of this (and CC you of course). I'm fine with that. keishi@?
On 2014/06/17 08:40:06, Henrik Grunell wrote: > On 2014/06/17 08:26:01, Mads Ager (chromium) wrote: > > I'm not sure this will work. Is the following possible: > > > > PeerConnection and DataChannels are all dead at the same time and there are no > > events pending for the DataChannels. In that case stop will never be called on > > the DataChannels. Is that scenario possible and is it OK to not call stop in > > that case. It looks like there is a notification of a chromium side object > that > > will not happen in that case (m_handler->setClient(0))? > > > > Maybe the above can be fixed by having the DataChannel destructor call > > m_handler->setClient(0) if m_stopped is false? > > m_handler is an OwnPtr in DataChannel. Doesn't that mean that it is deleted when > DataChannel is? It means that it is deleted automatically at the end of the destructor. It is safe in user destructor code to touch m_handler since the OwnPtr will only be destructed after the user destructor code as been executed.
On 2014/06/17 08:47:01, Mads Ager (chromium) wrote: > On 2014/06/17 08:40:06, Henrik Grunell wrote: > > On 2014/06/17 08:26:01, Mads Ager (chromium) wrote: > > > I'm not sure this will work. Is the following possible: > > > > > > PeerConnection and DataChannels are all dead at the same time and there are > no > > > events pending for the DataChannels. In that case stop will never be called > on > > > the DataChannels. Is that scenario possible and is it OK to not call stop in > > > that case. It looks like there is a notification of a chromium side object > > that > > > will not happen in that case (m_handler->setClient(0))? > > > > > > Maybe the above can be fixed by having the DataChannel destructor call > > > m_handler->setClient(0) if m_stopped is false? > > > > m_handler is an OwnPtr in DataChannel. Doesn't that mean that it is deleted > when > > DataChannel is? > > It means that it is deleted automatically at the end of the destructor. It is > safe in user destructor code to touch m_handler since the OwnPtr will only be > destructed after the user destructor code as been executed. I guess its easier to use weak processing. I'm sorry but could you revert to your previous code. I will fix the oilpan side.
* Reverted to call stop() in the destructor. No ptr to PeerConnection in DataChannel. * Code review fix. https://codereview.chromium.org/329093002/diff/170001/LayoutTests/fast/medias... File LayoutTests/fast/mediastream/RTCPeerConnection-lifetime.html (right): https://codereview.chromium.org/329093002/diff/170001/LayoutTests/fast/medias... LayoutTests/fast/mediastream/RTCPeerConnection-lifetime.html:40: gc(); On 2014/06/17 08:16:42, keishi wrote: > I think its better to use asyncGC(). It ensures that precise garbage collection > is run (instead of the conservative stack scanning). Done.
lgtm haraken@, could you do the OWNER review https://codereview.chromium.org/329093002/diff/230001/LayoutTests/fast/medias... File LayoutTests/fast/mediastream/RTCPeerConnection-lifetime.html (right): https://codereview.chromium.org/329093002/diff/230001/LayoutTests/fast/medias... LayoutTests/fast/mediastream/RTCPeerConnection-lifetime.html:55: asyncGC(function() {}); nit: if we aren't using the callback, asyncGC() and gc() are the same. https://codereview.chromium.org/329093002/diff/230001/LayoutTests/fast/medias... LayoutTests/fast/mediastream/RTCPeerConnection-lifetime.html:60: window.successfullyParsed = true; nit: successfullyParsed is handled in js-test.js now.
LGTM. https://codereview.chromium.org/329093002/diff/230001/LayoutTests/fast/medias... File LayoutTests/fast/mediastream/RTCPeerConnection-lifetime.html (right): https://codereview.chromium.org/329093002/diff/230001/LayoutTests/fast/medias... LayoutTests/fast/mediastream/RTCPeerConnection-lifetime.html:55: asyncGC(function() {}); On 2014/06/17 11:12:04, keishi wrote: > nit: if we aren't using the callback, asyncGC() and gc() are the same. I think you can just remove this asyncGC(). https://codereview.chromium.org/329093002/diff/230001/Source/modules/mediastr... File Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/329093002/diff/230001/Source/modules/mediastr... Source/modules/mediastream/RTCPeerConnection.cpp:222: stop(); Would you add a "FIXME: Oilpan: ..." on this? Even if it's going to be fixed in the near future, we want to keep trace of it. https://codereview.chromium.org/329093002/diff/230001/Source/modules/mediastr... File Source/modules/mediastream/RTCPeerConnection.h (right): https://codereview.chromium.org/329093002/diff/230001/Source/modules/mediastr... Source/modules/mediastream/RTCPeerConnection.h:109: bool requestMayFireStatsCallback() { return !m_stopped; } Nit: I'd rename these methods to shouldFireDefaultCallbacks & shouldFireGetStatsCallback. https://codereview.chromium.org/329093002/diff/230001/Source/modules/mediastr... Source/modules/mediastream/RTCPeerConnection.h:147: // checks if it should fire the callback before doing so. Nit: This comment sounds a bit redundant. Can we just mention that the RTCPeerConnection object is kept alive as long as it is not closed nor stopped? https://codereview.chromium.org/329093002/diff/230001/Source/modules/mediastr... File Source/modules/mediastream/RTCSessionDescriptionRequestImpl.cpp (right): https://codereview.chromium.org/329093002/diff/230001/Source/modules/mediastr... Source/modules/mediastream/RTCSessionDescriptionRequestImpl.cpp:66: bool mayFireCallback = m_requester ? m_requester->requestMayFireNonStatsCallback() : false; Nit: mayFireCallback => shouldFireCallback ? https://codereview.chromium.org/329093002/diff/230001/Source/modules/mediastr... File Source/modules/mediastream/RTCStatsRequestImpl.cpp (right): https://codereview.chromium.org/329093002/diff/230001/Source/modules/mediastr... Source/modules/mediastream/RTCStatsRequestImpl.cpp:73: return; I guess you need to call clear() in this case. So the code should be: bool mayFireCallback = m_requester ? m_requester>requestMayFireNonStatsCallback() : false; if (mayFireCallback && m_successCallback) m_successCallback->... clear();
The CQ bit was checked by grunell@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/grunell@chromium.org/329093002/250001
The CQ bit was unchecked by commit-bot@chromium.org
A disapproval has been posted by following reviewers: tommi@chromium.org. Please make sure to get positive review before another CQ attempt.
The CQ bit was checked by grunell@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/grunell@chromium.org/329093002/250001
The CQ bit was unchecked by commit-bot@chromium.org
A disapproval has been posted by following reviewers: tommi@chromium.org. Please make sure to get positive review before another CQ attempt.
https://codereview.chromium.org/329093002/diff/230001/LayoutTests/fast/medias... File LayoutTests/fast/mediastream/RTCPeerConnection-lifetime.html (right): https://codereview.chromium.org/329093002/diff/230001/LayoutTests/fast/medias... LayoutTests/fast/mediastream/RTCPeerConnection-lifetime.html:55: asyncGC(function() {}); On 2014/06/17 11:43:45, haraken wrote: > On 2014/06/17 11:12:04, keishi wrote: > > nit: if we aren't using the callback, asyncGC() and gc() are the same. > > I think you can just remove this asyncGC(). Changed to gc(). I think the point is (this test was already there) to force gc and make sure the data channel still runs. https://codereview.chromium.org/329093002/diff/230001/LayoutTests/fast/medias... LayoutTests/fast/mediastream/RTCPeerConnection-lifetime.html:60: window.successfullyParsed = true; On 2014/06/17 11:12:04, keishi wrote: > nit: successfullyParsed is handled in js-test.js now. Done. https://codereview.chromium.org/329093002/diff/230001/Source/modules/mediastr... File Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/329093002/diff/230001/Source/modules/mediastr... Source/modules/mediastream/RTCPeerConnection.cpp:222: stop(); On 2014/06/17 11:43:45, haraken wrote: > > Would you add a "FIXME: Oilpan: ..." on this? Even if it's going to be fixed in > the near future, we want to keep trace of it. Done. https://codereview.chromium.org/329093002/diff/230001/Source/modules/mediastr... File Source/modules/mediastream/RTCPeerConnection.h (right): https://codereview.chromium.org/329093002/diff/230001/Source/modules/mediastr... Source/modules/mediastream/RTCPeerConnection.h:109: bool requestMayFireStatsCallback() { return !m_stopped; } On 2014/06/17 11:43:45, haraken wrote: > > Nit: I'd rename these methods to shouldFireDefaultCallbacks & > shouldFireGetStatsCallback. Done. https://codereview.chromium.org/329093002/diff/230001/Source/modules/mediastr... Source/modules/mediastream/RTCPeerConnection.h:147: // checks if it should fire the callback before doing so. On 2014/06/17 11:43:45, haraken wrote: > > Nit: This comment sounds a bit redundant. Can we just mention that the > RTCPeerConnection object is kept alive as long as it is not closed nor stopped? Done. https://codereview.chromium.org/329093002/diff/230001/Source/modules/mediastr... File Source/modules/mediastream/RTCStatsRequestImpl.cpp (right): https://codereview.chromium.org/329093002/diff/230001/Source/modules/mediastr... Source/modules/mediastream/RTCStatsRequestImpl.cpp:73: return; On 2014/06/17 11:43:45, haraken wrote: > > I guess you need to call clear() in this case. So the code should be: > > bool mayFireCallback = m_requester ? > m_requester>requestMayFireNonStatsCallback() : false; > if (mayFireCallback && m_successCallback) > m_successCallback->... > clear(); Yep, you're right. Done.
The CQ bit was checked by grunell@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/grunell@chromium.org/329093002/250001
The CQ bit was unchecked by commit-bot@chromium.org
A disapproval has been posted by following reviewers: tommi@chromium.org. Please make sure to get positive review before another CQ attempt.
lgtm
The CQ bit was checked by grunell@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/grunell@chromium.org/329093002/250001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was checked by grunell@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/grunell@chromium.org/329093002/250001
Message was sent while issue was closed.
Change committed as 176349 |
