Move the ICE state transition ASSERTS to a lower level.

Move the ICE state transition ASSERTS to a lower level.

They don't really belong in PeerConnection because the state at that
level can change when a transport channel is removed. That makes almost
any state transition possible.

The asserts are now in P2PTransportChannel (the equivalent to
IceTransport in the spec). Currently it has a reduced set of states,
that don't even take into account writability, but I plan to change
that soon.

BUG=webrtc:4757
R=pthatcher@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/6aefc6307d3ab174dbc2356319a67197d391200a

[Taylor Brandstetter, 2016-05-23 19:55:52]

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

[pthatcher1, 2016-05-25 17:32:47]

https://codereview.webrtc.org/2005573002/diff/1/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (left):

https://codereview.webrtc.org/2005573002/diff/1/webrtc/api/webrtcsession.cc#o...
webrtc/api/webrtcsession.cc:1506: break;
With this move, don't we lose the check against the states changing because of
the aggregate of many transports channel states?  For example, if a transport
channel is added or removed, the state may change in a way that we've been
checking with an assert here, but which wouldn't be caught by an assertion in
the lower-level code.

[Taylor Brandstetter, 2016-05-25 20:54:09]

https://codereview.webrtc.org/2005573002/diff/1/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (left):

https://codereview.webrtc.org/2005573002/diff/1/webrtc/api/webrtcsession.cc#o...
webrtc/api/webrtcsession.cc:1506: break;
On 2016/05/25 17:32:47, pthatcher1 wrote:
> With this move, don't we lose the check against the states changing because of
> the aggregate of many transports channel states?  For example, if a transport
> channel is added or removed, the state may change in a way that we've been
> checking with an assert here, but which wouldn't be caught by an assertion in
> the lower-level code.
> 

That's part of the point of this CL. Any state transition is possible for the
aggregate state (except closed -> something else) and so the ASSERTs here are
invalid.

On that note, I might as well add back an assert that the current state is not
Closed.

[pthatcher1, 2016-05-26 17:59:41]

lgtm

[Taylor Brandstetter, 2016-05-26 22:22:19]

The CQ bit was checked by deadbeef@webrtc.org

[commit-bot: I haz the power, 2016-05-26 22:22:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2005573002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2005573002/20001

[commit-bot: I haz the power, 2016-05-26 22:23:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-26 22:23:41]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_clang_dbg/build...)
  android_compile_x86_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_dbg...)
  android_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/13671)
  android_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_gn_rel/builds/1...)
  ios32_sim_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/7821)
  ios64_gn_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gn_dbg/builds/79)
  ios64_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gn_rel/builds/70)
  ios64_sim_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/7803)
  ios_api_framework on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_api_framework/build...)
  ios_arm64_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/10141)
  ios_arm64_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/10064)
  ios_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/15356)
  ios_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/14004)
  linux_ubsan on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/2402)
  mac_baremetal on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/11685)
  mac_compile_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
  mac_gn_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/3501)
  mac_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/9719)

[Taylor Brandstetter, 2016-05-26 22:42:13]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2016-05-26 22:42:13]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2005573002/#ps40001 (title:
"Fixing patch conflict.")

[commit-bot: I haz the power, 2016-05-26 22:42:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2005573002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2005573002/40001

[commit-bot: I haz the power, 2016-05-26 23:08:36]

Description was changed from

==========
Move the ICE state transition ASSERTS to a lower level.

They don't really belong in PeerConnection because the state at that
level can change when a transport channel is removed. That makes almost
any state transition possible.

The asserts are now in P2PTransportChannel (the equivalent to
IceTransport in the spec). Currently it has a reduced set of states,
that don't even take into account writability, but I plan to change
that soon.

BUG=webrtc:4757
==========

to

==========
Move the ICE state transition ASSERTS to a lower level.

They don't really belong in PeerConnection because the state at that
level can change when a transport channel is removed. That makes almost
any state transition possible.

The asserts are now in P2PTransportChannel (the equivalent to
IceTransport in the spec). Currently it has a reduced set of states,
that don't even take into account writability, but I plan to change
that soon.

BUG=webrtc:4757
R=pthatcher@webrtc.org

Committed: https://crrev.com/6aefc6307d3ab174dbc2356319a67197d391200a
Cr-Commit-Position: refs/heads/master@{#12937}
==========

[Taylor Brandstetter, 2016-05-26 23:08:36]

Description was changed from

==========
Move the ICE state transition ASSERTS to a lower level.

They don't really belong in PeerConnection because the state at that
level can change when a transport channel is removed. That makes almost
any state transition possible.

The asserts are now in P2PTransportChannel (the equivalent to
IceTransport in the spec). Currently it has a reduced set of states,
that don't even take into account writability, but I plan to change
that soon.

BUG=webrtc:4757
==========

to

==========
Move the ICE state transition ASSERTS to a lower level.

They don't really belong in PeerConnection because the state at that
level can change when a transport channel is removed. That makes almost
any state transition possible.

The asserts are now in P2PTransportChannel (the equivalent to
IceTransport in the spec). Currently it has a reduced set of states,
that don't even take into account writability, but I plan to change
that soon.

BUG=webrtc:4757
R=pthatcher@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/6aefc6307d3ab174dbc235631...
==========

[commit-bot: I haz the power, 2016-05-26 23:08:37]

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

[Taylor Brandstetter, 2016-05-26 23:08:37]

Committed patchset #3 (id:40001) manually as
6aefc6307d3ab174dbc2356319a67197d391200a (presubmit successful).

[honghaiz3, 2016-05-27 17:49:00]

On 2016/05/26 23:08:37, Taylor Brandstetter wrote:
> Committed patchset #3 (id:40001) manually as
> 6aefc6307d3ab174dbc2356319a67197d391200a (presubmit successful).

I do not understand this (sorry I have not looked this earlier). 
Isn't the ICE state transition dictated by w3c?
And I think what w3c described is the aggregate states, not the lower-level
internal states.

[Taylor Brandstetter, 2016-05-27 19:36:20]

On 2016/05/27 17:49:00, honghaiz3 wrote:
> On 2016/05/26 23:08:37, Taylor Brandstetter wrote:
> > Committed patchset #3 (id:40001) manually as
> > 6aefc6307d3ab174dbc2356319a67197d391200a (presubmit successful).
> 
> I do not understand this (sorry I have not looked this earlier). 
> Isn't the ICE state transition dictated by w3c?
> And I think what w3c described is the aggregate states, not the lower-level
> internal states.

I made some pull requests recently to change this. They're now defined primarily
at the IceTransport level, with specific rules for how multiple states are
combined into one.