Fixing inconsistency with behavior of `ClearGettingPorts`.

webrtc/p2p/base/p2ptransportchannel.cc

Index: webrtc/p2p/base/p2ptransportchannel.cc
diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc
index 4ebd3a41c44612585ba4340a34d816e5e77fe167..189b4bfec8409f6f068ff8983576e633fd07f5f2 100644
--- a/webrtc/p2p/base/p2ptransportchannel.cc
+++ b/webrtc/p2p/base/p2ptransportchannel.cc
@@ -309,9 +309,14 @@ void P2PTransportChannel::SetRemoteIceMode(IceMode mode) {
 
 void P2PTransportChannel::SetIceConfig(const IceConfig& config) {
   if (config_.continual_gathering_policy != config.continual_gathering_policy) {
-    LOG(LS_INFO) << "Set continual_gathering_policy to "
-                 << config_.continual_gathering_policy;
-    config_.continual_gathering_policy = config.continual_gathering_policy;
+    if (!allocator_sessions_.empty()) {

[Taylor Brandstetter, 2016/07/07 17:52:37]

This class assumes the continual gathering policy doesn't change during the
course of the session. So we should enforce that assumption.

[honghaiz3, 2016/07/21 04:10:59]

If you set continual gathering policy after a session starts, it will have
effect on the next session.
It will also have effect on the current session unless the current session has
been stopped or cleared.

So I am not sure if we should enforce the assumption. 

[Taylor_Brandstetter, 2016/07/21 16:41:01]

This is the problem in my opinion. From an application's perspective it appears
like a race condition; depending on when the policy is changed, it may or may
not have an effect.

Is there is a compelling use case for changing the gathering policy mid-session,
we can allow it. But the behavior would need to be documented and covered
sufficiently by unit tests.

[honghaiz3, 2016/07/21 17:37:25]

In my opinion, the race condition is still there. You only changed it from
"depending on whether the current session is cleared" to "depending on whether
there is a session". 

We don't expect users to change it after session starts, it is just that I do
not see the benefit of enforcing that assumption. 

It may happen that the user created the transport channel and set the config
right away. If there is a pooled session, then the user may fail to set it. 
We can still print out the error message but don't have to stop the user from
setting it.  

If you really want to enforce it, do so only if the current session is cleared. 

[Taylor Brandstetter, 2016/07/21 18:38:38]

Sessions, including pooled sessions, are only added in MaybeStartGathering,
which is triggered by SetLocalDescription. So this condition should be
equivalent to "don't allow the policy to change after SetLocalDescription is
called". So I believe the behavior should be predictable.

Also: the benefit of enforcing the assumption is that it prevents us from
accidentally using a code path in the future that isn't covered by unit tests or
intentionally supported.

[honghaiz3, 2016/07/21 21:22:05]

Can you please make a comment on the parameter continual_gathering_policy about
the assumption that it must be set before SetLocalDescription and cannot be
changed afterwards?

[pthatcher1, 2016/07/22 18:11:22]

While I agree consistent behavior is good, it would be surprising to someone
that call SetConfiguration with continual gathering to not get continual
gathering and then get no error until they check the native log.  If we're going
to make the rule "you can't change the continual gathering setting", then we
should cause SetConfiguration to fail, if that's even possible.

Better would be to cause the current sessions to go into continual gathering
mode.  How hard would that be?

[Taylor Brandstetter, 2016/07/22 19:58:42]

JSEP already says that SetConfiguration only changes the ICE servers and
filtering policy. Other changes are ignored, or in the case of changing
certificates, an InvalidModificationError is thrown. I assumed we'd do one of
those 2 things for our custom ICE options as well.

As for "How hard would that be?": Like I said earlier, we'd need to document the
various caveats and add unit tests for changing the policy at different points
in time. This isn't trivial and I don't think it's worth the maintenance effort
unless we see a compelling use case for changing the policy post-PC-creation.

[pthatcher1, 2016/08/23 00:55:39]

That's a good point about SetConfiguration only allowing ICE servers.  You've
convinced me.

+      LOG(LS_ERROR) << "Trying to change continual gathering policy "
+                    << "when gathering has already started!";
+    } else {
+      LOG(LS_INFO) << "Set continual_gathering_policy to "
+                   << config_.continual_gathering_policy;
+      config_.continual_gathering_policy = config.continual_gathering_policy;
+    }
   }
 
   if (config.backup_connection_ping_interval >= 0 &&
@@ -470,6 +475,13 @@ void P2PTransportChannel::OnCandidatesReady(
 void P2PTransportChannel::OnCandidatesAllocationDone(
     PortAllocatorSession* session) {
   ASSERT(worker_thread_ == rtc::Thread::Current());
+  if (config_.gather_continually()) {
+    LOG(LS_INFO) << "P2PTransportChannel: " << transport_name()
+                 << ", component " << component()
+                 << " gathering complete, but using continual "
+                 << "gathering so not changing gathering state.";
+    return;
+  }
   gathering_state_ = kIceGatheringComplete;
   LOG(LS_INFO) << "P2PTransportChannel: " << transport_name() << ", component "
                << component() << " gathering complete";