Add PeerConnection IsClosed check.

Add PeerConnection IsClosed check.

Add IsClosed check when excuting some functions so that they can return early if the PeerConnection is closed.
The observer will not be called after the PeerConnection is closed.

BUG=webrtc:5861
Committed: https://crrev.com/29ff8446c0c89114729f6b8cfe49063c899b5764
Cr-Commit-Position: refs/heads/master@{#13544}

[Zhi Huang, 2016-05-11 21:46:59]

zhihuang@webrtc.org changed reviewers:
+ deadbeef@webrtc.org, pthatcher@webrtc.org

[Taylor Brandstetter, 2016-05-11 22:18:37]

The check should also probably be added in On(Audio|Video)Track(Added|Removed).
These methods can be called if a MediaStream added to the PeerConnection is
modified.

https://codereview.webrtc.org/1975453002/diff/1/webrtc/api/peerconnection.cc
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/1975453002/diff/1/webrtc/api/peerconnection.cc#...
webrtc/api/peerconnection.cc:1310: }
This and CreateVideoReceiver are internal methods, only called from
SetRemoteDescription. So I think the check should occur earlier, in
SetRemoteDescription (with another for SetLocalDescription).

[Zhi Huang, 2016-05-12 01:25:49]

https://codereview.webrtc.org/1975453002/diff/1/webrtc/api/peerconnection.cc
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/1975453002/diff/1/webrtc/api/peerconnection.cc#...
webrtc/api/peerconnection.cc:1310: }
On 2016/05/11 22:18:36, Taylor Brandstetter wrote:
> This and CreateVideoReceiver are internal methods, only called from
> SetRemoteDescription. So I think the check should occur earlier, in
> SetRemoteDescription (with another for SetLocalDescription).

Done.

[pthatcher1, 2016-05-12 05:44:13]

https://codereview.webrtc.org/1975453002/diff/20001/webrtc/api/peerconnection...
File webrtc/api/peerconnectioninterface_unittest.cc (right):

https://codereview.webrtc.org/1975453002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface_unittest.cc:707: }
Shouldn't we add some unit tests to verify the behavior of the various methods
when a PC is closed?

[Zhi Huang, 2016-05-13 01:45:05]

On 2016/05/12 05:44:13, pthatcher1 wrote:
>
https://codereview.webrtc.org/1975453002/diff/20001/webrtc/api/peerconnection...
> File webrtc/api/peerconnectioninterface_unittest.cc (right):
> 
>
https://codereview.webrtc.org/1975453002/diff/20001/webrtc/api/peerconnection...
> webrtc/api/peerconnectioninterface_unittest.cc:707: }
> Shouldn't we add some unit tests to verify the behavior of the various methods
> when a PC is closed?

There is a test called CloseAndTestMethod which tests some functions after the
peerconnection is closed.
I think it is not very straightforward to test the case that some events are
triggered after the peerconnection is closed.
There are some options on it.

1. Directly trigger the event like OnIceCandidateRemoved
OnIceCandidateReceivingChange.
These methods are private and I am not sure if we can use the FRIEND_TEST to
call them in the tests.
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...

2.Inject a MockTransportController object into the peerconnection by creating a
setter function to trigger the events.

3.Modified the FakePortAllocator and FakePortAllcatorSession to trigger the
event somehow.(I am not very clear how to trigger all the event so far. May take
more time.)
If using this method, we may to need to create an additional thread in the unit
test and there might be race between threads to reproduce problem.

Please correct me if I am wrong with something.

[Zhi Huang, 2016-05-13 01:45:07]

On 2016/05/12 05:44:13, pthatcher1 wrote:
>
https://codereview.webrtc.org/1975453002/diff/20001/webrtc/api/peerconnection...
> File webrtc/api/peerconnectioninterface_unittest.cc (right):
> 
>
https://codereview.webrtc.org/1975453002/diff/20001/webrtc/api/peerconnection...
> webrtc/api/peerconnectioninterface_unittest.cc:707: }
> Shouldn't we add some unit tests to verify the behavior of the various methods
> when a PC is closed?

There is a test called CloseAndTestMethod which tests some functions after the
peerconnection is closed.
I think it is not very straightforward to test the case that some events are
triggered after the peerconnection is closed.
There are some options on it.

1. Directly trigger the event like OnIceCandidateRemoved
OnIceCandidateReceivingChange.
These methods are private and I am not sure if we can use the FRIEND_TEST to
call them in the tests.
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...

2.Inject a MockTransportController object into the peerconnection by creating a
setter function to trigger the events.

3.Modified the FakePortAllocator and FakePortAllcatorSession to trigger the
event somehow.(I am not very clear how to trigger all the event so far. May take
more time.)
If using this method, we may to need to create an additional thread in the unit
test and there might be race between threads to reproduce problem.

Please correct me if I am wrong with something.

[pthatcher1, 2016-05-25 20:51:17]

On 2016/05/13 01:45:07, Zhi Huang wrote:
> On 2016/05/12 05:44:13, pthatcher1 wrote:
> >
>
https://codereview.webrtc.org/1975453002/diff/20001/webrtc/api/peerconnection...
> > File webrtc/api/peerconnectioninterface_unittest.cc (right):
> > 
> >
>
https://codereview.webrtc.org/1975453002/diff/20001/webrtc/api/peerconnection...
> > webrtc/api/peerconnectioninterface_unittest.cc:707: }
> > Shouldn't we add some unit tests to verify the behavior of the various
methods
> > when a PC is closed?
> 
> There is a test called CloseAndTestMethod which tests some functions after the
> peerconnection is closed.
> I think it is not very straightforward to test the case that some events are
> triggered after the peerconnection is closed.
> There are some options on it.
> 
> 1. Directly trigger the event like OnIceCandidateRemoved
> OnIceCandidateReceivingChange.

> These methods are private and I am not sure if we can use the FRIEND_TEST to
> call them in the tests.
>
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...

OnIceConnectionReceivingChange and OnIceCandidatesRemoved are both public.

> 
> 2.Inject a MockTransportController object into the peerconnection by creating
a
> setter function to trigger the events.
> 
> 3.Modified the FakePortAllocator and FakePortAllcatorSession to trigger the
> event somehow.(I am not very clear how to trigger all the event so far. May
take
> more time.)
> If using this method, we may to need to create an additional thread in the
unit
> test and there might be race between threads to reproduce problem.
> 
> Please correct me if I am wrong with something.

[Taylor Brandstetter, 2016-05-25 22:07:12]

On 2016/05/25 20:51:17, pthatcher1 wrote:
> On 2016/05/13 01:45:07, Zhi Huang wrote:
> > On 2016/05/12 05:44:13, pthatcher1 wrote:
> > >
> >
>
https://codereview.webrtc.org/1975453002/diff/20001/webrtc/api/peerconnection...
> > > File webrtc/api/peerconnectioninterface_unittest.cc (right):
> > > 
> > >
> >
>
https://codereview.webrtc.org/1975453002/diff/20001/webrtc/api/peerconnection...
> > > webrtc/api/peerconnectioninterface_unittest.cc:707: }
> > > Shouldn't we add some unit tests to verify the behavior of the various
> methods
> > > when a PC is closed?
> > 
> > There is a test called CloseAndTestMethod which tests some functions after
the
> > peerconnection is closed.
> > I think it is not very straightforward to test the case that some events are
> > triggered after the peerconnection is closed.
> > There are some options on it.
> > 
> > 1. Directly trigger the event like OnIceCandidateRemoved
> > OnIceCandidateReceivingChange.
> 
> > These methods are private and I am not sure if we can use the FRIEND_TEST to
> > call them in the tests.
> >
>
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...
> 
> OnIceConnectionReceivingChange and OnIceCandidatesRemoved are both public.
> 

I think Zhi means PeerConnection::OnIceConnectionReceivingChange (the signal
forwarded up from P2PTransportChannels, basically).

The only "right" ways to test this (in my opinion) are:

1. Add the ability to inject a fake WebRtcSession into PeerConnection (later, a
fake TransportController), and make the fake fire the necessary signals. Not
ideal because something that used to be an implementation detail
(TransportController) is now part of the API.

2. Test a PeerConnection against a complete fake ICE implementation such that we
can control when the fake sends STUN pings in order to trigger the bug. Not
ideal because it would require a lot of effort to maintain.

So basically: do we treat the PeerConnection as a giant black box, or treat it
as something coordinating various pieces and test its interactions between those
pieces? This exact situation came up for me recently as well, and I've been
meaning to ask Peter what his opinion is.

[pthatcher1, 2016-05-27 17:21:43]

On 2016/05/25 22:07:12, Taylor Brandstetter wrote:
> On 2016/05/25 20:51:17, pthatcher1 wrote:
> > On 2016/05/13 01:45:07, Zhi Huang wrote:
> > > On 2016/05/12 05:44:13, pthatcher1 wrote:
> > > >
> > >
> >
>
https://codereview.webrtc.org/1975453002/diff/20001/webrtc/api/peerconnection...
> > > > File webrtc/api/peerconnectioninterface_unittest.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.webrtc.org/1975453002/diff/20001/webrtc/api/peerconnection...
> > > > webrtc/api/peerconnectioninterface_unittest.cc:707: }
> > > > Shouldn't we add some unit tests to verify the behavior of the various
> > methods
> > > > when a PC is closed?
> > > 
> > > There is a test called CloseAndTestMethod which tests some functions after
> the
> > > peerconnection is closed.
> > > I think it is not very straightforward to test the case that some events
are
> > > triggered after the peerconnection is closed.
> > > There are some options on it.
> > > 
> > > 1. Directly trigger the event like OnIceCandidateRemoved
> > > OnIceCandidateReceivingChange.
> > 
> > > These methods are private and I am not sure if we can use the FRIEND_TEST
to
> > > call them in the tests.
> > >
> >
>
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...
> > 
> > OnIceConnectionReceivingChange and OnIceCandidatesRemoved are both public.
> > 
> 
> I think Zhi means PeerConnection::OnIceConnectionReceivingChange (the signal
> forwarded up from P2PTransportChannels, basically).

Ah, you're right.  PeerConnection::OnIceConnectionReceivingChange is private.

> 
> The only "right" ways to test this (in my opinion) are:
> 
> 1. Add the ability to inject a fake WebRtcSession into PeerConnection (later,
a
> fake TransportController), and make the fake fire the necessary signals. Not
> ideal because something that used to be an implementation detail
> (TransportController) is now part of the API.

A FakeTransportController makes sense, but a FakeWebRtcSession is a waste of
time since we plan to merge WebRtcSession and PeerConnection.

> 
> 2. Test a PeerConnection against a complete fake ICE implementation such that
we
> can control when the fake sends STUN pings in order to trigger the bug. Not
> ideal because it would require a lot of effort to maintain.

Can't a FakeTransportController accomplish this?

> 
> So basically: do we treat the PeerConnection as a giant black box, or treat it
> as something coordinating various pieces and test its interactions between
those
> pieces? This exact situation came up for me recently as well, and I've been
> meaning to ask Peter what his opinion is.

Another option is to make PeerConnection::OnIceConnectionReceivingChange public
and just fire it as if a signal had been fired.  That would save us a lot of
work and makes tests that are pretty much just as good.

[Taylor Brandstetter, 2016-05-27 19:32:43]

On 2016/05/27 17:21:43, pthatcher1 wrote:
> On 2016/05/25 22:07:12, Taylor Brandstetter wrote:
> > 2. Test a PeerConnection against a complete fake ICE implementation such
that
> we
> > can control when the fake sends STUN pings in order to trigger the bug. Not
> > ideal because it would require a lot of effort to maintain.
> 
> Can't a FakeTransportController accomplish this?

No. A fake transport controller uses fake transports, I'm talking about using
actual sockets that we have fine control over.
 
> Another option is to make PeerConnection::OnIceConnectionReceivingChange
public
> and just fire it as if a signal had been fired.  That would save us a lot of
> work and makes tests that are pretty much just as good.

Making private methods public in order to test something is an anti-pattern that
we already do way too often in my opinion. I'd like to get rid of all the places
where we do this, not add more of them.

[zhihuang1, 2016-06-01 21:11:20]

zhihuang@chromium.org changed reviewers:
+ zhihuang@chromium.org

[zhihuang1, 2016-06-01 21:11:21]

Inject the TransportController into PeerConnection.

https://codereview.webrtc.org/1975453002/diff/40001/webrtc/api/peerconnection...
File webrtc/api/peerconnectionfactory.cc (right):

https://codereview.webrtc.org/1975453002/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnectionfactory.cc:191: }
This empty line should not be deleted.

[Taylor Brandstetter, 2016-06-21 23:50:02]

Good job with the unit test; it ended up very clean.

https://codereview.webrtc.org/1975453002/diff/60001/webrtc/api/peerconnection...
File webrtc/api/peerconnectionfactory.cc (left):

https://codereview.webrtc.org/1975453002/diff/60001/webrtc/api/peerconnection...
webrtc/api/peerconnectionfactory.cc:162: 
Was this line deleted on purpose?

https://codereview.webrtc.org/1975453002/diff/60001/webrtc/api/peerconnection...
File webrtc/api/peerconnectioninterface_unittest.cc (right):

https://codereview.webrtc.org/1975453002/diff/60001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface_unittest.cc:545: bool is_triggered = false;
Maybe rename to "callback_triggered"?

https://codereview.webrtc.org/1975453002/diff/60001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface_unittest.cc:1052:
TEST_F(PeerConnectionInterfaceTest, CloseAndTestCallbackFunctions) {
You could add a comment explaining that this is testing that no callbacks on the
PeerConnectionObserver are called after the PeerConnection is closed.

https://codereview.webrtc.org/1975453002/diff/60001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/1975453002/diff/60001/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.cc:1479: cricket::TransportController*
transport_controller) {
Could we just pass the TransportController into the WebRtcSession constructor?
It seems odd that the WebRtcSession constructs its own TransportController, then
it's immediately replaced.

I know this means updating webrtcsession_unittest.cc, but hopefully it's not too
bad...

[Zhi Huang, 2016-06-22 22:01:08]

zhihuang@webrtc.org changed reviewers:
+ honghaiz@webrtc.org
- pthatcher@webrtc.org, zhihuang@chromium.org

[Zhi Huang, 2016-06-22 22:01:12]

https://codereview.webrtc.org/1975453002/diff/60001/webrtc/api/peerconnection...
File webrtc/api/peerconnectioninterface_unittest.cc (right):

https://codereview.webrtc.org/1975453002/diff/60001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface_unittest.cc:545: bool is_triggered = false;
On 2016/06/21 23:50:02, Taylor Brandstetter wrote:
> Maybe rename to "callback_triggered"?

OK. This is more readable.

https://codereview.webrtc.org/1975453002/diff/60001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/1975453002/diff/60001/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.cc:1479: cricket::TransportController*
transport_controller) {
On 2016/06/21 23:50:02, Taylor Brandstetter wrote:
> Could we just pass the TransportController into the WebRtcSession constructor?
> It seems odd that the WebRtcSession constructs its own TransportController,
then
> it's immediately replaced.
> 
> I know this means updating webrtcsession_unittest.cc, but hopefully it's not
too
> bad...

This is a good point. I can do this.

https://codereview.webrtc.org/1975453002/diff/80001/webrtc/api/peerconnection.cc
File webrtc/api/peerconnection.cc (left):

https://codereview.webrtc.org/1975453002/diff/80001/webrtc/api/peerconnection...
webrtc/api/peerconnection.cc:1246: 
I will add it back before submitting.

[honghaiz3, 2016-06-23 18:36:02]

lgtm

https://codereview.webrtc.org/1975453002/diff/80001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/1975453002/diff/80001/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.cc:477:
transport_controller_.reset(transport_controller);
Could this be assigned in the argument list above?

[Taylor Brandstetter, 2016-06-23 19:42:18]

lgtm with minor comment.

https://codereview.webrtc.org/1975453002/diff/80001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/1975453002/diff/80001/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.cc:477:
transport_controller_.reset(transport_controller);
On 2016/06/23 18:36:02, honghaiz3 wrote:
> Could this be assigned in the argument list above?

Agreed. Also, it would be slightly better if the argument was an
std::unique_ptr, to represent the transfership of ownership. Then in the
initializer list you would have:

transport_controller_(std::move(transport_controller)), ...

[Zhi Huang, 2016-07-11 21:41:19]

zhihuang@webrtc.org changed reviewers:
+ pthatcher@webrtc.org
- deadbeef@webrtc.org, honghaiz@webrtc.org

[Zhi Huang, 2016-07-11 21:41:20]

Hi Peter,
Can you take a look for this as well? Thanks.

https://codereview.chromium.org/1975453002/diff/80001/webrtc/api/webrtcsessio...
File webrtc/api/webrtcsession.cc (right):

https://codereview.chromium.org/1975453002/diff/80001/webrtc/api/webrtcsessio...
webrtc/api/webrtcsession.cc:477:
transport_controller_.reset(transport_controller);
On 2016/06/23 19:42:17, Taylor Brandstetter wrote:
> On 2016/06/23 18:36:02, honghaiz3 wrote:
> > Could this be assigned in the argument list above?
> 
> Agreed. Also, it would be slightly better if the argument was an
> std::unique_ptr, to represent the transfership of ownership. Then in the
> initializer list you would have:
> 
> transport_controller_(std::move(transport_controller)), ...

Done.

[Zhi Huang, 2016-07-27 17:27:55]

The CQ bit was checked by zhihuang@webrtc.org

[Zhi Huang, 2016-07-27 17:27:56]

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org,
honghaiz@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1975453002/#ps100001 (title:
"Make it an unique_ptr when passing the TransportController to WebRtcSession.")

[commit-bot: I haz the power, 2016-07-27 17:27:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-07-27 18:07:29]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-07-27 18:07:38]

Description was changed from

==========
Add PeerConnection IsClosed check.

Add IsClosed check when excuting some functions so that they can return early if
the PeerConnection is closed.
The observer will not be called after the PeerConnection is closed.

BUG=webrtc:5861
==========

to

==========
Add PeerConnection IsClosed check.

Add IsClosed check when excuting some functions so that they can return early if
the PeerConnection is closed.
The observer will not be called after the PeerConnection is closed.

BUG=webrtc:5861

Committed: https://crrev.com/29ff8446c0c89114729f6b8cfe49063c899b5764
Cr-Commit-Position: refs/heads/master@{#13544}
==========

[commit-bot: I haz the power, 2016-07-27 18:07:39]

Patchset 6 (id:??) landed as
https://crrev.com/29ff8446c0c89114729f6b8cfe49063c899b5764
Cr-Commit-Position: refs/heads/master@{#13544}